Hi Chris, Thanks for the updates and the additional context. I have one last nit after doing another readthrough: in the public interfaces section it states that "There shouldn't be changes to the main client API as the API doesn't use jakarta". This is almost true, but like Greg has mentioned previously, there is the REST extension API: see the ConnectRestExtensionContext interface [1] in the connect-api module. I think we should call out that the signature of ConnectRestExtensionContext::configurable will change to return an instance of jakarta.ws.rs.core.Configurable instead of javax.ws.rs.core.Configurable.
This doesn't block my approval of the KIP since I think that the above is an acceptable change to make, but IMO it should be added to give a better picture of the changes to anyone else who views the KIP. [1] - https://github.com/apache/kafka/blob/43676f7612b2155ecada54c61b129d996f58bae2/connect/api/src/main/java/org/apache/kafka/connect/rest/ConnectRestExtensionContext.java#L22 Cheers, Other Chris On Tue, Jul 9, 2024 at 3:40 PM Christopher Shannon < christopher.l.shan...@gmail.com> wrote: > Hi Chris, > > I can make those changes. For number 2 that was a holdover back when we > were discussing 11 vs 12 and whether or not JDK 17 would be used, but we > are going with 12 since 11 is deprecated so I will fix that. > > > On Tue, Jul 9, 2024 at 2:54 PM Chris Egerton <chr...@aiven.io.invalid> > wrote: > > > Hi Chris, > > > > Thanks for the KIP! It's a bit of a drastic change to rip the bandaid off > > like this and require users running Connect to upgrade to JDK 17, but I > > think it's the best out of the options that are available to us. > > > > Two small nits on the KIP: > > 1. It'd be nice to link to KIP-1013 in the motivation section to > establish > > that there is already some precedent for bumping to JDK 17+ for > server-side > > components (i.e., Kafka brokers) in 4.0. > > 2. In the compatibility section it's stated that "JDK 17+ will be > required > > if Jetty 12.x is chosen to be used". This is a pretty big "if". My > > understanding is that we will definitely be using Jetty 12.x; can we > remove > > the "if" or is it still up for debate whether we'll do that switch for > > Connect? > > > > Cheers, > > > > Other Chris > > > > On Tue, Jul 9, 2024 at 10:49 AM Christopher Shannon < > > christopher.l.shan...@gmail.com> wrote: > > > > > Hi Greg, > > > > > > I will make a couple quick tweaks and open a vote. I think we should > > target > > > JDK 17, JavaEE 10 and Jetty 12 because Jetty 11 is now EOL. > > > > > > Chris > > > > > > On Mon, Jul 8, 2024 at 2:13 PM Greg Harris > <greg.har...@aiven.io.invalid > > > > > > wrote: > > > > > > > Hi Chris, > > > > > > > > Please open a vote thread for this. > > > > > > > > Thanks, > > > > Greg > > > > > > > > On Tue, May 14, 2024 at 9:07 AM Christopher Shannon < > > > > christopher.l.shan...@gmail.com> wrote: > > > > > > > > > I just wanted to bump this and see if anyone had more feedback > before > > > > > trying to call a vote for this for 4.0? > > > > > > > > > > Chris > > > > > > > > > > On Mon, Apr 1, 2024 at 3:41 PM Christopher Shannon < > > > > > christopher.l.shan...@gmail.com> wrote: > > > > > > > > > > > Greg, > > > > > > > > > > > > 1. Ok sounds good we can target JDK 17 in this KIP if we decide > to > > do > > > > > that. > > > > > > 2. For the EE version, I don't think it really matters since we > > won't > > > > be > > > > > > using any new features that I am aware of. It's just something I > > > > noticed > > > > > > that we will need to pick because Jetty 12 supports multiple > > versions > > > > so > > > > > it > > > > > > would affect which spec jars we use. In the past Jetty versions > > have > > > > > been > > > > > > tied to a specific Servlet spec but the new Jetty 12 they have > > > > abstracted > > > > > > things away and they support multiple versions simultaneously. > > > There's > > > > > > different versions for all the specs but the primary one to note > > for > > > us > > > > > > would be that JavaEE 9 uses the Servlet 5.0 spec and JavaEE 10 > uses > > > the > > > > > > Servlet 6.0 spec. JavaEE 11 is under development and will use the > > > > Servlet > > > > > > 6.1 spec. So we may not really need to call out the EE version at > > all > > > > if > > > > > it > > > > > > doesn't matter and we are not using specific features but I > wanted > > to > > > > > bring > > > > > > it up since multiple versions are listed as being compatible with > > > Jetty > > > > > 12 > > > > > > so we need to pick one. On the main page they list the different > > > > servlet > > > > > > specs they support: https://eclipse.dev/jetty/ > > > > > > 3. Right, I didn't mean we should include it in the KIP, I was > more > > > > > asking > > > > > > I guess how to go about things. It looks like we could use a lot > of > > > it > > > > > and > > > > > > adapt the work already done. While it's under the Apache 2.0 > > license > > > > and > > > > > we > > > > > > could use it, someone else wrote it so it would still be good to > > > > properly > > > > > > credit that person as you mentioned. If I work on it I would > > probably > > > > > start > > > > > > over with a new branch and just use the old PR as a guide and > then > > > > maybe > > > > > > figure out a way to credit the original author. There's always > that > > > > > > co-author tag that could be used I think. > > > > > > > > > > > > Chris > > > > > > > > > > > > > > > > > > On Mon, Apr 1, 2024 at 12:11 PM Greg Harris > > > > <greg.har...@aiven.io.invalid > > > > > > > > > > > > wrote: > > > > > > > > > > > >> Hey Chris, > > > > > >> > > > > > >> Thanks for your questions! > > > > > >> > > > > > >> 1. KIPs are generally immutable once they've been voted on. In > > this > > > > > >> case, I think it's better that this KIP include the bump to Java > > 17 > > > > > >> just for Connect and MirrorMaker 2, and should include that in > the > > > KIP > > > > > >> title. > > > > > >> 2. I'm not familiar with the difference, can you provide some > more > > > > > >> context that would help us make a decision? AFAIU we haven't > > > specified > > > > > >> an EE version in the past, and we don't do any sort of automated > > > > > >> testing for compatibility. I think it would be good to call out > > > which > > > > > >> components have JavaEE-sensitive dependencies (just > > > connect-runtime?). > > > > > >> We do not want to accidentally give users the idea that the > > clients > > > > > >> depend on the JavaEE version, as that could be very confusing. > > > > > >> 3. That's an implementation detail left up to anyone that > effects > > > this > > > > > >> KIP on the repo, and doesn't need to be mentioned in the KIP > > > itself. I > > > > > >> have seen people adopt changes from un-merged PRs after the > > original > > > > > >> contributor has lost interest, while still crediting the > original > > > > > >> contributor for their portion of the changes. If you're doing > > this, > > > > > >> then it's ultimately up to your judgement. > > > > > >> > > > > > >> Thanks, > > > > > >> Greg > > > > > >> > > > > > >> On Mon, Apr 1, 2024 at 6:30 AM Christopher Shannon > > > > > >> <christopher.l.shan...@gmail.com> wrote: > > > > > >> > > > > > > >> > Hi Greg, > > > > > >> > > > > > > >> > Thanks for the detailed analysis on the connect framework. It > > > sounds > > > > > >> like > > > > > >> > hopefully we can go ahead and require JDK 17+ and bump that > > > > dependency > > > > > >> for > > > > > >> > the ConnectRestExtensionContext. > > > > > >> > > > > > > >> > I agree we can leave it open and hear what others think as > well > > > > about > > > > > >> the > > > > > >> > requirement, if everyone ends up agreeing, I can update the > KIP > > to > > > > > >> reflect > > > > > >> > requiring JDK 17 and going with Jetty 12. > > > > > >> > > > > > > >> > I had a couple of questions > > > > > >> > 1) If we go with JDK 17 as a requirement for the Connect > > > framework, > > > > > >> should > > > > > >> > we modify and make that part of KIP-1013 or keep it in this > one? > > > > > >> > 2) Should we go with JavaEE 9 or JavaEE 10? I'm not sure how > > much > > > it > > > > > >> > matters in this case. > > > > > >> > 3) Can we just re-open > > https://github.com/apache/kafka/pull/10176 > > > > as > > > > > a > > > > > >> > starting point or maybe we can create a new PR and use it as a > > > > basis? > > > > > >> It's > > > > > >> > a bit old so I suspect there would be a ton of conflicts so it > > > might > > > > > be > > > > > >> > best to start over and use it as a guide. I can work on a new > PR > > > > once > > > > > we > > > > > >> > are all on the same page. I don't think it would take too long > > to > > > > put > > > > > >> > together since most of the work is just dependency updates and > > > > package > > > > > >> > renaming. > > > > > >> > > > > > > >> > Chris > > > > > >> > > > > > > >> > > > > > > >> > On Fri, Mar 29, 2024 at 8:59 PM Greg Harris > > > > > >> <greg.har...@aiven.io.invalid> > > > > > >> > wrote: > > > > > >> > > > > > > >> > > Hey all, > > > > > >> > > > > > > > >> > > I looked into how Debezium handled the javax->jakarta > > changeover > > > > for > > > > > >> > > Quarkus, and found this release note: > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > https://debezium.io/blog/2023/04/20/debezium-2-2-final-released/#new-quarkus-3 > > > > > >> > > It appears that Debezium 2.1 required Quarkus < 3.0, and > > > Debezium > > > > > 2.2 > > > > > >> > > required Quarkus >= 3.0. The upgrade for Kafka could be very > > > > similar > > > > > >> > > and not incur a major version release. > > > > > >> > > > > > > > >> > > We can leave some time to hear from anyone else that is > > > impacted > > > > by > > > > > >> > > this change, but from the open source projects present on > > > github, > > > > > this > > > > > >> > > LGTM. > > > > > >> > > > > > > > >> > > Thanks, > > > > > >> > > Greg > > > > > >> > > > > > > > >> > > On Fri, Mar 29, 2024 at 5:27 PM Greg Harris < > > > greg.har...@aiven.io > > > > > > > > > > >> wrote: > > > > > >> > > > > > > > > >> > > > Hi Chris, > > > > > >> > > > > > > > > >> > > > Thank you so much for opening this KIP, and making sure > > Kafka > > > > > keeps > > > > > >> up > > > > > >> > > > with the rest of the Java ecosystem! > > > > > >> > > > > > > > > >> > > > I took a look around at some Open Source connector > > > > > implementations, > > > > > >> > > > and checked their Java version support: > > > > > >> > > > * The Aiven connect plugins (http, bigquery, jdbc, > > > > elasticsearch, > > > > > >> > > > opensearch, commons, s3, transforms, gcs), 6/9 are tested > > with > > > > JDK > > > > > >> 17 > > > > > >> > > > in CI, 2/9 JDK 11, and 1/9 JDK 8. I'll look into improving > > the > > > > > >> testing > > > > > >> > > > matrix, but I don't expect substantial problems with > > requiring > > > > JDK > > > > > >> 17. > > > > > >> > > > * The Debezium Project lists Java 11+ compatibility: > > > > > >> > > > https://debezium.io/releases/ and appears to use Java 22 > > (ga) > > > > and > > > > > >> 23 > > > > > >> > > > (ea) in their CI: > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > https://github.com/debezium/debezium/blob/9cdaa38453c9f065c6075d31636592a5b147518f/.github/workflows/jdk-outreach-workflow.yml#L20 > > > > > >> > > > > > > > > >> > > > I think the bigger problem really is the > > ConnectRestExtension, > > > > > since > > > > > >> > > > we've baked the rs-api type into the signature of > > > > > >> > > > ConnectRestExtensionContext. > > > > > >> > > > * Aiven doesn't have any ConnectRestExtensions, so this > > isn't > > > a > > > > > >> concern > > > > > >> > > for us. > > > > > >> > > > * The Debezium Project has at least 6 ConnectRestExtension > > > > > >> > > > implementations: > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > https://github.com/search?q=repo%3Adebezium%2Fdebezium+ConnectRestExtension+language%3AJava&type=code&l=Java > > > > > >> > > > . Some of these are baked into artifacts that I know for a > > > fact > > > > > are > > > > > >> > > > used in normal connect deployments. > > > > > >> > > > * I found a healthcheck extension that looks unmaintained: > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > https://github.com/LoObp4ck/kafka-connect-healthchecks/blob/2d9dbfee900d9f85e6acd9a09bd04969afa46261/src/main/java/com/loobpack/data/kafka/connect/healthcheck/extension/HealthCheckConnectRestExtension.java#L9 > > > > > >> > > > > > > > > >> > > > I figure that adopting this KIP would mean that the > Debezium > > > > > project > > > > > >> > > > would be forced to bump their major version 3.0 to be > > > compatible > > > > > >> with > > > > > >> > > > Connect 4.0, or otherwise change their packaging, so I'd > > like > > > to > > > > > >> hear > > > > > >> > > > from the Debezium folks what they think of this proposal. > > > > > >> > > > > > > > > >> > > > Thanks, > > > > > >> > > > Greg > > > > > >> > > > > > > > > >> > > > On Wed, Mar 27, 2024 at 4:43 PM Christopher Shannon > > > > > >> > > > <christopher.l.shan...@gmail.com> wrote: > > > > > >> > > > > > > > > > >> > > > > Hi, > > > > > >> > > > > > > > > > >> > > > > I'm proposing a KIP for Kafka 4.0 to upgrade to to > Jakarta > > > and > > > > > >> JavaEE 9 > > > > > >> > > > > APIs. This will also upgrade dependencies like Jetty and > > > move > > > > > >> away from > > > > > >> > > > > the depcrated javax namespace to be in line with other > > > > libraries > > > > > >> and > > > > > >> > > > > frameworks. There was some initial > > > > > >> > > > > < > > > > > https://lists.apache.org/thread/p4qbqh8r77h4khn3yoof2b0gbq3wbc5q > > > > > >> > > > > > > >> > > > > discussion and below is the KIP. > > > > > >> > > > > > > > > > >> > > > > Please take a look and let me know what you think: > > > > > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1032%3A+Upgrade+to+Jakarta+and+JavaEE+9+in+Kafka+4.0 > > > > > >> > > > > > > > > > >> > > > > Thanks, > > > > > >> > > > > Chris > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > >
