Skip to content
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(java): Full java 17 only support #2107

Merged
merged 15 commits into from
Nov 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ jobs:
uses: docker/setup-buildx-action@v3
- uses: actions/setup-java@v4
with:
java-version: 11
java-version: 17
distribution: 'zulu'
cache: 'gradle'
- name: Prepare build variables
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/pr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ jobs:
uses: docker/setup-buildx-action@v3
- uses: actions/setup-java@v4
with:
java-version: 11
java-version: 17
distribution: 'zulu'
cache: 'gradle'
- name: Prepare build variables
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ jobs:
uses: docker/setup-buildx-action@v3
- uses: actions/setup-java@v4
with:
java-version: 11
java-version: 17
distribution: 'zulu'
cache: 'gradle'
- name: Assemble release info
Expand Down
4 changes: 2 additions & 2 deletions Dockerfile.compile
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
FROM ubuntu:bionic
FROM ubuntu:jammy
LABEL maintainer="sig-platform@spinnaker.io"
RUN echo "mysql-server mysql-server/root_password password sa" | debconf-set-selections
RUN echo "mysql-server mysql-server/root_password_again password sa" | debconf-set-selections
RUN apt-get update && apt-get install -y \
openjdk-11-jdk \
openjdk-17-jdk \
mysql-server \
&& rm -rf /var/lib/apt/lists/*
ENV GRADLE_USER_HOME /workspace/.gradle
Expand Down
4 changes: 2 additions & 2 deletions Dockerfile.slim
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
FROM alpine:3.11
FROM alpine:3.20
LABEL maintainer="sig-platform@spinnaker.io"
RUN apk --no-cache add --update bash openjdk11-jre
RUN apk --no-cache add --update bash openjdk17-jre
RUN addgroup -S -g 10111 spinnaker
RUN adduser -S -G spinnaker -u 10111 spinnaker
COPY keel-web/build/install/keel /opt/keel
Expand Down
4 changes: 2 additions & 2 deletions Dockerfile.ubuntu
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
FROM ubuntu:bionic
FROM ubuntu:jammy
LABEL maintainer="sig-platform@spinnaker.io"
RUN apt-get update && apt-get -y install openjdk-11-jre-headless wget
RUN apt-get update && apt-get -y install openjdk-17-jre-headless wget
RUN adduser --system --uid 10111 --group spinnaker
COPY keel-web/build/install/keel /opt/keel
RUN mkdir -p /opt/keel/plugins && chown -R spinnaker:nogroup /opt/keel/plugins
Expand Down
4 changes: 0 additions & 4 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,6 @@ allprojects {
apply(plugin: "com.github.ben-manes.versions")
apply(plugin: "com.diffplug.spotless")

java {
sourceCompatibility = JavaVersion.VERSION_11
targetCompatibility = JavaVersion.VERSION_11
}

dependencies {
annotationProcessor(platform("io.spinnaker.kork:kork-bom:${korkVersion}"))
Expand Down
3 changes: 3 additions & 0 deletions gradle.properties
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
targetJava17=true
kotlinVersion=1.6.21
fiatVersion=1.50.0
korkVersion=7.243.0
Expand All @@ -9,6 +10,8 @@ okHttpVersion=4.5.0
resilience4jVersion=1.5.0
spinnakerGradleVersion=8.32.1

org.gradle.jvmargs=-Xmx4g -Xms4g

# Used to control whether to spin up docker to run liquibase before jooq
buildingInDocker=false

Expand Down
4 changes: 2 additions & 2 deletions gradle/kotlin.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ apply plugin: "kotlin-allopen"
compileKotlin {
kotlinOptions {
languageVersion = "1.6"
jvmTarget = "11"
jvmTarget = "17"
// see https://kotlinlang.org/docs/java-to-kotlin-interop.html#compatibility-mode-for-default-methods
freeCompilerArgs += "-Xjvm-default=enable"
}
Expand All @@ -14,7 +14,7 @@ compileKotlin {
compileTestKotlin {
kotlinOptions {
languageVersion = "1.6"
jvmTarget = "11"
jvmTarget = "17"
freeCompilerArgs += "-Xjvm-default=enable"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import strikt.assertions.isNull
import strikt.assertions.isTrue
import java.time.Clock
import java.time.Instant
import java.time.temporal.ChronoUnit

abstract class ArtifactRepositoryTests<T : ArtifactRepository> : JUnit5Minutests {
val publisher: ApplicationEventPublisher = mockk(relaxed = true)
Expand Down Expand Up @@ -649,7 +650,10 @@ abstract class ArtifactRepositoryTests<T : ArtifactRepository> : JUnit5Minutests
}

context("artifact creation timestamp exists") {
val createdAt = Instant.now()
// We truncate this since we're using a serialization to java that reduces the level of precision
// and later comparisons break otherwise. This is needed to work with generated columns in
// certain databases. See the PrecisionSqlSerializer class for more info
val createdAt = Instant.now().truncatedTo(ChronoUnit.MICROS)

before {
subject.register(versionedSnapshotDebian)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package com.netflix.spinnaker.keel.serialization;

import com.fasterxml.jackson.annotation.JsonFormat;
import com.fasterxml.jackson.datatype.jsr310.ser.InstantSerializerBase;
import java.time.Instant;
import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeFormatterBuilder;

/**
* This class overloads the default Serialization instance in the JavaTimeModule to a precision
* level that the database can process via str_to_date string conversion It's needed because we do
* JSON serialization to a column and then the column gets used to generate another field via string
* conversion. Specifically * triggered_at * dismissed_at are "generated" columns. NOTE this should
* ONLY impact running on Linux as the system clock on Linux systems returns a higher precision in
* Java 17+. It's also impactful as ISO_INSTANT has NO precision limitation giving inconsistent
* string output as a result
*
* <p>See
*
* <ul>
* <li>https://stackoverflow.com/questions/74781495/changes-in-instant-now-between-java-11-and-java-17-in-aws-ubuntu-standard-6-0
* <li>https://stackoverflow.com/a/38042457 for more information.
* </ul>
*
* <p>NOTE we're going to 6 digits to match <a
* href="https://www.w3schools.com/sql/func_mysql_str_to_date.asp">Mysql str_to_date function
* limits</a> on microseconds parsing. This is used in the SQL definition: <code>
* add column triggered_at datetime(3) generated always as (str_to_date(json->>'$.triggeredAt', '%Y-%m-%dT%T.%fZ'))
* </code> column in the 20210616-create-dismissible-notifications.yml liquibase change set.
*/
public class PrecisionSqlSerializer extends InstantSerializerBase<Instant> {
jasonmcintosh marked this conversation as resolved.
Show resolved Hide resolved
public PrecisionSqlSerializer() {
super(
Instant.class,
Instant::toEpochMilli,
Instant::getEpochSecond,
Instant::getNano,
new DateTimeFormatterBuilder().appendInstant(6).toFormatter());
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment above about using 3 digits could use a tweak.

Copy link
Member Author

Choose a reason for hiding this comment

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

Was waiting for tests to pass ;) LOOKS Like they're happy with the 6 precision, though datetime(3) MAY truncate it more. The question there to my mind is... do we want this to go to 3 or 6?

Copy link
Contributor

Choose a reason for hiding this comment

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

What did we have with java 11?

Copy link

@xibz xibz Nov 15, 2024

Choose a reason for hiding this comment

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

Was the point for this? To just allow for more precision? If that's the case, we may want to just leave it at 3. Like I said that was the only thing I was unsure of, but more precision also does not hurt, but Spinnaker also doesn't need ot be the most precise service either

Copy link
Member Author

Choose a reason for hiding this comment

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

Java 11 looks like only supported 6 digits of precision. It wasn't until 17 and it seems VERY tied to the OS on what level it supports. In this particular case what's MORE important is that databases don't support more than 6 (that we support)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm getting tripped up in the wording, but isn't there a difference between what java 11 supports, and what keel actually used? I'd prefer to keep it the same as what we had if possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

SO not quite. Java 11 supported and delivered 6 digits of precision at MOST. Java 17 ALLOWED a higher level of precision if the system clock supported it. Instant.toString was never consistent on the output - it would just output the MOST available. This led to situations where when doing serialization to JSON, you'd get inconsistent results. And on an OS with HIGHER precision, you'd go over the bounds that the database supported. SO the primary impact and root of the problem is when Keel does auto generated fields OFF of the JSON string in date format, causing a failure. Java was always able to handle it, but databases didn't handle the higher precision UNLESS they were storing things as a string (which most are). Timestamp fields however would NOT work.

SO the key on this is to serialize at MAX precision for the storage layer. NOTE: There are POTENTIAL headaches on tests in other places if we do comparisons of what's written & read on unit tests in a real database. SQLLite being Java MAY support a higher preceision - but I'd bet it depends on compatibility mode. SO if we have a test that writes the timestamps using a mysql test container, then tries to do an equals on a read of that timestamp it will NOT work as mysql wouldn't support the full precision that the JVM supports.

Copy link
Member Author

Choose a reason for hiding this comment

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

To note... Orca uses LONG's for persistence of timestamps - so it worked without issue. It's JUST keel that stores times as Instants and in string format. Least so far that I found. That was the original headache with a simple replacement serialization implementation - I had to switch to the time base serializer which is smarter on it's handling.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, sounds like we're going with 6. I guess we don't include keel in our release notes, so we're off the hook there :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I should correct H2 was what was thinking on memory. SQLlite looks like it supports 6 - but there are extensions to add more precision (again just a quick search on this one)

}

@Override
protected InstantSerializerBase<?> withFormat(
Boolean aBoolean, DateTimeFormatter dateTimeFormatter, JsonFormat.Shape shape) {
return this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule
import com.fasterxml.jackson.module.kotlin.registerKotlinModule
import com.netflix.spinnaker.keel.jackson.registerKeelApiModule
import de.huxhorn.sulky.ulid.ULID
import org.springframework.boot.jackson.JsonComponentModule
import java.text.SimpleDateFormat
import java.time.Instant
import java.util.TimeZone

/**
Expand All @@ -29,18 +29,20 @@ fun configuredObjectMapper(): ObjectMapper = ObjectMapper().configureForKeel()
* Factory method for [YAMLMapper]s configured how we like 'em.
*/
fun configuredYamlMapper(): YAMLMapper = YAMLMapper().configureForKeel().disable(USE_NATIVE_TYPE_ID)

fun <T : ObjectMapper> T.configureForKeel(): T =
apply {
fun <T : ObjectMapper> T.configureForKeel(): T {
val javaTimeModule = JavaTimeModule()
javaTimeModule.addSerializer(Instant::class.java,PrecisionSqlSerializer())
Copy link

Choose a reason for hiding this comment

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

Can we add a round trip test that uses the serializer and then deserializes to ensure precision is in fact kept? Other than that, this PR looks good

Copy link
Member Author

Choose a reason for hiding this comment

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

It's actually tested indirectly via the ArtifactRepositoryTests file but let me see about a direct test. It'll only keep as much precision as it writes in the JSON fields. So this WILL reduce precision on these columns from what the JVM could track - but it's never had that until java 17 it looks like so we're not losing anything - though I may try to do a "6" instead of "3" precision as docs say MySQL CAN handle that level.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a test see what you think :)

return apply {
registerKeelApiModule()
.registerKotlinModule()
.registerULIDModule()
.registerModule(JavaTimeModule())
.registerModule(javaTimeModule)
.configureSaneDateTimeRepresentation()
.disable(FAIL_ON_UNKNOWN_PROPERTIES)
.enable(ACCEPT_CASE_INSENSITIVE_ENUMS)
.setSerializationInclusion(NON_NULL)
}
}

private fun ObjectMapper.registerULIDModule(): ObjectMapper =
registerModule(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package com.netflix.spinnaker.keel.serialization

import dev.minutest.junit.JUnit5Minutests
import dev.minutest.rootContext
import org.assertj.core.api.Assertions.assertThat
import java.time.Instant
import java.time.LocalDateTime
import java.time.OffsetDateTime
import java.time.ZoneOffset
import java.time.format.DateTimeFormatter
import java.time.format.DateTimeFormatterBuilder

class ObjectMapperSerializationTests : JUnit5Minutests {

class ObjectMapperSerializationTestsObject {
var date = Instant.from(DateTimeFormatter.ISO_INSTANT.parse("2024-03-10T12:28:19.228816801Z"))
}

fun tests() = rootContext {
test("Verify instant precision is no more than 6 characters") {
val mapper = configuredObjectMapper()
val sampleObject = ObjectMapperSerializationTestsObject()
val readBack = mapper.readValue(
mapper.writeValueAsString(sampleObject),
ObjectMapperSerializationTestsObject::class.java
)
// 6 precision is the max allowed via str_to_date in SQL. See
// https://dev.mysql.com/doc/refman/8.4/en/date-and-time-type-syntax.html#:~:text=MySQL%20permits%20fractional%20seconds%20for,microseconds%20(6%20digits)%20precision.
// SO we want to make sure waht we store matches this
val foramt = DateTimeFormatterBuilder().parseCaseInsensitive().appendInstant(6).parseStrict().toFormatter()
assertThat(readBack.date).isEqualTo(foramt.format(sampleObject.date))
}
}
}
2 changes: 1 addition & 1 deletion keel-sql/src/test/resources/testcontainers.properties
Original file line number Diff line number Diff line change
@@ -1 +1 @@
ryuk.container.image = public.ecr.aws/s4w6t4b6/testcontainers/ryuk:0.3.4
ryuk.container.image = testcontainers/ryuk:0.11.0
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI the ECR image wasn't working in local testing - thus the need to switch. PROBABLY should use docker.io/testcontainers/ryuk:0.11.0 but this works