Skip to content
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

Manually install JDK #127

Closed
wants to merge 2 commits into from
Closed

Conversation

yuce
Copy link
Collaborator

@yuce yuce commented Aug 6, 2024

No description provided.

@yuce yuce requested a review from JackPGreen August 6, 2024 15:18
@JackPGreen
Copy link
Contributor

Why doesn't it work as is?

I'm guessing this is the failure?

It looks like it's installed the right version of Java, but for some reason the JAVA_HOME env isn't set. Which is weird as it usually is...

I'd rather avoid hardcoding the Java version because it's another place to increment when we upgrade.

@yuce
Copy link
Collaborator Author

yuce commented Aug 6, 2024

@JackPGreen I had to hardcode the path to the JDK in the RC script in the v1.4.z branch of the Go client last time, because the correct JDk wasn't selected. For v1.5..z I use RCD, so this shouldn't be a problem. Maybe later we can migrate v1.4.z to RCD too.

@ihsandemir
Copy link
Collaborator

Why doesn't it work as is?

I'm guessing this is the failure?

It looks like it's installed the right version of Java, but for some reason the JAVA_HOME env isn't set. Which is weird as it usually is...

I'd rather avoid hardcoding the Java version because it's another place to increment when we upgrade.

Can't you just add that environment variable rather than manual install?

@yuce
Copy link
Collaborator Author

yuce commented Aug 6, 2024

@ihsandemir JAVA_HOME is already set. But it's hardcoded to a JDK location which doesn't exist. This PR just installs JDK so it's at the expected location.

@JackPGreen
Copy link
Contributor

@ihsandemir JAVA_HOME is already set. But it's hardcoded to a JDK location which doesn't exist. This PR just installs JDK so it's at the expected location.

https://github.com/hazelcast/hazelcast-go-client/blob/16cd443fbe69554483c9d5758efcfe91506e606e/rc.sh#L189

Can't we remove this instead? Why's it there?

@yuce
Copy link
Collaborator Author

yuce commented Aug 6, 2024

@JackPGreen

Can't we remove this instead? Why's it there?

@JackPGreen I had to hardcode the path to the JDK in the RC script in the v1.4.z branch of the Go client last time, because the correct JDk wasn't selected

@JackPGreen
Copy link
Contributor

I had to hardcode the path to the JDK in the RC script in the v1.4.z branch of the Go client last time, because the correct JDk wasn't selected

Ok - do you know why that was? What JDK was being selected vs expected?

@yuce
Copy link
Collaborator Author

yuce commented Aug 6, 2024

You can see a failing run here:
https://github.com/hazelcast/hazelcast-go-client/actions/runs/10175906206/job/28144201884

The RC jars require JDK 17, so I've updated the workflow to use that here:
hazelcast/hazelcast-go-client@cadeb79

But the RC still uses JDK 8 it finds from somewhere.

Could you have a look at what's wrong?

@yuce
Copy link
Collaborator Author

yuce commented Aug 6, 2024

This is the branch we have to update:
https://github.com/hazelcast/hazelcast-go-client/tree/v1.4.z

@JackPGreen
Copy link
Contributor

But the RC still uses JDK 8 it finds from somewhere.

The runners come with JDK8 out-of-the-box. So I'm guessing that no other JDK was configured in that environment?

You can see a failing run here:
https://github.com/hazelcast/hazelcast-go-client/actions/runs/10175906206/job/28144201884

How can you tell that's a Java-8 related issue?

@yuce
Copy link
Collaborator Author

yuce commented Aug 6, 2024

In the "Start Hazelcast Remote Controller" step, there is this:

Error: LinkageError occurred while loading main class com.hazelcast.remotecontroller.Main
	java.lang.UnsupportedClassVersionError: com/hazelcast/remotecontroller/Main has been compiled by a more recent version of the Java Runtime (class file version 61.0), this version of the Java Runtime only recognizes class file versions up to 55.0

@JackPGreen
Copy link
Contributor

In the "Start Hazelcast Remote Controller" step, there is this:

Error: LinkageError occurred while loading main class com.hazelcast.remotecontroller.Main
	java.lang.UnsupportedClassVersionError: com/hazelcast/remotecontroller/Main has been compiled by a more recent version of the Java Runtime (class file version 61.0), this version of the Java Runtime only recognizes class file versions up to 55.0

Thanks!

I think the problem is on the hazelcast-go-client side.

We install JDK 17 via apt-get:
https://github.com/yuce/hazelcast-go-client/blob/16cd443fbe69554483c9d5758efcfe91506e606e/.github/workflows/test-race.yaml#L22

From my testing, this doesn't seem to set JAVA_HOME btw:
docker run --interactive --tty --rm --platform linux/amd64 ubuntu:latest bash -c "apt-get update && apt-get install -y openjdk-17-jdk-headless maven && printenv"

HOSTNAME=cbb205a95dff
PWD=/
HOME=/root
TERM=xterm
SHLVL=0
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
_=/usr/bin/printenv

But in rc.sh we hardcode JAVA_HOME to a path that I don't think is correct:
https://github.com/yuce/hazelcast-go-client/blob/16cd443fbe69554483c9d5758efcfe91506e606e/rc.sh#L189
Locally it's fine, but I think in the GitHub runner it's different

I've made a branch of hazelcast-go-client 1.4.z where:

  • use actions/setup-java over apt-get install
  • remove explicitly setting JAVA_HOME

And that action looks to work there:

Java version:
openjdk version "17.0.12" 2024-07-16 LTS
{...}
JAVA_HOME: /opt/hostedtoolcache/Java_Zulu_jdk/17.0.12-7/x64
{...}
20:07:20,258  INFO [Main] - Starting Remote Controller Server on port:9701
20:07:20,259  INFO [Main] - TServerEventHandler.preServe - server starts accepting connections

@ihsandemir
Copy link
Collaborator

You can see a failing run here: https://github.com/hazelcast/hazelcast-go-client/actions/runs/10175906206/job/28144201884

The RC jars require JDK 17, so I've updated the workflow to use that here: hazelcast/hazelcast-go-client@cadeb79

But the RC still uses JDK 8 it finds from somewhere.

Could you have a look at what's wrong?

@yuce did you check other clients, how tey start rc, because we do not have this problem for them. You can check python or .NET as most up to date ones. I wonder what are we doing different here.

@yuce
Copy link
Collaborator Author

yuce commented Aug 7, 2024

@JackPGreen I remember something complaining about JAVA_HOME not set correctly, so I've set it in rc.sh in ``v1.4.z`.

I think GH sets the PATH so that the default JDK on that container has priority over the one I install using apt. That's why I had to change the path to use the one install.

Anyway, I think your solution is better than updating rc.sh. Could you send a PR to v1.4.z?

Thanks for coming up with this solution.

@yuce
Copy link
Collaborator Author

yuce commented Aug 7, 2024

@ihsandemir Python and .Net clients don't use rc.sh. Apart from the v1.4.z branch of the Go client, C++ client is the only one that uses it.

This problem didn't arise before, since RC jar was compiled with JDk 8. You may get the same problem if you run the C++ client tests (and GH workflow installs JDK using apt)

JackPGreen added a commit to JackPGreen/hazelcast-go-client that referenced this pull request Aug 7, 2024
The [tests are failing](https://github.com/hazelcast/hazelcast-go-client/actions/runs/10175906206/job/28144201884) because of a conflict between:
- `JAVA_HOME`
- Multiple Java installations on the runner

Based on my [investigation and testing](hazelcast/client-compatibility-suites#127 (comment)) using `setup-java`, and avoiding hardcoding, resolves these issues.
JackPGreen added a commit to JackPGreen/hazelcast-cpp-client that referenced this pull request Aug 7, 2024
In hazelcast/client-compatibility-suites#127 (comment) it was identified that using `apt-get` to install Java on GitHub runners can cause issues with Maven using the right JDK, and that using `setup-java` instead resolves that.

Fixes: hazelcast#1222 🤞
JackPGreen added a commit to hazelcast/hazelcast-go-client that referenced this pull request Aug 7, 2024
The [tests are
failing](https://github.com/hazelcast/hazelcast-go-client/actions/runs/10175906206/job/28144201884)
because of a conflict between:
- `JAVA_HOME`
- Multiple Java installations on the runner

Based on my [investigation and
testing](hazelcast/client-compatibility-suites#127 (comment))
using `setup-java`, and avoiding hardcoding, resolves these issues.
@JackPGreen
Copy link
Contributor

JackPGreen commented Aug 7, 2024

Anyway, I think your solution is better than updating rc.sh. Could you send a PR to v1.4.z?

hazelcast/hazelcast-go-client#1005

This problem didn't arise before, since RC jar was compiled with JDk 8. You may get the same problem if you run the C++ client tests (and GH workflow installs JDK using apt)

I'll have a look at that as well...
hazelcast/hazelcast-cpp-client#1227

@JackPGreen
Copy link
Contributor

Anyway, I think your solution is better than updating rc.sh. Could you send a PR to v1.4.z?

hazelcast/hazelcast-go-client#1005

@yuce based on ^ that ^ do we still need this PR? Or do you want to retest?

@yuce
Copy link
Collaborator Author

yuce commented Aug 7, 2024

@JackPGreen Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants