-
Notifications
You must be signed in to change notification settings - Fork 146
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
chore(java17): upgrade java runtime to JRE17 and compilation using JDK17, but target remains Java11 #2106
Conversation
97f5ba6
to
55da1d3
Compare
@@ -36,7 +38,7 @@ jobs: | |||
- name: Build | |||
env: | |||
ORG_GRADLE_PROJECT_version: ${{ steps.build_variables.outputs.VERSION }} | |||
run: ./gradlew build --stacktrace ${{ steps.build_variables.outputs.REPO }}-web:installDist | |||
run: ./gradlew -PenableCrossCompilerPlugin=true build --stacktrace ${{ steps.build_variables.outputs.REPO }}-web:installDist |
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 need separate java11 + java 17 dockerfiles + build steps too, 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.
Added the required steps and docker files.
Note: this is similar to both spinnaker/echo#1342 and spinnaker/echo#1426. |
@@ -25,7 +25,9 @@ jobs: | |||
uses: docker/setup-buildx-action@v3 | |||
- uses: actions/setup-java@v4 | |||
with: | |||
java-version: 11 | |||
java-version: | | |||
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.
Think it's time to remove 11....
…K17, but target remains Java11 While upgrading java 17, encountered below error during the test execution process of keel-core-test and keel-sql modules: ``` retrieves timestamp successfully SqlArtifactRepositoryTests retrieves timestamp successfully FAILED (11.5s) ▼ Expect that 2024-11-04T16:23:59.680067Z: ✗ is equal to 2024-11-04T16:23:59.680066531Z found 2024-11-04T16:23:59.680067Z ``` ``` SqlDeliveryConfigRepositoryTests deletes data successfully for known application FAILED (11.5s) org.jooq.exception.DataAccessException: SQL [insert into `keel`.`event` (`uid`, `scope`, `json`) values (?, ?, ?)]; Incorrect datetime value: '2024-11-04T16:47:17.740578522Z' for function str_to_date Caused by: java.sql.SQLException: Incorrect datetime value: '2024-11-04T16:47:17.740578522Z' for function str_to_date SqlDeliveryConfigRepositoryTests throws exception for unknown application FAILED (12.8s) org.jooq.exception.DataAccessException: SQL [insert into `keel`.`event` (`uid`, `scope`, `json`) values (?, ?, ?)]; Incorrect datetime value: '2024-11-04T16:47:29.434469839Z' for function str_to_date Caused by: java.sql.SQLException: Incorrect datetime value: '2024-11-04T16:47:29.434469839Z' for function str_to_date SqlDeliveryConfigRepositoryTests deletes related application events FAILED (12s) org.jooq.exception.DataAccessException: SQL [insert into `keel`.`event` (`uid`, `scope`, `json`) values (?, ?, ?)]; Incorrect datetime value: '2024-11-04T16:47:42.199694193Z' for function str_to_date Caused by: java.sql.SQLException: Incorrect datetime value: '2024-11-04T16:47:42.199694193Z' for function str_to_date SqlDeliveryConfigRepositoryTests deletes related resource events FAILED (12.6s) org.jooq.exception.DataAccessException: SQL [insert into `keel`.`event` (`uid`, `scope`, `json`) values (?, ?, ?)]; Incorrect datetime value: '2024-11-04T16:47:54.338147807Z' for function str_to_date Caused by: java.sql.SQLException: Incorrect datetime value: '2024-11-04T16:47:54.338147807Z' for function str_to_date SqlDeliveryConfigRepositoryTests deletes related application pause records FAILED (11.5s) org.jooq.exception.DataAccessException: SQL [insert into `keel`.`event` (`uid`, `scope`, `json`) values (?, ?, ?)]; Incorrect datetime value: '2024-11-04T16:48:06.896010045Z' for function str_to_date Caused by: java.sql.SQLException: Incorrect datetime value: '2024-11-04T16:48:06.896010045Z' for function str_to_date SqlDeliveryConfigRepositoryTests deletes related resource pause records FAILED (12.6s) org.jooq.exception.DataAccessException: SQL [insert into `keel`.`event` (`uid`, `scope`, `json`) values (?, ?, ?)]; Incorrect datetime value: '2024-11-04T16:48:18.409434437Z' for function str_to_date Caused by: java.sql.SQLException: Incorrect datetime value: '2024-11-04T16:48:18.409434437Z' for function str_to_date SqlDeliveryConfigRepositoryTests deletes any older versions of resources FAILED (11.8s) org.jooq.exception.DataAccessException: SQL [insert into `keel`.`event` (`uid`, `scope`, `json`) values (?, ?, ?)]; Incorrect datetime value: '2024-11-04T16:48:31.166252032Z' for function str_to_date Caused by: java.sql.SQLException: Incorrect datetime value: '2024-11-04T16:48:31.166252032Z' for function str_to_date SqlDeliveryConfigRepositoryTests deletes data successfully for known delivery config FAILED (11.7s) org.jooq.exception.DataAccessException: SQL [insert into `keel`.`event` (`uid`, `scope`, `json`) values (?, ?, ?)]; Incorrect datetime value: '2024-11-04T16:48:42.855107042Z' for function str_to_date Caused by: java.sql.SQLException: Incorrect datetime value: '2024-11-04T16:48:42.855107042Z' for function str_to_date SqlDeliveryConfigRepositoryTests throws exception for unknown delivery config FAILED (12.1s) org.jooq.exception.DataAccessException: SQL [insert into `keel`.`event` (`uid`, `scope`, `json`) values (?, ?, ?)]; Incorrect datetime value: '2024-11-04T16:48:54.920960756Z' for function str_to_date Caused by: java.sql.SQLException: Incorrect datetime value: '2024-11-04T16:48:54.920960756Z' for function str_to_date ``` The error occurred due to change in behaviour of `java.time.Instant` class from Java 11 to Java 17. In Java 17, Instant class returns clock with precision of microsecond and nanosecond based on underlying system, which is not the case with Java 11. This causes the MYSQL str_to_date function fail while data insertion. In order to fix this issue, truncated the microseconds from the Instant. https://stackoverflow.com/questions/74781495/changes-in-instant-now-between-java-11-and-java-17-in-aws-ubuntu-standard-6-0
Working on an alternative that'll HOPEFULLY help in a more generic manner. |
SO main concern with the trunctation is it doesn't 100% solve the format conversion issue. As there's another edge case around 0's & the way that happens. See
The root of the problem is the auto generated columns that PARSE this via a str_to_date formatting and REQUIRE formatting of a certain precision/text, and instant.toString() which is not consistent on it's output (SHOULDN'T be using Instant for this). Truncating the dates... MAY work but there's a lot of potential places this misses or may not take into effect. https://github.com/spinnaker/keel/pull/2107/files#diff-c663beecab3faa9b8803c228774a2a51d4b1da4ea0195a97114712df74ca899bR16 This should take care of this by changing the serialization to truncate the stuff when it encounters an instant. Either way, bad design to do conversion using STRINGS instead of LONG for this kinda thing. That's a ... JSON issue but not sure I want to fix it JUST yet. |
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.
Overall lgtm, but had some comments around docker image standards and a minor suggestion about instant
@@ -0,0 +1,9 @@ | |||
FROM alpine:3.11 | |||
LABEL maintainer="[email protected]" | |||
RUN apk --no-cache add --update bash openjdk11-jre |
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 avoid multiple RUN commands. This explodes the image size in docker.
Instead
RUN addgroup -S -g 10111 spinnaker && \
adduser -S -G spinnaker -u 10111 spinnaker \
...
@@ -0,0 +1,8 @@ | |||
FROM ubuntu:bionic | |||
LABEL maintainer="[email protected]" | |||
RUN apt-get update && apt-get -y install openjdk-11-jre-headless wget |
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.
Same here
LABEL maintainer="[email protected]" | ||
RUN apk --no-cache add --update bash openjdk11-jre | ||
RUN apk --no-cache add --update bash openjdk17-jre |
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.
Turns out all of our images need to do this
@@ -101,7 +102,7 @@ data class ResourceCreated( | |||
resource.id, | |||
resource.version, | |||
resource.application, | |||
clock.instant() | |||
clock.instant().truncatedTo(ChronoUnit.MICROS) |
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.
While I think this is fine, we may want to have our instant source in kork in the event someone forgets to truncate
superceded by #2107 |
While upgrading java 17, encountered below error during the test execution process of keel-core-test and keel-sql modules:
The error occurred due to change in behaviour of
java.time.Instant
class from Java 11 to Java 17. In Java 17, Instant class returns clock with precision of microsecond and nanosecond based on underlying system, which is not the case with Java 11. This causes the MYSQL str_to_date function fail while data insertion. In order to fix this issue, truncated the microseconds from the Instant.https://stackoverflow.com/questions/74781495/changes-in-instant-now-between-java-11-and-java-17-in-aws-ubuntu-standard-6-0