[DRAFT] Build this project and unit test using JDK 8. #157
Conversation
…ompile and run using JDK 11. Uses OpenSearch low-level client 1.3.2 since this supports Java 8. opensearch-project#156 Signed-off-by: David Venable <[email protected]>
|
Add JDK8 CI as part of this PR please. @reta WDYT? |
|
@dblock I feel uneasy about this change: I have not seen any strong argument from the community that JDk-8 is needed at all, taking into account that large number of projects have switched to at least JDK-11, if not JDK-17. |
| implementation("org.opensearch.client", "opensearch-rest-client", opensearchVersion) | ||
| testImplementation("org.opensearch.test", "framework", opensearchVersion) | ||
| implementation("org.opensearch.client", "opensearch-rest-client", "1.3.2") | ||
| integrationTestImplementation("org.opensearch.test", "framework", opensearchVersion) |
There was a problem hiding this comment.
If anything, we should not take this path: opensearch-rest-client of one version, framework of another version, just my opinion. The clean solution would be to have: a) concise Opensearch version b) clean build under 8 and 11 toolchains.
There was a problem hiding this comment.
@reta , I mentioned the 1.3.2 problem in the PR description. And I fully agree that this is not the right approach - it is temporary to have a working build. I aim to be able to remove with opensearch-project/OpenSearch#3181. But, without that merged in, this would not work.
There was a problem hiding this comment.
Ok, thanks @dlvenable , I still don't understand how it is supposed to work with framework (the opensearch-rest-client will be fine): the JDK-8 toolchain will fail trying to use JDK-11 bytecode.
There was a problem hiding this comment.
This PR builds and runs unit tests against JDK 8. It runs the integration tests using JDK 11 to support the framework library.
There was a problem hiding this comment.
Uh ... thanks @dlvenable , @dblock supporting this JDK-8/JDK-11 "split brain" across project would be quite tricky, we need a really compelling reason to go with this change (#156 has no evidence it is really needed, @dlvenable please correct me if I am wrong).
There was a problem hiding this comment.
Also, I do think this is a mess, and OpenSearch is exposing this mess to Java clients. I'd like to break that somehow.
There was a problem hiding this comment.
There was a problem hiding this comment.
The low-level REST client provides very little value to this library. I suggest you just git rid of it and use its underlying HTTP transport instead. One less dependency to worry about.
There was a problem hiding this comment.
I agree. Maybe you want to take a stab at that @mtimmerm? :)
Description
This PR shows initial work to support JDK 8 for this client, which will allow Java 8 applications to continue to use OpenSearch 2 and beyond.
This is a draft for a few reasons:
opensearch-rest-client1.3.2. See Support Java 8 for opensearch-rest-client OpenSearch#3181opensearch-rest-client, event with Java 8, reports it is Java 11. I'll look further into this if we get general agreement on the approach.Issues Resolved
This draft demonstrates a possible solution for #156.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.