CNDB-11508: Enable our test config explicitly for JDK22 and apply some test fixes #1398
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.
I should remove 1.8
There was a problem hiding this comment.
Maybe add 21?
There was a problem hiding this comment.
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.
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.
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.
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.
Not sure whether this is applicable for our CI, it was applicable for Apache, TBD
There was a problem hiding this comment.
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.
This comment should be removed when things are settled
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
This comment should be removed when things are settled
There was a problem hiding this comment.
those are really just for compilation/running tests, right?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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 |
ekaterinadimitrova2
force-pushed
the
CNDB-11508
branch
from
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.
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. |
ekaterinadimitrova2
force-pushed
the
CNDB-11508
branch
from
0131563
to
3512cbe
Compare
|
Cleaned JDK1.8 from allowed JDKs and fixed a bug in the tooling config. |
ekaterinadimitrova2
force-pushed
the
CNDB-11508
branch
from
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.
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.
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.
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.
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.
unless is probably not needed - the entire script should fail if we attempt to use 8
There was a problem hiding this comment.
now are do not ever define java.version.8 in the script
There was a problem hiding this comment.
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.
perhaps the misc.xml should include 11 by default as well
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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. |
ekaterinadimitrova2
changed the title
ekaterinadimitrova2
changed the title
|
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.
why is it defined here?
There was a problem hiding this comment.
This is not part of the current patches but: This is for compilation, not for runtime
conf/jvm22-server.options
Outdated
| # Java 11 (and newer) GC logging options: | ||
| # See description of https://bugs.openjdk.java.net/browse/JDK-8046148 for details about the syntax | ||
| # The following is the equivalent to -XX:+PrintGCDetails -XX:+UseGCLogFileRotation -XX:NumberOfGCLogFiles=10 -XX:GCLogFileSize=10M | ||
| -Xlog:gc=info,heap*=debug,age*=debug,safepoint=info,promotion*=debug:file=/var/log/cassandra/gc.log:time,uptime,pid,tid,level:filecount=10,filesize=10485760 |
There was a problem hiding this comment.
I suggest to leave it commented out.
file=/var/log/cassandra/gc.log does not confirm to the rest of the configuration where log directory is defined by CASSANDRA_LOG_DIR env and defaults to $CASSANDRA_HOME/logs. /var/log/cassandra is not always created and for the development or may not have right permissions for the user.
This will end up by server not start up due to permission problems.
If you want to fix that, I think you should put $CASSANDRA_LOG_DIR for the directory, and then, in the cassandra.in.sh, where the file is processed, where we have:
for opt in `grep "^-" $JVM_OPTS_FILE` `grep "^-" $JVM_DEP_OPTS_FILE`
do
JVM_OPTS="$JVM_OPTS $opt"
donechange it maybe to something like:
for opt in `grep "^-" $JVM_OPTS_FILE` `grep "^-" $JVM_DEP_OPTS_FILE`
do
JVM_OPTS="$JVM_OPTS `sh -c "echo $opt"`"
doneso that in case we reference env variables they are resolved...
There was a problem hiding this comment.
disregard - I saw the comment in main-5.0 after reviewing this - please use the approach from main-5.0
|
Feedback addressed. @jacek-lewandowski , I believe the main questions are:
|
…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
ekaterinadimitrova2
force-pushed
the
CNDB-11508
branch
from
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 |
|
❌ Build ds-cassandra-pr-gate/PR-1398 rejected by Butler9 new test failure(s) in 4 builds Found 9 new test failures
Found 14 known test failures |
|
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. |
What is the issue
We cannot:
mainon 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
mainbranch. 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
NoSpamLoggerfor log lines that may appear frequently in the logs