Run integration test with a given Java version and vendor #1749

Torch3333 · 2024-05-20T07:43:17Z

Description

To verify that ScalarDB is compatible with JDK with version >8, we can now determine which SDK version and vendor (any version and vendors are possible) are used to run the integration test.
Additionally, when triggering the CI manually, we can determine the JDK LTS version (8, 11, 17, or 21) and vendor (Temurin, Oracle, Microsoft, or Amazon) used to run the integration test.

Related issues and/or PRs

N/A

Changes made

Define three optional Gradle properties to configure which SDK is used by compilation or integration test tasks, for example :

gradle integrationTestJdbc -PintegrationTestJavaVersion=11 -PintegrationTestJavaVendor=temurin

~~- compilationJavaVendor : configure the SDK vendor used by compilation tasks~~

integrationTestJavaVersion : configure all tasks name starting with "integrationTest" to run with the given SDK version
integrationTestJavaVendor : configure all tasks name starting with "integrationTest" to run with the given SDK vendor

Gradle will search the system for a JDK that matches the criteria; if Gradle cannot detect the desired SDK, please refer to the Gradle Toolchain documentation.

Update the project build to use Gradle 8.7 (the latest version), which allows to configure tasks to run with the latest Java version (it uses the toolchain feature)

Remove the com.palentir.docker plugin and call directly the Docker cli for docker task
Slight refactoring of the base plugin configuration to reduce deprecation warnings.

When manually triggering the CI, the SDK version and vendor used to run the integration test are configurable.

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation to reflect the changes.
Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
Tests (unit, integration, etc.) have been added for the changes.
My changes generate no new warnings.
Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

N/A

Release notes

N/A

Torch3333 · 2024-05-21T00:34:25Z

.github/workflows/ci.yaml

@@ -1,19 +1,44 @@
 name: CI
+run-name: "${{ github.event_name == 'workflow_dispatch' && format('Dispatch : Run integration test with JDK {0} ({1})', inputs.int_test_java_version, inputs.int_test_java_vendor) || '' }}"


When the workflow is triggered manually, set a specific name to identify which SDK version and vendor were used to run the integration test.
It uses a ternary operator syntax explained here

Torch3333 · 2024-05-21T00:37:58Z

.github/workflows/ci.yaml

+ inputs:
+ int_test_java_version:
+ description: JDK version used to run the integration test
+ type: choice
+ required: false
+ default: '8'
+ options:
+ - '8'
+ - '11'
+ - '17'
+ - '21'
+ int_test_java_vendor:
+ description: Vendor of the JDK used to run the integration test
+ type: choice
+ required: false
+ default: 'temurin'
+ options:
+ - 'corretto'
+ - 'microsoft'
+ - 'oracle'
+ - 'temurin'


The parameters when triggering the workflow manually to select the JDK version and vendor for the integration test.

Torch3333 · 2024-05-21T00:40:37Z

.github/workflows/ci.yaml

 - name: Setup and execute Gradle 'integrationTestCassandra' task
 uses: gradle/gradle-build-action@v3
 with:
- arguments: integrationTestCassandra
+ arguments: integrationTestCassandra -PcompilationJavaVendor=temurin -PintegrationTestJavaVersion=${{ env.INT_TEST_JAVA_VERSION }} -PintegrationTestJavaVendor=${{ env.INT_TEST_JAVA_VENDOR }}


Always set the compilation vendor to temurin with -PcompilationJavaVendor=temurin to avoid nondeterministic behavior if the integration test uses the JDK 8 with another vendor.

Torch3333 · 2024-05-21T00:43:11Z

buildSrc/src/main/java/com/scalar/db/JdkConfigurationPlugin.java

+ * gradle integrationTestJdbc -PcompilationJavaVendor=amazon -PintegrationTestJavaVersion=11 -PintegrationJavaVendor=temurin
+ * </code></pre>
+ */
+public class JdkConfigurationPlugin implements Plugin<Project> {


This is a Gradle plugin used to configure the compilation and integration test tasks to use a given SDK configured through Gradle properties.

Instead of writing a plugin, I could have added logic directly in build.gradle files but it was easier to develop that way and our build.gradle files are already complicated enough.

Torch3333 · 2024-05-21T00:46:55Z

core/build.gradle

 task integrationTestCassandra(type: Test) {
 description = 'Runs the integration tests for Cassandra.'
 group = 'verification'
 testClassesDirs = sourceSets.integrationTestCassandra.output.classesDirs
 classpath = sourceSets.integrationTestCassandra.runtimeClasspath
 outputs.upToDateWhen { false } // ensures integration tests are run every time when called
 options {
- systemProperties(System.getProperties())
+ systemProperties(System.getProperties().findAll{it.key.toString().startsWith("scalardb")})


When running the integration test with Java 9 but using Java 8 to run IDEA, some Java 8 environment properties were unwillingly added by IDEA that caused the test to crash, so I restricted only to passing properties starting with scalardb

Torch3333 · 2024-05-21T00:49:19Z

gradle/wrapper/gradle-wrapper.properties

@@ -1,5 +1,5 @@
 distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-7.0.2-bin.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-8.7-bin.zip


Upgraded Gradlew to the latest stable version 8.7

Torch3333 · 2024-05-21T00:50:36Z

schema-loader/build.gradle

+task copyFilesToDockerBuildContextDir(type: Copy) {
+ description 'Copy files to a temporary folder to build the Docker image'
 dependsOn shadowJar
- name "ghcr.io/scalar-labs/scalardb-schema-loader:${project.version}"
- files tasks.shadowJar.outputs
+ from("Dockerfile")
+ from(tasks.shadowJar.archiveFile)
+ into('build/docker')
 }

-task dockerfileLint(type: Exec) {
- description 'Lint the Dockerfile'
- commandLine "${project.rootDir}/ci/dockerfile_lint.sh"
+task docker(type: Exec) {
+ description 'Build ScalarDB Schema Loader Docker image'
+ dependsOn copyFilesToDockerBuildContextDir
+ workingDir 'build/docker'
+ commandLine 'docker', 'build', "--tag=ghcr.io/scalar-labs/scalardb-schema-loader:${project.version}", "."


The com.palantir.docker plugin we used to build the Docker image supports the latest Gradle version but requires to use Java >=11 with Gradle, so I removed it.

Since the plugin was just a wrapper for the Docker CLI, I think we can go without it, considering our simple use of building a Docker image from a Dockerfile.

Torch3333 · 2024-05-21T02:01:45Z

server/build.gradle

+task copyFilesToDockerBuildContextDir(type: Copy) {
+ description 'Copy files to a temporary folder to build the Docker image'
+ dependsOn distTar
+ from('Dockerfile')
+ from('conf') {
+ include 'log4j2.properties'
+ include 'database.properties'
+ }
+ from('docker-entrypoint.sh')
+ from(tasks.distTar.archiveFile)
+ into('build/docker')
+}
+
+task docker(type: Exec) {
+ description 'Build ScalarDB Server Docker image'
+ dependsOn copyFilesToDockerBuildContextDir
+ workingDir 'build/docker'
+ commandLine 'docker', 'build', "--tag=ghcr.io/scalar-labs/scalardb-server:${project.version}", "."
+}


Likewise here

komamitsu · 2024-05-21T06:18:18Z

schema-loader/build.gradle

@@ -33,16 +36,24 @@ dependencies {
 javadoc {
 title = "ScalarDB Schema Loader"
 }
+task dockerfileLint(type: Exec) {


[minor] An empty line would be nice?

Thank you, I revised it in 6869be5

komamitsu · 2024-05-21T07:06:19Z

buildSrc/src/main/java/com/scalar/db/JdkConfigurationPlugin.java

+public class JdkConfigurationPlugin implements Plugin<Project> {
+ private static final Logger logger = LoggerFactory.getLogger(JdkConfigurationPlugin.class);
+
+ private static final JavaLanguageVersion COMPILATION_JAVA_VERSION = JavaLanguageVersion.of(8);


This Java version for CompileJava is fixed to 8 while Java vendor can be specified. It might be a bit confusing, considering the case that someone wants to build with Java vender oracle and version 17, but only the Java vender can be passed and Oracle JDK 1.8 is needed in the end.

If we want to always build JavaCompile with temurin JDK 1.8, I guess we can remove compilationJavaVendor property?

Or, adding compilationJavaVersion for consistency while we'll basically use temurin JDK 1.8 for our CI may be an option.

You are right; forcing the same Java version and vendor for compilation and building release makes sense. If so, we can remove compilationJavaVendor

Adding a compilationJavaVersion is a possibility, but in what case do you think it is useful?
Do you want to provide this so that anybody can check out ScalarDB code without requiring Java 8 installation?

Adding a compilationJavaVersion is a possibility, but in what case do you think it is useful?

I have no idea for now. For example, we might want to build our CLI applications with GraalVM to shorten the bootstrap time in the future? I'm not sure, though. I think we can simply remove compilationJavaVendor for now.

All right, thanks for your comment.
I will remove the compilationJavaVendor and require the compilation task to use JDK 8 (temurin).

I revised it in 6869be5

Sorry for the late reply.
Eventually, we will use JDK 11 for compilation, so enabling it to specify a version (and vendor) for compilation might be helpful.
What do you think?

In that case, the following names might be more consistent, but this is a minor comment from my preference.

JavaCompilerVersion

JavaCompilerVendor

integrationTestJavaRuntimeVersion

integrationTestJavaRuntimeVendor

You wish to set JDK 11 (temurin) as the default compiler, which can be overriden via the javaCompilerVersion and the javaCompilerVendor Gradle properties. Is my understanding correct?
I don't see a problem with making the compiler configurable, but as discussed above with Mitsu, we are not sure if there is really a use case for it now. Do you have something in mind?

Thank you for the naming suggestion. The naming is getting longer, but it's more consistent and correct, so I agree with it.

@Torch3333

You wish to set JDK 11 (temurin) as the default compiler, which can be overriden via the javaCompilerVersion and the javaCompilerVendor Gradle properties. Is my understanding correct?

No. Using JDK 8 (temurin) by default is fine, but I would like the compilation version to be configurable so that we don't have to update the code and ci.yaml when we end the support for JDK 8, which might be coming soon (or might not be coming soon).
Of-course, we can update JdkConfigurationPlugin.java to do that, but we want to avoid it if it is simply avoidable.
What do you think?

I see. In that case, to limit changes when we switch the compiler to Java 11, in my opinion, setting the compiler version in the JdkConfigurationPlugin.java without making it configurable with Gradle properties is still the best course of action.
In any scenario, we can't avoid making changes, but not having a configurable compiler only requires slight changes when switching to Java 11.

If we compare the impact of making the JDK configurable or not and then switching to Java 11:

Without a configurable JDK, we need to:

in the CI file, replace all mentions of JDK 8 by JDK 11

update a single variable in the JdkConfigurationPlugin.java

With a configurable JDK, we need to :

in the CI file, replace all mentions of JDK 8 by JDK 11

we will need to add -PjavaCompilerVersion=11 -PjavaCompilerVendor=temurin every time we use gradle, which yields poor usability. We also need to add the same parameters for each gradle job in the CI too, which is a bit verbose.

increase the JdkConfigurationPlugin.java complexity since we need to parse properties to configure the compiler.

@feeblefakie @brfrn169
Per our offline discussion, I will :

In the Gradle build, make the compiler version and vendor configurable with Gradle properties

refactor the CI to use global variables that define the compiler version and vendor to remove the hardcoded usage of Java 8 temurin

brfrn169

LGTM! Thank you!

brfrn169 · 2024-05-23T01:36:05Z

buildSrc/build.gradle

@@ -0,0 +1,26 @@
+plugins {
+ id 'java-gradle-plugin'
+ id "com.diffplug.spotless" version "6.13.0"


Small nit:

Suggested change

id "com.diffplug.spotless" version "6.13.0"

id 'com.diffplug.spotless' version '6.13.0'

Thank you.
I revised it in 6869be5

kota2and3kan

LGTM! Thank you!

…alardb into run_int_test_with_any_jdk

komamitsu

LGTM, thank you!

feeblefakie

Overall, looking good.
Left one comment and suggestion. PTAL!

feeblefakie · 2024-05-27T04:33:34Z

buildSrc/src/main/java/com/scalar/db/JdkConfigurationPlugin.java

+public class JdkConfigurationPlugin implements Plugin<Project> {
+ private static final Logger logger = LoggerFactory.getLogger(JdkConfigurationPlugin.class);
+
+ private static final JavaLanguageVersion COMPILATION_JAVA_VERSION = JavaLanguageVersion.of(8);


Sorry for the late reply.
Eventually, we will use JDK 11 for compilation, so enabling it to specify a version (and vendor) for compilation might be helpful.
What do you think?

In that case, the following names might be more consistent, but this is a minor comment from my preference.

JavaCompilerVersion

JavaCompilerVendor

integrationTestJavaRuntimeVersion

integrationTestJavaRuntimeVendor

komamitsu · 2024-05-30T02:14:36Z

.github/workflows/ci.yaml


 - name: Setup and execute Gradle 'integrationTestJdbc' task
 uses: gradle/gradle-build-action@v3
 with:
- arguments: integrationTestJdbc -Dscalardb.jdbc.url=jdbc:oracle:thin:@//localhost:1521/FREEPDB1 -Dscalardb.jdbc.username=SYSTEM -Dscalardb.jdbc.password=Oracle
+ arguments: integrationTestJdbc -PintegrationTestJavaRuntimeVersion=${{ env.INT_TEST_JAVA_RUNTIME_VERSION }} -PintegrationTestJavaRuntimeVendor=${{ env.INT_TEST_JAVA_RUNTIME_VENDOR }} -Dscalardb.jdbc.url=jdbc:oracle:thin:@//localhost:1521/FREEPDB1 -Dscalardb.jdbc.username=SYSTEM -Dscalardb.jdbc.password=Oracle


JdkConfigurationPlugin can accept javaVersion and javaVendor, but this workflow file doesn't seem to pass the information to the plugin. So, I guess the following confusing situation could occur later.

Decide to build ScalarDB with JDK 11

Revise this workflow to change env.JAVA_VERSION to 11

Indeed, JDK 11 is installed by actions/setup-java

But JdkConfigurationPlugin still tries to use the default JDK 8

To reduce future confusions and efforts, how about explicitly passing env.JAVA_VERSION and env.JAVA_VENDOR to the plugin in this PR?

I agree, thank you.
Revised in 1ed1997

feeblefakie

LGTM! Thank you!

brfrn169

LGTM! Thank you!

…_test_with_any_jdk

komamitsu

LGTM, thank you!

# Conflicts: # .github/workflows/ci.yaml # server/build.gradle

# Conflicts: # .github/workflows/ci.yaml # build.gradle # server/build.gradle

Torch3333 added the improvement label May 20, 2024

Torch3333 self-assigned this May 20, 2024

Torch3333 changed the title ~~Run int test with any jdk~~ Run CI integration test with a given Java version and vendor May 20, 2024

Torch3333 changed the title ~~Run CI integration test with a given Java version and vendor~~ Run CI's integration test with a given Java version and vendor May 20, 2024

Torch3333 changed the title ~~Run CI's integration test with a given Java version and vendor~~ Run Integration test with a given Java version and vendor May 20, 2024

Torch3333 force-pushed the run_int_test_with_any_jdk branch 5 times, most recently from 879575b to d10c9d5 Compare May 21, 2024 00:31

Run integration test with a given JDK

affa33c

Torch3333 force-pushed the run_int_test_with_any_jdk branch from d10c9d5 to affa33c Compare May 21, 2024 01:21

Torch3333 commented May 21, 2024

View reviewed changes

Merge branch 'master' into run_int_test_with_any_jdk

7c0d807

Torch3333 marked this pull request as ready for review May 21, 2024 03:30

Torch3333 changed the title ~~Run Integration test with a given Java version and vendor~~ Run integration test with a given Java version and vendor May 21, 2024

Torch3333 requested review from komamitsu, brfrn169 and feeblefakie May 21, 2024 03:32

komamitsu reviewed May 21, 2024

View reviewed changes

brfrn169 approved these changes May 23, 2024

View reviewed changes

brfrn169 requested a review from kota2and3kan May 23, 2024 01:41

kota2and3kan approved these changes May 23, 2024

View reviewed changes

Torch3333 and others added 3 commits May 23, 2024 14:31

Fix according to comments

6869be5

Merge branch 'run_int_test_with_any_jdk' of github.com:scalar-labs/sc…

c7a46ab

…alardb into run_int_test_with_any_jdk

Merge branch 'master' into run_int_test_with_any_jdk

f2a51aa

Torch3333 requested a review from komamitsu May 27, 2024 01:29

komamitsu approved these changes May 27, 2024

View reviewed changes

feeblefakie reviewed May 27, 2024

View reviewed changes

Torch3333 requested a review from feeblefakie May 27, 2024 06:08

Torch3333 requested a review from komamitsu May 29, 2024 08:34

komamitsu reviewed May 30, 2024

View reviewed changes

feeblefakie approved these changes May 30, 2024

View reviewed changes

brfrn169 approved these changes May 30, 2024

View reviewed changes

Torch3333 added 2 commits May 31, 2024 09:22

Fix according to comments

1ed1997

Merge branch 'master' of github.com:scalar-labs/scalardb into run_int…

44ad4cb

…_test_with_any_jdk

Torch3333 requested a review from komamitsu May 31, 2024 01:25

komamitsu approved these changes May 31, 2024

View reviewed changes

brfrn169 merged commit 339203a into master May 31, 2024
25 checks passed

brfrn169 deleted the run_int_test_with_any_jdk branch May 31, 2024 01:49

Torch3333 added a commit that referenced this pull request May 31, 2024

Run integration test with a given Java version and vendor (#1749)

cc0f686

# Conflicts: # .github/workflows/ci.yaml # server/build.gradle

Torch3333 mentioned this pull request May 31, 2024

Backport to branch(3.12) : Run integration test with a given Java version and vendor #1820

Merged

Torch3333 added a commit that referenced this pull request May 31, 2024

Run integration test with a given Java version and vendor (#1749)

0aafa03

Torch3333 mentioned this pull request May 31, 2024

Backport to branch(3.11) : Run integration test with a given Java version and vendor #1821

Merged

Torch3333 added a commit that referenced this pull request May 31, 2024

Run integration test with a given Java version and vendor (#1749)

e9ffa9e

# Conflicts: # .github/workflows/ci.yaml # server/build.gradle

Torch3333 mentioned this pull request May 31, 2024

Backport to branch(3.10) : Run integration test with a given Java version and vendor #1822

Merged

Torch3333 added a commit that referenced this pull request May 31, 2024

Run integration test with a given Java version and vendor (#1749)

e47554e

# Conflicts: # .github/workflows/ci.yaml # build.gradle # server/build.gradle

Torch3333 added a commit that referenced this pull request May 31, 2024

Run integration test with a given Java version and vendor (#1749)

34cc92c

Torch3333 added a commit that referenced this pull request May 31, 2024

Run integration test with a given Java version and vendor (#1749)

7845f2c

komamitsu mentioned this pull request Jun 7, 2024

Fix a build error of docs/getting-started #1852

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run integration test with a given Java version and vendor #1749

Run integration test with a given Java version and vendor #1749

Torch3333 commented May 20, 2024 •

edited

Torch3333 May 21, 2024

Torch3333 May 21, 2024

Torch3333 May 21, 2024

Torch3333 May 21, 2024 •

edited

Torch3333 May 21, 2024

Torch3333 May 21, 2024

Torch3333 May 21, 2024

Torch3333 May 21, 2024

komamitsu May 21, 2024

Torch3333 May 27, 2024

komamitsu May 21, 2024 •

edited

Torch3333 May 21, 2024

komamitsu May 21, 2024

Torch3333 May 21, 2024

Torch3333 May 27, 2024

feeblefakie May 27, 2024

Torch3333 May 27, 2024

feeblefakie May 28, 2024

Torch3333 May 28, 2024 •

edited

Torch3333 May 29, 2024 •

edited

brfrn169 left a comment

brfrn169 May 23, 2024

Torch3333 May 27, 2024

kota2and3kan left a comment

komamitsu left a comment

feeblefakie left a comment

feeblefakie May 27, 2024

komamitsu May 30, 2024 •

edited

Torch3333 May 31, 2024

feeblefakie left a comment

brfrn169 left a comment

komamitsu left a comment

		@@ -1,19 +1,44 @@
		name: CI
		run-name: "${{ github.event_name == 'workflow_dispatch' && format('Dispatch : Run integration test with JDK {0} ({1})', inputs.int_test_java_version, inputs.int_test_java_vendor) \|\| '' }}"

	id "com.diffplug.spotless" version "6.13.0"
	id 'com.diffplug.spotless' version '6.13.0'

Run integration test with a given Java version and vendor #1749

Run integration test with a given Java version and vendor #1749

Conversation

Torch3333 commented May 20, 2024 • edited

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Torch3333 May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komamitsu May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Torch3333 May 28, 2024 • edited

Choose a reason for hiding this comment

Torch3333 May 29, 2024 • edited

Choose a reason for hiding this comment

brfrn169 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kota2and3kan left a comment

Choose a reason for hiding this comment

komamitsu left a comment

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komamitsu May 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

brfrn169 left a comment

Choose a reason for hiding this comment

komamitsu left a comment

Choose a reason for hiding this comment

Torch3333 commented May 20, 2024 •

edited

Torch3333 May 21, 2024 •

edited

komamitsu May 21, 2024 •

edited

Torch3333 May 28, 2024 •

edited

Torch3333 May 29, 2024 •

edited

komamitsu May 30, 2024 •

edited