[build] cloud-aws doesn't register s3 repos anymore

[dadoonet]

Reported in #11647 (comment)

btw, I think you broke some plugins on Master, cloud-aws doesn't register s3 repos anymore.

org.elasticsearch.common.inject.CreationException: Guice creation errors:

1) Error injecting constructor, java.lang.NoClassDefFoundError: org/apache/http/protocol/HttpContext
  at org.elasticsearch.repositories.s3.S3Repository.<init>(Unknown Source)
  while locating org.elasticsearch.repositories.s3.S3Repository
  while locating org.elasticsearch.repositories.Repository

1 error
    at org.elasticsearch.common.inject.internal.Errors.throwCreationExceptionIfErrorsExist(Errors.java:344)
    at org.elasticsearch.common.inject.InjectorBuilder.injectDynamically(InjectorBuilder.java:178)
    at org.elasticsearch.common.inject.InjectorBuilder.build(InjectorBuilder.java:110)
    at org.elasticsearch.common.inject.InjectorImpl.createChildInjector(InjectorImpl.java:140)
    at org.elasticsearch.common.inject.ModulesBuilder.createChildInjector(ModulesBuilder.java:69)
    at org.elasticsearch.repositories.RepositoriesService.createRepositoryHolder(RepositoriesService.java:404)
    at org.elasticsearch.repositories.RepositoriesService.registerRepository(RepositoriesService.java:368)
    at org.elasticsearch.repositories.RepositoriesService.access$100(RepositoriesService.java:55)
    at org.elasticsearch.repositories.RepositoriesService$1.execute(RepositoriesService.java:110)
    at org.elasticsearch.cluster.service.InternalClusterService$UpdateTask.run(InternalClusterService.java:378)
    at org.elasticsearch.common.util.concurrent.PrioritizedEsThreadPoolExecutor$TieBreakingPrioritizedRunnable.runAndClean(PrioritizedEsThreadPoolExecutor.java:209)
    at org.elasticsearch.common.util.concurrent.PrioritizedEsThreadPoolExecutor$TieBreakingPrioritizedRunnable.run(PrioritizedEsThreadPoolExecutor.java:179)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.NoClassDefFoundError: org/apache/http/protocol/HttpContext
    at com.amazonaws.AmazonWebServiceClient.<init>(AmazonWebServiceClient.java:129)
    at com.amazonaws.services.s3.AmazonS3Client.<init>(AmazonS3Client.java:432)
    at com.amazonaws.services.s3.AmazonS3Client.<init>(AmazonS3Client.java:414)
    at org.elasticsearch.cloud.aws.InternalAwsS3Service.getClient(InternalAwsS3Service.java:153)
    at org.elasticsearch.cloud.aws.InternalAwsS3Service.client(InternalAwsS3Service.java:82)
    at org.elasticsearch.repositories.s3.S3Repository.<init>(S3Repository.java:125)
    at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
    at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
    at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
    at java.lang.reflect.Constructor.newInstance(Constructor.java:422)
    at org.elasticsearch.common.inject.DefaultConstructionProxyFactory$1.newInstance(DefaultConstructionProxyFactory.java:56)
    at org.elasticsearch.common.inject.ConstructorInjector.construct(ConstructorInjector.java:86)
    at org.elasticsearch.common.inject.ConstructorBindingImpl$Factory.get(ConstructorBindingImpl.java:104)
    at org.elasticsearch.common.inject.FactoryProxy.get(FactoryProxy.java:54)
    at org.elasticsearch.common.inject.ProviderToInternalFactoryAdapter$1.call(ProviderToInternalFactoryAdapter.java:47)
    at org.elasticsearch.common.inject.InjectorImpl.callInContext(InjectorImpl.java:865)
    at org.elasticsearch.common.inject.ProviderToInternalFactoryAdapter.get(ProviderToInternalFactoryAdapter.java:43)
    at org.elasticsearch.common.inject.Scopes$1$1.get(Scopes.java:59)
    at org.elasticsearch.common.inject.InternalFactoryToProviderAdapter.get(InternalFactoryToProviderAdapter.java:46)
    at org.elasticsearch.common.inject.InjectorBuilder$1.call(InjectorBuilder.java:201)
    at org.elasticsearch.common.inject.InjectorBuilder$1.call(InjectorBuilder.java:193)
    at org.elasticsearch.common.inject.InjectorImpl.callInContext(InjectorImpl.java:858)
    at org.elasticsearch.common.inject.InjectorBuilder.loadEagerSingletons(InjectorBuilder.java:193)
    at org.elasticsearch.common.inject.InjectorBuilder.injectDynamically(InjectorBuilder.java:175)
    ... 13 more
Caused by: java.lang.ClassNotFoundException: org.apache.http.protocol.HttpContext
    at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
    at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:331)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
    ... 37 more

Closes #12034.

[dadoonet]

@rmuir Can you review it please?

[rmuir]

I'm a bit confused why the delete by query pom needs changes (why only that one?). I guess i don't understand the root issue here: is it that we don't have a proper test-framework module and depend on a test-jar (and maven transitive dependencies do not work there)

[dadoonet]

Exact @rmuir: as we don't have (yet) a module for test framework transitive dependency don't work so anyone who wants to use httpclient in tests has to explicitly define it in the pom.xml.

That's what happened with PR #11516: https://github.com/elastic/elasticsearch/pull/11516/files#diff-ef94b341bb064a2092d371fa4cb7504fR58
but it was unfortunately added in plugins/pom.xml with test scope.

As we don't explicitly define this dependency in cloud-aws (it's a transitive one), its default compile scope was overloaded by the one in plugins/pom.xml.

So my change is only to move this lib in each project which actually needs it. That being said, with the latest commit I pushed, this PR might now fails REST tests.

Yeah: sadly we don't have a REST Test module which brings its needed httpclient dependency.

[dadoonet]

Yeah. Rebasing makes that failing now:

Caused by: java.lang.ClassNotFoundException: org.apache.http.conn.HttpClientConnectionManager

I need to push a new commit

[rmuir]

So my change is only to move this lib in each project which actually needs it. That being said, with the latest commit I pushed, this PR might now fails REST tests

can we avoid this? Seems like instead the ones using it should declare it as compile dependency.

[rmuir]

Also i really think we need a test here. Regardless of what happens here this is too fragile. the aws plugin is not working but no tests fail.

[dadoonet]

It's an anti-pattern IMO (declaring a dependency that is never used directly in your code) but yes I can do it and add a TODO in the pom.xml to remove it when we will have a REST Test module.

Also i really think we need a test here. Regardless of what happens here this is too fragile. the aws plugin is not working but no tests fail.

Some tests exists but have been disabled: elastic/elasticsearch-cloud-aws#211
They only run with -Dtests.thirdparty=true -Dtests.config=/path/to/config

I agree that we seriously need to invest time on this.

[rmuir]

Some tests exists but have been disabled: elastic/elasticsearch-cloud-aws#211
They only run with -Dtests.thirdparty=true -Dtests.config=/path/to/config

I think short-term we need jenkins builds going that run these third party tests. I think they are not happening in jenkins for any of these cloud modules, which is bad, because its the majority of their testing today. I know they are sometimes flaky too but we have to do something.

[rmuir]

REST test framework needs to share some of the blame here. Why in the world does it use httpclient? Why not just use e.g. HttpURLConnection?

I think its dangerous for it to bring in this commonly used dependency only for test purposes. Because its in the test classpath, but may not be in the actual compile classpath and can hide bugs. There might be other bugs around this too.

[dadoonet]

REST test framework needs to share some of the blame here. Why in the world does it use httpclient? Why not just use e.g. HttpURLConnection?

😄 indeed. I did not even think about it!

I can try to look if I can replace it...

[dadoonet]

@rmuir I pushed a new change

[rmuir]

+1 thank you @dadoonet