-
Notifications
You must be signed in to change notification settings - Fork 22
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
CNDB-11508: Enable our test config explicitly for JDK22 and apply some test fixes #1398
base: main
Are you sure you want to change the base?
Conversation
build.xml
Outdated
--> | ||
<property name="java.default" value="11" /> | ||
<!-- Not sure whether we need 17 below, at least jvm options were added some time ago for 17 --> | ||
<property name="java.supported" value="1.8,11,17,22" /> |
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 should remove 1.8
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.
Maybe add 21?
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.
Is 1.8 actually still supported? I don't think it is. I would leave this as 11/17/22 as those are the JVMs we are using.
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.
Yeah, I think what we agreed is the cleaning of dead config for JDK8 is done in main-5.0
but we can remove it from supported here.
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.
Given we are compiling our binaries with 11, I think we should start making things show meaningful errors if someone uses 8 rather than the strange error you get when it actually tries to run.
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.
Yes, I removed 1.8 and now it shows: Unsupported JDK version used: 1.8
Same as any other version which is not on that list of java.supported
build.xml
Outdated
<!-- JDKs supported. | ||
All releases are built with the default JDK. | ||
Builds with non-default JDKs are considered experimental and for development and testing purposes. | ||
When building, javac's source and target flags are set to the jdk used, so lower JDKs are not supported at runtime. | ||
The use of both CASSANDRA_USE_JDK11 and use-jdk11 is deprecated. |
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.
Not sure whether this is applicable for our CI, it was applicable for Apache, TBD
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 don't think we need this comment
<string>-XX:G1RSetUpdatingPauseTimePercent=5</string> | ||
<string>-XX:MaxGCPauseMillis=100</string> | ||
|
||
<!-- Copy-paste for now these options from cndb/docker-entrypoint.sh; we will verify later whether we |
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 comment should be removed when things are settled
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.
How do these all work? You mention these are for tests in the comment, why are there both _jvm22_arg_items and _jvm22_test_arg_items?
It would be nice if we did not need to have all these duplicated here and also in the server.options files, but that is probably something for another ticket.
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.
It would be nice if we did not need to have all these duplicated here and also in the server.options files, but that is probably something for another ticket.
Yes and no. Good reference: CASSANDRA-18263
So pre-CMS being out - Apache Cassandra was using CMS for testing, but a different config for prod. Optimization for our tests or so. When we moved past JDK8 - CASSANDRA-18263 was born. Until someone determines what we do in tests, we will keep the same configuration as the server.options files for now. So far no one has reported any issues around that, and I am not sure we will get to differ anything again. Exactly the opposite, @jacek-lewandowski did very good points in favor to keep G1GC and the same config as prod.
But there is one more caveat - sometimes we need to open/export some JDK internals just for some of the test frameworks we use, but not for prod code. In those cases we will want that to be done only in build.xml.
TLDR - let's consider a change in a different ticket.
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 different ticket, and OSS first in this case;
we should get server.properties + extra options for testing;
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 believe CASSANDRA-18263 can be used for that and there is no need of additional one. I will leave a comment there that we may want to reconsider how we stream options between server.options and build.xml to reduce duplications and minimize the potential for bugs.
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.
Actually, I think in this part we won't be sharing much with OSS because we will likely be ahead all the time with JVM support.
Therefore, I suggest doing the following:
- Sets of exports and opens of the 11 are included in the corresponding sets of the 22, leverage that fact just having j11 args with them and j22 args with extra stuff j22 needs; then add both to the command line
- Don't use different GC settings, we can share that as well
This way we can avoid duplications and problems
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 disagree here. I want to be able to see what is needed for which JDK and have things split. We also remove some options in newer JDKs or add new ones, and we want to mimic what we use in tests to what we use in the jvm options files.
Example:
We need <string>--add-opens java.base/jdk.internal.util.jar=ALL-UNNAMED</string>
only with JDK11.
By not including it in the tests for JDK17, I saw in tests I am still getting warnings that it is not needed and it triggered me to find out that we do not read the right jvm options file in Python dtests (which mimic real world where we use those files and not build.xml). And this was after I removed the option from the file I was supposed to use with newer JDK.
Not having explicit config per JDKs is also risky for people not to realize what they test or what they are supposed to test.
<!-- Below two are used in Cassandra, not in CNDB--> | ||
<!--<string>-XX:MaxTenuringThreshold=1</string> | ||
<string>-XX:G1HeapRegionSize=16m</string>--> |
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 comment should be removed when things are settled
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.
those are really just for compilation/running tests, right?
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.
The options in build.xml are only for testing, yes. In prod we would use the ones in server.properties files for self-managed. For Astra, they are at a different place.
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.
The the comment above does not seem to be relevant at all - for building and testing we do not do anything specific to CNDB
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 test code that will be run in CNDB, so we would love to test similar config. Does this make sense? Do we want those options or should I just remove them together with the 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.
Unit tests environment is pretty different to the production setup; we usually do not even start full server; also, unit tests should be as JVM configuration agnostic as possible
@@ -50,9 +50,6 @@ fi | |||
if [[ -z "${java_version}" ]]; then | |||
java_version=$(java -version 2>&1 | awk -F '"' '/version/ {print $2}' | sed 's/^1\.//g' | awk -F. '{print $1}') | |||
fi | |||
if [[ ${java_version} -ge 11 ]]; then |
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.
Testing will show whether this is enough and whether CI is affected in some way by this change
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 we use the new CCM, we should not observe problems; it should just use the Java version from the environment
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 am not sure which the new CCM is, but so far I did not see any problems.
The PR is not finished, but I wanted to do preliminary JDK11 testing to see whether I broke anything so far |
b68440f
to
286bf05
Compare
tools/bin/cassandra.in.sh
Outdated
|
||
JAVA_VERSION=11 | ||
if [ "$JVM_VERSION" = "1.8.0" ] ; then |
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 think we should just require 11+ given we are doing our builds with 11.
I think this looks good for a full review. When you put it up for full review, please include links to 11/17/22 based CI runs. |
0131563
to
3512cbe
Compare
Cleaned JDK1.8 from allowed JDKs and fixed a bug in the tooling config. |
3512cbe
to
42378b4
Compare
There was some issue with CI. |
build.xml
Outdated
<!-- JDKs supported. | ||
All releases are built with the default JDK. | ||
Builds with non-default JDKs are considered experimental and for development and testing purposes. | ||
When building, javac's source and target flags are set to the jdk used, so lower JDKs are not supported at runtime. | ||
The use of both CASSANDRA_USE_JDK11 and use-jdk11 is deprecated. |
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 don't think we need this comment
When building, javac's source and target flags are set to the jdk used, so lower JDKs are not supported at runtime. | ||
The use of both CASSANDRA_USE_JDK11 and use-jdk11 is deprecated. | ||
--> | ||
<property name="java.default" value="11" /> |
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.
Not sure - why do we need this? Default Java version is the version of Java we run the build with
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.
Exactly, it is a placeholder one-stop-place to bump when we start building instead with another version.
We also warn people if they build on their own with different version that it is not a default one we ship with.
<!-- Below two are used in Cassandra, not in CNDB--> | ||
<!--<string>-XX:MaxTenuringThreshold=1</string> | ||
<string>-XX:G1HeapRegionSize=16m</string>--> |
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.
those are really just for compilation/running tests, right?
<replace file="${eclipse.project.name}.iml" token="JDK_1_8" value="JDK_11"/> | ||
<replace file=".idea/misc.xml" token="JDK_1_8" value="JDK_11"/> | ||
<replace file=".idea/misc.xml" token="1.8" value="11"/> | ||
<target name="_maybe_update_idea_to_java11plus" depends="init" unless="java.version.8"> |
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.
unless is probably not needed - the entire script should fail if we attempt to use 8
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.
now are do not ever define java.version.8
in the script
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.
It is already failing with:
Buildfile: /Users/ekaterina.dimitrova/cassandra/build.xml
BUILD FAILED
//cassandra/build.xml:208: Unsupported JDK version used: 1.8
This goes to the follow up ticket for removal the dead codecfrom JDK8 I suggest.
Please check my suggestion here: #1398 (comment)
<replace file=".idea/misc.xml" token="1.8" value="11"/> | ||
<target name="_maybe_update_idea_to_java11plus" depends="init" unless="java.version.8"> | ||
<replace file="${eclipse.project.name}.iml" token="JDK_1_8" value="JDK_${ant.java.version}"/> | ||
<replace file=".idea/misc.xml" token="JDK_1_8" value="JDK_${ant.java.version}"/> |
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.
perhaps the misc.xml
should include 11 by default as well
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.
You mean we should update the token in misc.xml? Or anything else?
I was trying to skip the cleaning from CASSANDRA-18255 on main
as we already have it in main-5.0
to keep it simple. As less changes we have, the less we can expect missed issues. Let me know if you prefer to clean 8 completely too.
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 think so, I don't expect breaking changes coming form OSS 4.0 so at this point, when we have main-5.0 separated, we should optimize the build for simplicity and our needs.
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.
That's good, but we are trying to get rid from main
in favor of main-5.0
, not to optimize it.
Can we at least port CASSANDRA-18255 in a separate ticket, please? This one is already big and got delayed. Incremental changes
@@ -2205,6 +2238,8 @@ | |||
|
|||
<!-- Generate IDEA project description files --> | |||
<target name="generate-idea-files" depends="init,maven-ant-tasks-init,resolver-dist-lib,gen-cql3-grammar,generate-jflex-java,createVersionPropFile" description="Generate IDEA files"> | |||
<delete dir=".idea"/> |
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.
please don't delete .idea
. Whenever you want to just update your project you will lose all your local configuration, like configured tests, etc. Please just delete what we need to replace.
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 is already in 5.0 after discussion on the mailing list - https://lists.apache.org/thread/o2fdkyv2skvf9ngy9jhpnhvo92qvr17m
Do you want to change it in datastax/cassandra all branches? It seems there was consensus.
By the way what config files for testing you have that may be useful and we can actually add for everyone? maybe?
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 someone runs ant generate-idea-files
that says to me they want to start from freshly generated files? Otherwise why are you running it?
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 run it when we switch between JDK versions for example. Before this change I had to invalidate caches and restart IDE almost every time...
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.
For example, I run generate-idea-files
to update the libraries when updating or switching branch - I don't want to lose all the run configurations I had
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.
actually this is the scenario which always annoyed me: I've added/removed a dependency (could by branch update or explicitly) - I need to do clean and generate idea files again. I'm losing everything, all the setups - for example I had test configurations prepared to run with compression or something else, that is gone. Now, it will be gone even without explicitly calling realclean.
generate-idea-files
will be even worse now - it still does not refresh the libraries directory (only appends new libraries) but it removes the custom configurations
I understand the motivation, but I'm sceptical about side-effects. Can we not include this change here?
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.
Now, it will be gone even without explicitly calling realclean.
On my end I almost never can switch branches properly without using realclean
.
generate-idea-files will be even worse now - it still does not refresh the libraries directory (only appends new libraries) but it removes the custom configurations
I honestly never hit that problem. I always do ant realclean ant jar ant generate-idea-files
when switching branches. Only the case of config makes sense to me and you made me wonder whether we want to add compression config somehow.
I understand the motivation, but I'm sceptical about side-effects. Can we not include this change here?
I can do it for main
, but it is already accepted in Apache and it is in main-5.0
. Do you suggest to remove it in main-5.0 too? So far I heard positive feedback from others but I want to make your life easy too. Maybe we open follow up issue to think how we can improve explicitly that target? And for now we just remove the deletion part here and leave things as they are in main-5.0
? Let me know, thanks
Thank you for the reviews. |
CI is in a bad shape today going up and down and I am not being able to really check test results. |
<equals arg1="${ant.java.version}" arg2="11"/> | ||
</condition> | ||
<condition property="_std-test-jvmargs" value="${_jvm22_test_arg_items_concat}"> | ||
<equals arg1="${ant.java.version}" arg2="22"/> | ||
</condition> | ||
|
||
<!-- needed to compile org.apache.cassandra.utils.JMXServerUtils --> | ||
<!-- needed to compile org.apache.cassandra.distributed.impl.Instance--> | ||
<!-- needed to compile org.apache.cassandra.utils.memory.BufferPool --> | ||
<property name="jdk11plus-javac-exports" value="--add-exports java.rmi/sun.rmi.registry=ALL-UNNAMED --add-exports java.rmi/sun.rmi.transport.tcp=ALL-UNNAMED --add-exports java.base/jdk.internal.ref=ALL-UNNAMED --add-exports java.base/sun.nio.ch=ALL-UNNAMED"/> |
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.
why is it defined here?
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 is not part of the current patches but: This is for compilation, not for runtime
Feedback addressed. @jacek-lewandowski , I believe the main questions are:
|
04c61e3
to
4990896
Compare
Thank you for the reviews! There are a few conversations about how we can improve/change certain things like |
The PR failed with no space left - https://jenkins-stargazer.aws.dsinternal.org/job/ds-cassandra-pr-gate/job/PR-1398/6/cloudbees-pipeline-explorer/. I will follow up with the db team as there was some ongoing work on these matters yesterday. |
Pushed empty commit for new CI run |
The failing tests marked as regressions are not related, and we've seen before - timeouts, some leakage we see a lot these days in CI. They did not also fail locally for me. |
7956576
to
53e0648
Compare
The two failures marked as regressions here are legit. The regressions marked in Butler UI are not regressions. |
53e0648
to
6756cf7
Compare
…roperties `java.default` and `java.supported`. ant generate-idea-files now support JDK 8, JDK 11 and JDK 22. To add support of another JDK the java-jvmargs property must be set for the JDK in question (see how it's done in build.xml for Java 11 and 22) Other minor, but notable changes are: - test jvmargs are now added to idea run configurations - .idea dir and project iml file are first removed and then recreated during `ant generate-idea-files` Based on what was done in CASSANDRA-18467, CASSANDRA-18179, CASSANDRA-18258 for 17 plus additional stuff for 21 Co-authored-by: Ekaterina Dimitrova<[email protected]> Co-authored-by: Mick Semb Wever <[email protected]> Co-authored-by: Jakub Zytka <[email protected]>
…dit logger and vectorizedMismatch
…his was already done with the main-5.0 patch, but forgotten here
6756cf7
to
687ec9b
Compare
Quality Gate passedIssues Measures |
❌ Build ds-cassandra-pr-gate/PR-1398 rejected by Butler1 new test failure(s) in 7 builds Found 1 new test failures
Found 28 known test failures |
What is the issue
We cannot:
main
on JDK22What does this PR fix and why was it fixed
Porting CASSANDRA-18467, CASSANDRA-18179, CASSANDRA-18258 and adding updates based on testing we already did in a POC branch will enable us to build and run Cassandra on JDK22 plus run all test suites on JDK22 from the
main
branch. We also added numerous fixes that get CI to a very good state on JDK22.Removes the use of CASSANDRA_USE_JDK11 (we should verify that will not affect CI systems and other infra scripts in any way) and introduces the build.xml properties java.default and java.supported.
ant generate-idea-files now support JDK 8, JDK 11, and JDK 22.
To add support for another JDK the java-jvmargs property must be set for the JDK in question (see how it's done in build.xml for Java 11 and 22)
Other minor, but notable changes are:
Adds some additional add-opens and add-exports needed and upgrades mockito for testing.
CI runs posted on the GH issue.
Checklist before you submit for review
NoSpamLogger
for log lines that may appear frequently in the logs