You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "cshannon (via GitHub)" <gi...@apache.org> on 2024/07/31 16:31:57 UTC
[PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
cshannon opened a new pull request, #16754:
URL: https://github.com/apache/kafka/pull/16754
This is a draft implementation of [KIP-1032](https://cwiki.apache.org/confluence/display/KAFKA/KIP-1032%3A+Upgrade+to+Jakarta+and+JavaEE+10+in+Kafka+4.0). This updates Kafka to Jakarta specs and JavaEE and Jetty 12. The changes here primarily effect Kafka Connect and MM2.
There are a couple of things that are still outstanding (marked as TODOs in the code) but it's generally close to being finished and the connect and MM2 tests are passing.
**TODO/Notes:**
1. For now I just bumped the entire build to JDK 17 (except the client to 11) to make it easier vs having to go through each module and change which ones require 17. I know the broker is being bumped as well so I figure this can be sorted out soon.
2. There is an issue with one of the MM2 tests that I had to change for now that involves escaping characters and not quite sure what to do about it yet. The cause is the Servlet 6 spec changing what is allowed in the path. See: https://github.com/jetty/jetty.project/issues/11890
3. I had to configure the idle timeout in Jetty requests to match our request timeout so tests didn't fail. This was needed to fix the `ConnectWorkerIntegrationTest#testPollTimeoutExpiry()` test
4. `CrossOriginFilter` has been deprecated and replaced with `CrossOriginHandler` so we may want to switch to that.
Testing is being done by just using the existing tests for Connect and MM2 which should be sufficient.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #16754:
URL: https://github.com/apache/kafka/pull/16754#issuecomment-2501289599
@chia7712 - I merged the latest trunk and fixed the conflicts so this PR is now up to date
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #16754:
URL: https://github.com/apache/kafka/pull/16754#issuecomment-2501174582
@cshannon Could you please rebase code to include #17522?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #16754:
URL: https://github.com/apache/kafka/pull/16754#discussion_r1879000236
##########
gradle/dependencies.gradle:
##########
@@ -162,15 +163,16 @@ libs += [
jacksonModuleScala: "com.fasterxml.jackson.module:jackson-module-scala_$versions.baseScala:$versions.jackson",
jacksonJDK8Datatypes: "com.fasterxml.jackson.datatype:jackson-datatype-jdk8:$versions.jackson",
jacksonBlackbird: "com.fasterxml.jackson.module:jackson-module-blackbird:$versions.jackson",
- jacksonJaxrsJsonProvider: "com.fasterxml.jackson.jaxrs:jackson-jaxrs-json-provider:$versions.jackson",
+ jacksonJakartarsJsonProvider: "com.fasterxml.jackson.jakarta.rs:jackson-jakarta-rs-json-provider:$versions.jackson",
jaxAnnotationApi: "javax.annotation:javax.annotation-api:$versions.jaxAnnotation",
jaxbApi: "javax.xml.bind:jaxb-api:$versions.jaxb",
- jaxrsApi: "javax.ws.rs:javax.ws.rs-api:$versions.jaxrs",
+ jakartaRsApi: "jakarta.ws.rs:jakarta.ws.rs-api:$versions.jakartaRs",
+ jakartaServletApi: "jakarta.servlet:jakarta.servlet-api:$versions.jakartaServlet",
Review Comment:
Yeah it is being pulled in transitively by the ee10 Jetty jars so we can probably remove it because it's tied to that version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #16754:
URL: https://github.com/apache/kafka/pull/16754#discussion_r1872222772
##########
gradle/spotbugs-exclude.xml:
##########
@@ -562,4 +568,10 @@ For a detailed description of spotbugs bug categories, see https://spotbugs.read
<Bug pattern="SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR"/>
</Match>
+ <Match>
+ <!-- DateFormat is not used in more than one thread -->
+ <Class name="org.apache.kafka.tools.ReplicaVerificationTool$ReplicaBuffer"/>
+ <Bug pattern="STCAL_INVOKE_ON_STATIC_DATE_FORMAT_INSTANCE"/>
Review Comment:
I don't remember why I needed to add this because I first worked on this back in July, so I went back to take a look at the file and it looks like I can remove this block entirely because @chia7712 recently added the [same](https://github.com/apache/kafka/blob/0f2a96879030e1ccc575ed10b5cb113efa8200bb/gradle/spotbugs-exclude.xml#L550-L553) exclusion in https://github.com/apache/kafka/pull/17426
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #16754:
URL: https://github.com/apache/kafka/pull/16754#issuecomment-2542505992
This pull request causes a build issue. I have opened [KAFKA-18235](https://issues.apache.org/jira/browse/KAFKA-18235) to address and fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #16754:
URL: https://github.com/apache/kafka/pull/16754#discussion_r1879098262
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/HerderRequestHandler.java:
##########
@@ -113,6 +113,7 @@ public <T, U> T completeOrForwardRequest(FutureCallback<T> cb,
}
String forwardUrl = uriBuilder.build().toString();
log.debug("Forwarding request {} {} {}", forwardUrl, method, body);
+ // TODO, we may need to set the reqest timeout as Idle timeout on the HttpClient
Review Comment:
If we want to configure the idle timeout I guess the question is the best way to do so. I think we'd either need to pass it as an argument to the request() methods or add it to the AbstractConfig for RestClient so it has access to it when constructing the HttpClient. I guess there's also the question if it should always match the request timeout or it could be configured as a separate config. Maybe this could be done as a follow on task after this is merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #16754:
URL: https://github.com/apache/kafka/pull/16754#issuecomment-2533261940
@gharris1727 and @chia7712 - I think I addressed all the comments so this can be merged. The only outstanding [one](https://github.com/apache/kafka/pull/16754#discussion_r1879098262) was if we wanted to set the idle timeout on the HttpClient to match the same thing as the request timeout (or even make it configurable separately) but I am still not quite sure if we should and I think that might be better as a follow on discussion and don't think it's a blocker. I can work on a PR for that depending on what we wanted to do (if anything)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #16754:
URL: https://github.com/apache/kafka/pull/16754#issuecomment-2529909264
> I re-ran the tests three times, looks good. I'm approving this now, but will wait a few days before merging to see if @gharris1727 or @C0urante (or @chia7712) have anything to add.
@mumrah - Sounds good, thanks for the review.
I forgot to mention that I looked more into what to do about having to [change](https://github.com/apache/kafka/pull/16754/files#diff-ab24c09f7ecde101419a599c0ee3625a8f180911149a252792490a5618c7dc7cR283) the `DedicatedMirrorIntegrationTest` to prevent a failure because of the Servlet 6 and Jakarta EE10 spec change that no longer allows ambiguous characters anymore in the URL path.
If we wanted to restore the previous behavior to allow it, we could [set](https://javadoc.jetty.org/jetty-12/org/eclipse/jetty/server/HttpConfiguration.html#setUriCompliance(org.eclipse.jetty.http.UriCompliance)) the UriComplianceMode to be [LEGACY](https://javadoc.jetty.org/jetty-12/org/eclipse/jetty/http/UriCompliance.html#LEGACY). See this comment: https://github.com/jetty/jetty.project/issues/11890#issuecomment-2156449534
However, I would say it's better to not allow it as the spec now says it's a violation so I think the current change I made is fine, but I guess it depends if there's use cases to allow it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #16754:
URL: https://github.com/apache/kafka/pull/16754#issuecomment-2359143095
The checks on Java 8 and Java 11 fail because this PR requires JDK 17+
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on code in PR #16754:
URL: https://github.com/apache/kafka/pull/16754#discussion_r1879053818
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/util/SSLUtilsTest.java:
##########
@@ -33,6 +36,22 @@
public class SSLUtilsTest {
+ private Map<String, Object> sslConfig;
+ private String keystorePath;
+ private String truststorePath;
+ private Password keystorePassword;
+ private Password truststorePassword;
+
+ @BeforeEach
+ public void bofore() throws Exception {
Review Comment:
nit typo: bofore
##########
connect/mirror/src/test/java/org/apache/kafka/connect/mirror/integration/DedicatedMirrorIntegrationTest.java:
##########
@@ -280,7 +280,12 @@ public void testMultiNodeCluster() throws Exception {
// Cluster aliases
final String a = "A";
// Use a convoluted cluster name to ensure URL encoding/decoding works
- final String b = "B- ._~:/?#[]@!$&'()*+;=\"<>%{}|\\^`618";
+ // TODO: Jetty 12 throws a 400 amiguous error with the previous test string here
+ // It seems like at least some encoded characters are not allowed anymore based on
+ // https://github.com/jetty/jetty.project/issues/11890#issuecomment-2156442947
+ // See https://github.com/jetty/jetty.project/issues/11890
+ //final String b = "B- ._~:/?#[]@!$&'()*+;=\"<>%{}|\\^`618";
+ final String b = "B";
Review Comment:
Could you keep most of these suspicious characters, and just remove the ones which are ambiguous?
From reading the linked issue, it might just be the `/` character, and the remainder are still valid.
It looks like this test was sensitive enough to find the underlying change in the library, so I'd like to keep it sensitive in case something like this happens again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #16754:
URL: https://github.com/apache/kafka/pull/16754
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #16754:
URL: https://github.com/apache/kafka/pull/16754#discussion_r1877503804
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/HerderRequestHandler.java:
##########
@@ -113,6 +113,7 @@ public <T, U> T completeOrForwardRequest(FutureCallback<T> cb,
}
String forwardUrl = uriBuilder.build().toString();
log.debug("Forwarding request {} {} {}", forwardUrl, method, body);
+ // TODO, we may need to set the reqest timeout as Idle timeout on the HttpClient
Review Comment:
typo: `reqest` -> `request`
##########
gradle/dependencies.gradle:
##########
@@ -162,15 +163,16 @@ libs += [
jacksonModuleScala: "com.fasterxml.jackson.module:jackson-module-scala_$versions.baseScala:$versions.jackson",
jacksonJDK8Datatypes: "com.fasterxml.jackson.datatype:jackson-datatype-jdk8:$versions.jackson",
jacksonBlackbird: "com.fasterxml.jackson.module:jackson-module-blackbird:$versions.jackson",
- jacksonJaxrsJsonProvider: "com.fasterxml.jackson.jaxrs:jackson-jaxrs-json-provider:$versions.jackson",
+ jacksonJakartarsJsonProvider: "com.fasterxml.jackson.jakarta.rs:jackson-jakarta-rs-json-provider:$versions.jackson",
jaxAnnotationApi: "javax.annotation:javax.annotation-api:$versions.jaxAnnotation",
jaxbApi: "javax.xml.bind:jaxb-api:$versions.jaxb",
- jaxrsApi: "javax.ws.rs:javax.ws.rs-api:$versions.jaxrs",
+ jakartaRsApi: "jakarta.ws.rs:jakarta.ws.rs-api:$versions.jakartaRs",
+ jakartaServletApi: "jakarta.servlet:jakarta.servlet-api:$versions.jakartaServlet",
Review Comment:
`jetty` 12.0.15 is using `jakarta.servlet` 6.0.0, so maybe we can remove this unused declaration?
##########
gradle/dependencies.gradle:
##########
@@ -162,15 +163,16 @@ libs += [
jacksonModuleScala: "com.fasterxml.jackson.module:jackson-module-scala_$versions.baseScala:$versions.jackson",
jacksonJDK8Datatypes: "com.fasterxml.jackson.datatype:jackson-datatype-jdk8:$versions.jackson",
jacksonBlackbird: "com.fasterxml.jackson.module:jackson-module-blackbird:$versions.jackson",
- jacksonJaxrsJsonProvider: "com.fasterxml.jackson.jaxrs:jackson-jaxrs-json-provider:$versions.jackson",
+ jacksonJakartarsJsonProvider: "com.fasterxml.jackson.jakarta.rs:jackson-jakarta-rs-json-provider:$versions.jackson",
jaxAnnotationApi: "javax.annotation:javax.annotation-api:$versions.jaxAnnotation",
jaxbApi: "javax.xml.bind:jaxb-api:$versions.jaxb",
- jaxrsApi: "javax.ws.rs:javax.ws.rs-api:$versions.jaxrs",
+ jakartaRsApi: "jakarta.ws.rs:jakarta.ws.rs-api:$versions.jakartaRs",
Review Comment:
Could you please remove unnecessary item from license file after this change?
https://github.com/apache/kafka/blob/633cab91c1d5ca80763103fa38fb778dcf545709/LICENSE-binary#L305
Also, the following rule seems to be unnecessary.
https://github.com/apache/kafka/blob/trunk/build.gradle#L2900
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/HerderRequestHandler.java:
##########
@@ -113,6 +113,7 @@ public <T, U> T completeOrForwardRequest(FutureCallback<T> cb,
}
String forwardUrl = uriBuilder.build().toString();
log.debug("Forwarding request {} {} {}", forwardUrl, method, body);
+ // TODO, we may need to set the reqest timeout as Idle timeout on the HttpClient
Review Comment:
This comment is valid to me, as the default idle timeout for HttpClient is 30 seconds, and the default request timeout is 90 seconds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #16754:
URL: https://github.com/apache/kafka/pull/16754#issuecomment-2536204667
It seems the JDK 23 build is timing out but I'm not sure why, it seems like the issue could be unrelated to the tests themselves. The build is currently running again (and appears hung but I can't see why until it finishes it says). I pulled the logs from yesterday to figure out why it was hung and it shows:
```
2024-12-11T02:27:43.9906670Z Gradle Test Run :streams:integration-tests:test > Gradle Test Executor 183 > VersionedKeyValueStoreIntegrationTest > shouldManualUpgradeFromNonVersionedTimestampedToVersioned() STARTED
2024-12-11T02:28:31.2902594Z
2024-12-11T02:28:31.2904162Z Gradle Test Run :streams:integration-tests:test > Gradle Test Executor 183 > VersionedKeyValueStoreIntegrationTest > shouldManualUpgradeFromNonVersionedTimestampedToVersioned() PASSED
2024-12-11T02:28:33.2903866Z Copy JUnit XML for integration-tests to /home/runner/work/kafka/kafka/build/junit-xml/streams/integration-tests/test
2024-12-11T03:05:56.8106484Z Timed out after 175 minutes. Dumping threads now...
```
That seems to show it finishes tests and just does nothing for 37 minutes when trying to copy the XML files and then times out
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on code in PR #16754:
URL: https://github.com/apache/kafka/pull/16754#discussion_r1872040064
##########
gradle/spotbugs-exclude.xml:
##########
@@ -562,4 +568,10 @@ For a detailed description of spotbugs bug categories, see https://spotbugs.read
<Bug pattern="SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR"/>
</Match>
+ <Match>
+ <!-- DateFormat is not used in more than one thread -->
+ <Class name="org.apache.kafka.tools.ReplicaVerificationTool$ReplicaBuffer"/>
+ <Bug pattern="STCAL_INVOKE_ON_STATIC_DATE_FORMAT_INSTANCE"/>
Review Comment:
Did any relevant code change in this PR for this spotbugs exclusion?
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java:
##########
@@ -1071,7 +1071,7 @@ private void awaitStopTask(ConnectorTaskId taskId, long timeout) {
}
if (!task.awaitStop(timeout)) {
- log.error("Graceful stop of task {} failed.", task.id());
+ log.error(" Graceful stop of task {} failed.", task.id());
Review Comment:
nit: did you mean to add this space?
##########
gradle/spotbugs-exclude.xml:
##########
@@ -40,6 +40,12 @@ For a detailed description of spotbugs bug categories, see https://spotbugs.read
<Bug pattern="DM_EXIT"/>
</Match>
+ <Match>
+ <!-- Disable warnings about doProvileged, This has been deprecated
+ as of JDK 17 for removal, see https://openjdk.org/jeps/411 -->
+ <Bug pattern="DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED"/>
Review Comment:
This comment confused me at first. Can you include the description of `DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED` to make it clear to the reader?
> Classloaders should only be created inside doPrivileged block
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #16754:
URL: https://github.com/apache/kafka/pull/16754#discussion_r1872212261
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java:
##########
@@ -1071,7 +1071,7 @@ private void awaitStopTask(ConnectorTaskId taskId, long timeout) {
}
if (!task.awaitStop(timeout)) {
- log.error("Graceful stop of task {} failed.", task.id());
+ log.error(" Graceful stop of task {} failed.", task.id());
Review Comment:
Nope, that was definitely inadvertent so I will fix that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on code in PR #16754:
URL: https://github.com/apache/kafka/pull/16754#discussion_r1811421382
##########
build.gradle:
##########
@@ -318,6 +318,18 @@ subprojects {
options.compilerArgs << "-Werror"
options.release = minJavaVersion
addParametersForTests(name, options)
+
+ // Todo: Do we need to separate some components to not require JDK 17? For example,
+ // streams had to be bumped becuase of the dependency on tools.
Review Comment:
Streams should still be built and tested with Java 11. I think this means we're blocked by https://issues.apache.org/jira/browse/KAFKA-17857
Also we are going to probably conflict with https://issues.apache.org/jira/browse/KAFKA-17811, and we can add connect and mirror to that list of modules to build with 17 as the minimum.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #16754:
URL: https://github.com/apache/kafka/pull/16754#discussion_r1811569693
##########
build.gradle:
##########
@@ -318,6 +318,18 @@ subprojects {
options.compilerArgs << "-Werror"
options.release = minJavaVersion
addParametersForTests(name, options)
+
+ // Todo: Do we need to separate some components to not require JDK 17? For example,
+ // streams had to be bumped becuase of the dependency on tools.
Review Comment:
I can just keep this PR open until we are unblocked and periodically update it by merging trunk into the PR to make sure I resolve any conflicts so not a huge deal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #16754:
URL: https://github.com/apache/kafka/pull/16754#issuecomment-2521575540
@mumrah - Thanks for the review, I pushed an update and I believe I addressed all 3 of your comments (as well as merging in the latest from trunk) and that kicked off a new build. I can also run tests a couple more times with an empty commit after the tests finish.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #16754:
URL: https://github.com/apache/kafka/pull/16754#discussion_r1879095660
##########
connect/mirror/src/test/java/org/apache/kafka/connect/mirror/integration/DedicatedMirrorIntegrationTest.java:
##########
@@ -280,7 +280,12 @@ public void testMultiNodeCluster() throws Exception {
// Cluster aliases
final String a = "A";
// Use a convoluted cluster name to ensure URL encoding/decoding works
- final String b = "B- ._~:/?#[]@!$&'()*+;=\"<>%{}|\\^`618";
+ // TODO: Jetty 12 throws a 400 amiguous error with the previous test string here
+ // It seems like at least some encoded characters are not allowed anymore based on
+ // https://github.com/jetty/jetty.project/issues/11890#issuecomment-2156442947
+ // See https://github.com/jetty/jetty.project/issues/11890
+ //final String b = "B- ._~:/?#[]@!$&'()*+;=\"<>%{}|\\^`618";
+ final String b = "B";
Review Comment:
So there's actually a few characters that are not allowed, control characters, backslash, etc. I went ahead and removed a few problem characters so it works now. I also updated the comments for the section. You can see the exact rules in the `Rejecting Suspicious Sequences` section as part of https://jakarta.ee/specifications/servlet/6.0/jakarta-servlet-spec-6.0#uri-path-canonicalization
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #16754:
URL: https://github.com/apache/kafka/pull/16754#issuecomment-2536239754
@cshannon There is a flaky test causing a timeout, and we are going to disable it temporarily. See [#18138](https://github.com/apache/kafka/pull/18138).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16437 - Upgrade to Jakarta and Jetty 12 (KIP-1032) [kafka]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #16754:
URL: https://github.com/apache/kafka/pull/16754#issuecomment-2260926847
@gharris1727 and @C0urante - If either (or both) of you could take a look at some point and see what you think. There's no rush but I wanted to get this PR going now that trunk has been bumped to 4.0.0-SNAPSHOT. I am going to be out of town the rest of the week but I'll be back online middle of next week.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org