-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Fix backwards compatibility with pre-Java11 build-tools users #45633
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix backwards compatibility with pre-Java11 build-tools users #45633
Conversation
|
Pinging @elastic/es-core-infra |
jasontedor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a question. I'm pretty sure that we need JDK 8 here from:
org/elasticsearch/gradle/test/ClusterConfiguration has been compiled by a more recent version of the Java Runtime (class file version 55.0), this version of the Java Runtime only recognizes class file versions up to 52.0
| targetCompatibility = 11 | ||
| sourceCompatibility = 11 | ||
| targetCompatibility = 10 | ||
| sourceCompatibility = 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn’t the source/target compatibility need to be 8 not 10, otherwise we can still get into a trouble if an API from JDK 9 or 10 is used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be quite honest, It's not clear to me why this is 10 or if it needs to be. There are funny interactions here between this and the Groovy plugin as well. This is simply a revert back to what this was before the problematic commit that caused the hadoop builds to begin exploding. I think it's likely we use Java9+ APIs in .java code but that isn't used by elasticsearch-hadoop at runtime so all is well.
This is admittedly very awkward and the true solution is to fix the hadoop build to bring it up to Gradle 5.x but we've been kicking that can down the road. This PR is just to unblock the snapshot release builds for the time being. Essentially, this is just the minimal change to bring us back to a known working state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, in that case, I think we need to stop kicking the can down the road. I don't like that we are held back from using modern features because of this. I also don't like that we're making a change it seems we don't fully understand (myself included)? I don't think this change is going to prevent us from making this mistake yet again, breaking important downstream builds for days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 We had a chat about sorting out the hadoop stuff while back, we just need to prioritize that. I'll put this on the top of my list for when I return from PTO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mark-vieira. ❤️
More Java compatibility issueswith the elasticsearch-hadoop project.
This time we introduced a requirement on Java11 into the 7.x branch. This refactors some things so we can run in a Java8 runtime.