Skip to content

[KEYCLOAK-17254] [KEYCLOAK-18267] Fixes#8106

Merged
mposolda merged 2 commits intokeycloak:masterfrom
iankko:KEYCLOAK-18267_KEYCLOAK-17254
Jun 3, 2021
Merged

[KEYCLOAK-17254] [KEYCLOAK-18267] Fixes#8106
mposolda merged 2 commits intokeycloak:masterfrom
iankko:KEYCLOAK-18267_KEYCLOAK-17254

Conversation

@iankko
Copy link
Contributor

@iankko iankko commented May 27, 2021 

[KEYCLOAK-17254] Adaptively add the default modular JVM options
to the "javaVmArguments" to start the cache server container with,
if the JVM used to run the cache server is modular (JDK 9+)

Signed-off-by: Jan Lieskovsky <jlieskov@redhat.com>

Fixes issues / errors like:

Listening for transport dt_socket at address: 6001
org.jboss.modules.ModuleNotFoundException: java.se
	at org.jboss.modules.Module.addPaths(Module.java:1266)
	at org.jboss.modules.Module.link(Module.java:1622)
	at org.jboss.modules.Module.relinkIfNecessary(Module.java:1650)
	at org.jboss.modules.ModuleLoader.loadModule(ModuleLoader.java:296)
	at org.jboss.modules.Main.main(Main.java:437)

when running Cross-DC tests on hosts having both JDK 8 & JDK 11 installed (e.g. recent Fedora or RHEL-8.1 systems).

[KEYCLOAK-18267] Fix 'java.lang.NoClassDefFoundError: Could not initialize class org.jboss.marshalling.river.RiverMarshaller'
error for:

* org.keycloak.testsuite.crossdc.LastSessionRefreshCrossDCTest and
* org.keycloak.testsuite.crossdc.SessionExpirationCrossDCTest

tests, when running cross-DC tests with JDK 11

Signed-off-by: Jan Lieskovsky <jlieskov@redhat.com>

Fixes issues / errors like:

java.lang.NoClassDefFoundError">java.lang.NoClassDefFoundError: Could not initialize class org.jboss.marshalling.river.RiverMarshaller
        at org.jboss.marshalling.river.RiverMarshallerFactory.createMarshaller(RiverMarshallerFactory.java:54)
...

when running Cross-DC tests on hosts having both JDK 8 & JDK 11 simultaneously installed with -Pjava11-auth-server profile enabled.

Jan Lieskovsky added 2 commits May 27, 2021 18:05
[KEYCLOAK-18267] Fix 'java.lang.NoClassDefFoundError: Could not initi…
19ab601 
…alize class org.jboss.marshalling.river.RiverMarshaller'

error for:

* org.keycloak.testsuite.crossdc.LastSessionRefreshCrossDCTest and
* org.keycloak.testsuite.crossdc.SessionExpirationCrossDCTest

tests, when running cross-DC tests with JDK 11

Signed-off-by: Jan Lieskovsky <jlieskov@redhat.com>
[KEYCLOAK-17254] Adaptively add the default modular JVM options
862785b 
to the "javaVmArguments" to start the cache server container with,
if the JVM used to run the cache server is modular (JDK 9+)

Signed-off-by: Jan Lieskovsky <jlieskov@redhat.com>
@iankko
Copy link
Contributor Author

iankko commented May 27, 2021

@vramik @vmuzikar @tkyjovsk @pdrozd PTAL (especially at the 2-nd commit once got a chance) **
@mposolda PTAL (especially at the 1-th commit once got a chance) **

Thanks,
Jan

** Of course you are welcome to review the PR as a whole, just your primary look at selected change is appreciated. Thanks, Jan

@iankko
Copy link
Contributor Author

iankko commented May 31, 2021 •
edited
Loading

Corresponding testing pipeline runs for the change:

@tkyjovsk
Copy link
Contributor

tkyjovsk commented May 31, 2021 •
edited by mposolda
Loading

@iankko I'm waiting for PR #7774 for KEYCLOAK-13757 to be reviewed.

I have also cretated branch KEYCLOAK-18267_KEYCLOAK-17254_KEYCLOAK-13757 which includes all these changes including some necessary changes that are needed to merge all of these issues, and I successfully ran the tests:

https://keycloak-jenkins.com/view/pipelines/job/universal-test-pipeline-server/2708/testReport/

I suggest merging this PR first before merging PR #7774 because 1 additional commit is needed to adapt KEYCLOAK-13757 to changes in this PR.

@iankko
Copy link
Contributor Author

iankko commented Jun 1, 2021

@iankko I'm waiting for PR #7774 for KEYCLOAK-13757 to be reviewed.

Briefly went through the code changes & LGTM AFAICT.

I have also cretated branch KEYCLOAK-18267_KEYCLOAK-17254_KEYCLOAK-13757 which includes all these changes including some necessary changes that are needed to merge all of these issues, and I successfully ran the tests:

Nice, thanks!

I suggest merging this PR first before merging PR #7774 because 1 additional commit is needed to adapt KEYCLOAK-13757 to changes in this PR.

Either way WFM. Let's do it following the way which is better for your preference / convenience.

@iankko
Copy link
Contributor Author

iankko commented Jun 2, 2021

@pdrozd @vramik @vmuzikar @mposolda Any feedback / reviews on the PR? (asking because according to the comment above, this would be great to be merged sooner than #7774 one)

PTAL once got a chance

Thanks,
Jan

vmuzikar
vmuzikar approved these changes Jun 3, 2021
View reviewed changes
Copy link
Contributor

@vmuzikar vmuzikar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT (e.g. CrossDC tests are a bit outside of my expertise :)), looks good to me. My only concern is the renaming of infinispan-jboss-marshalling,10.1.8.Final,Apache Software License 2.0 which seemed a bit suspicious to me, but I'm sure it is an intentional change hence approving. @iankko Thanks!

@iankko
Copy link
Contributor Author

iankko commented Jun 3, 2021

AFAICT (e.g. CrossDC tests are a bit outside of my expertise :)), looks good to me. My only concern is the renaming of infinispan-jboss-marshalling,10.1.8.Final,Apache Software License 2.0 which seemed a bit suspicious to me, but I'm sure it is an intentional change hence approving. @iankko Thanks!

Thanks for looking, Vashek! Asked you for review, since looked you touched some of the files, the PR is changing (IIRC that jvm.extra.args functionality was contributed by you. Since changing it again, is great to have your opinion if it's OK to be modified).

And for that file renaming part, yes, it's intentional. Since other (the remaining license files in that directory follow the groupId,artifactId,versionId form (IOW comma character is the delimiter). That renamed file was the only one having dot between groupId & artifactId. So renamed it to follow the naming scheme used by other files.

mposolda
mposolda approved these changes Jun 3, 2021
View reviewed changes
Copy link
Contributor

@mposolda mposolda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iankko @tkyjovsk @vmuzikar Thanks for this work and review. I am approving and merging this to unblock the further work on cross-dc tests.

@mposolda mposolda merged commit cbd4288 into keycloak:master Jun 3, 2021
@iankko
Copy link
Contributor Author

iankko commented Jun 3, 2021

@iankko @tkyjovsk @vmuzikar Thanks for this work and review. I am approving and merging this to unblock the further work on cross-dc tests.

@mposolda Wonderful, TYVM!

@tkyjovsk -^ #7774 should be G2G now too.

@tkyjovsk tkyjovsk mentioned this pull request Jun 3, 2021
Merged
@iankko iankko deleted the KEYCLOAK-18267_KEYCLOAK-17254 branch March 3, 2023 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mposolda mposolda mposolda approved these changes

@vmuzikar vmuzikar vmuzikar approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@iankko @tkyjovsk @mposolda @vmuzikar