-
Notifications
You must be signed in to change notification settings - Fork 35
minor fixes to permit running with Java 25 in restricted mode. #272
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
base: 3.0.X
Are you sure you want to change the base?
Conversation
| } | ||
| if (p0 > 11) { | ||
| System.err.println( | ||
| "Warning: Java versions > Java 11 can only operate in restricted mode where no off-heap operations are allowed!"); |
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.
this makes sense, the only concern is, will it be too noisy? maybe add a static field JAVA_VERSION_WARNING_PRINTED as a flag, then we can print it just once.
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.
Good point, but I’m not sure how to do that. I would think that even a static flag would be called every time a class is loaded. Which could be a lot.
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.
A field access is relatively cheap compared to the version check logic
|
@leerho Awesome, thanks a lot! For the testing plan you suggested at #271 (comment),
It already passes our internal test cases, which are a subset of Apache Spark's official test cases. The most confident approach would be:
|
|
We can certainly test with a snapshot, but ultimately it will need to be released. And DS-Java will need a release to reference it. All this pain is why it is essential to get Spark on 25! And since we will have to release I found some issues with the GHA workflows, which I want to fix. |
|
@leerho I can override the ds-memory version directly in I have opened a draft PR on the Spark repo to collect anything that benefits Spark supports Java 25, the testing work will be based on that. |
| static final String NOT_MAPPED_FILE_RESOURCE = "This is not a memory-mapped file resource"; | ||
| static final String THREAD_EXCEPTION_TEXT = "Attempted access outside owning thread"; | ||
|
|
||
| private static boolean JAVA_VERSION_WARNING_PRINTED = false; |
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.
checkJavaVersion can be called concurrently, it can be addressed by using AtomicBoolean, and the caller side can be
if (p0 > 11 && JAVA_VERSION_WARNING_PRINTED.compareAndSet(false, true)) {
// print warning message
}
but this is not a big deal, there is no hurt to print a few more times.
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! What I implemented so far is simpler, I think. I'm sure using an Atomic Boolean will also work, but I'm not sure that Atomic Boolean is really necessary here since none of our sketches are thread safe anyway.
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.
If you really need thread concurrency, you need to wrap the appropriate classes with a synchronized API. :)
Although it looks like a lot of changes, it actually consists of the following:
"Warning: Java versions > Java 11 can only operate in restricted mode where no off-heap operations are allowed!"
None of these changes constitute a change in core logic.