Fix check java version() in 3.0.x

[leerho]

This applies the same fix to 3.0.X as was applied to master. Cherry-picking worked!

[Jackie-Jiang]

Hi Lee, this change is breaking the java 21 build. Could you share some more context on why not allowing java 21 to use this library?

[leerho]

Please read the README: to wit:
https://github.com/apache/datasketches-memory?tab=readme-ov-file#release-300-400

You will have to wait until we have a version designed to work with 17 and 21++

[leerho]

Which I hope will be before the end of this year.

[leerho]

Hi Jackie,
Sorry, I didn't mean to be curt. But the original ds-memory was written using JVM internals such as unsafe, vm, bits, etc. in order to achieve dramatic speed improvements. And we have enjoyed this enhanced performance for almost 10 years. But later versions of java after Java11 have removed all access to these functions or changed the code significantly even if I could get access. Meanwhile, Java has not provided anything comparable to unsafe, etc until Java 17 with the Panama project which was only in incubation as of Java 17. I have a version of ds-memory now working for Java 17, but I need to finish and release it. Also even Panama API changed considerably between Java 17 and Java 21 and hasn't become part of the language until Java 22 (wrt LTS releases, Java 25).

Even preparing the Java 17 release is proving a challenge. It should run under 21 but will not compile under 21.

I have been pulled off of this work onto other priorities and hope to get back to it as soon as I can.

Lee.

[Jackie-Jiang]

Thanks for the detailed response! We are already on 6.0.0 and is running java 21. Do you see potential issues running it with our current settings?

[leerho]

I’m afraid you will have problems with 21. You may be able to run on 17 as long as you don’t allocate off-heap memory. Directly from DS-memory. Sorry, but we have extremely limited resources. From my cell phone.

On Thu, Sep 19, 2024 at 6:07 PM Xiaotian (Jackie) Jiang < ***@***.***> wrote: Thanks for the detailed response! We are already on 6.0.0 and is running

java 21. Do you see potential issues running it with our current settings? — Reply to this email directly, view it on GitHub <#209 (comment)> TV, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADCXRQTSS7XXJSPW3GLZKPDZXNYLPAVCNFSM6AAAAABNJJS6B2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRSGUYTCNZYGI> .

…

You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[gortiz]

Hi there! Another Apache Pinot contributor here. We have been using datascketches for a while in Java 21 at runtime and we didn't find any issue so far. It seems that this PR is being a bit paranoid and doesn't let us use datasketches even if we do not call the methods that may be problematic in Java 17 and 21.

But later versions of java after Java11 have removed all access to these functions or changed the code significantly even if I could get access. Meanwhile, Java has not provided anything comparable to unsafe, etc until Java 17 with the Panama project which was only in incubation as of Java 17. I have a version of ds-memory now working for Java 17, but I need to finish and release it. Also even Panama API changed considerably between Java 17 and Java 21 and hasn't become part of the language until Java 22 (wrt LTS releases, Java 25).

We had the same issue. We provide our own buffer library (see https://github.com/apache/pinot/tree/master/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/memory/unsafe) which seems to be pretty similar to your Memory class (including using longs as offsets). As you may expect, we had the same problem when we tried migrate from Java 11 to 21. In our case the main issue was how to release the allocated memory, given these internal methods have changed in different runtimes.

Our solution was to abstract the release function and have one implementation that using reflection calls the method that actually exist in the runtime we are running (see MapSection and the different implementations). Do you think you can do something like that? Maybe we can help you to do so?

Alternatively, could be possible to disable the check added here? I understand you want to by default fail indicating that the JVM is not supported but could be possible to add a system property or something like that we can provide to say we assume the risk? ie something like:

  public static void checkJavaVersion(final String jdkVer, final int p0, final int p1) {
    final boolean skipCheck = Boolean.getBoolean(Constants.SKIP_JAVA_RUNTIME_CHECK)
    final boolean ok = ( ((p0 == 1) && (p1 == 8)) || (p0 == 8) || (p0 == 11) || (p0 == 17) );
    if (!skipCheck && !ok) {
      throw new IllegalArgumentException(
          "Unsupported Runtime JDK Major Version, must be one of 1.8, 8, 11, 17: " + jdkVer);
    }
  }

[leerho]

I can see that this is quite painful, and you have come up with a rather clever solution!
Let me study this and get back to you.

[leerho]

If adding the system property would hold you over until I can get the Java 17 version released (which should run on 21), then please submit a PR to do that. --Thanks for your help on this!

[leerho]

Is it ok with you that you pull this updated code directly from the github site?

[gortiz]

I was trying to open a PR to update the code, but it looks like this code have changed significantly and now checkJavaVersion is defined in ResourceImpl as:

  static void checkJavaVersion(final String jdkVer, final int p0, final int p1 ) {
    final boolean ok = ((p0 == 1) && (p1 == 8)) || (p0 == 8) || (p0 == 11) || (p0 == 17 || (p0 == 21));
    if (!ok) { throw new IllegalArgumentException(
        "Unsupported JDK Major Version. It must be one of 1.8, 8, 11, 17, 21: " + jdkVer);
    }
  }

So it doesn't seem to be needed right?

[leerho]

The running with Java 17 or Java 21 should be resolved with release 3.0.2.

[Jackie-Jiang]

Thanks @leerho for the update! Will there be a new release of datasketches-java which picks this new dependency version?

[leerho]

Yes, it is in Release Process now.

[leerho]

You can vote on it! Check out ***@***.***

…

On Mon, Oct 7, 2024 at 11:17 AM Xiaotian (Jackie) Jiang < ***@***.***> wrote: Thanks @leerho <https://github.com/leerho> for the update! Will there be a new release of datasketches-java which picks this new dependency version? — Reply to this email directly, view it on GitHub <#209 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADCXRQSTT2CECJRBDEC4CRDZ2LF2RAVCNFSM6AAAAABNJJS6B2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJXGU4DMOJQHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>