KAFKA-16093: Fix spurious REST-related warnings on Connect startup #15149

C0urante · 2024-01-08T18:50:40Z

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

… spurious warnings

…ct REST resources, fixing spurious warnings

gharris1727

Oh my... I thought these warnings were here to stay! I've seen them so many times that I couldn't understand what they meant, even though they are very direct about what is incorrect.

Thank you so much for fixing this!

...t/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectResource.java

…ilter

gharris1727

LGTM, thanks Chris! No more nuisance stderr from tests!

…ey resources

C0urante · 2024-01-09T15:29:40Z

Thanks @gharris1727!

I wanted to take a stab at fixing these errors "correctly", i.e., by registering classes instead of instances with the regular and admin ResourceConfig. I've pushed a commit that does this; it's a bit lengthier, but it does work. Let me know what you think; happy to revert and stick with the hack if we'd rather minimize the diff.

C0urante · 2024-01-09T15:30:28Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestServer.java

            adminResources.forEach(adminResourceConfig::register);
            adminResourceConfig.register(ConnectExceptionMapper.class);
+            adminResourceConfig.property(ServerProperties.WADL_FEATURE_DISABLE, true);


This fixes another spurious warning message that gets emitted if the worker is configured with the admin.listeners property.

this is some light duplication, perhaps the ResourceConfig constructor and the 4 lines accompanying it (request timeout, jackson, exception mapper, and wadl feature disable) can be deduplicated.

Good call, done. I've also cleaned up the configuration of the adminResourceConfig to hopefully prevent other kinds of duplication-related issues in the future; LMKWYT

gharris1727

I really like this approach, and I agree it's the non-hacky way forward. TIL about AbstractBinder and the dependency injection mechanism that Jersey provides.

In particular I like removing ConnectResource, and making the RestRequestTimeout object mutable instead of having all of the resources themselves mutable.

gharris1727 · 2024-01-09T16:48:37Z

...me/src/test/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResourceTest.java

@@ -159,7 +163,8 @@ public class ConnectorsResourceTest {
    public void setUp() throws NoSuchMethodException {
        when(serverConfig.topicTrackingEnabled()).thenReturn(true);
        when(serverConfig.topicTrackingResetEnabled()).thenReturn(true);
-        connectorsResource = new ConnectorsResource(herder, serverConfig, restClient);
+        RestRequestTimeout requestTimeout = () -> RestServer.DEFAULT_REST_REQUEST_TIMEOUT_MS;


nit: unused

Good catch, thanks 👍

gharris1727 · 2024-01-09T17:09:54Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestServer.java

            adminResources.forEach(adminResourceConfig::register);
            adminResourceConfig.register(ConnectExceptionMapper.class);
+            adminResourceConfig.property(ServerProperties.WADL_FEATURE_DISABLE, true);


this is some light duplication, perhaps the ResourceConfig constructor and the 4 lines accompanying it (request timeout, jackson, exception mapper, and wadl feature disable) can be deduplicated.

- Deduplicate RestServer instantiation and configuration of ResourceConfig instances - Remove unused RestRequestTimeout instance from ConnectorsResourceTest - Fix failing ConnectRestServerTest suite

gharris1727

LGTM, thanks Chris!

vamossagar12

Thanks for looking into these log messages! Added some nit level comments.

vamossagar12 · 2024-01-10T07:31:12Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/HerderRequestHandler.java

@@ -41,18 +41,11 @@ public class HerderRequestHandler {

    private final RestClient restClient;

-    private volatile long requestTimeoutMs;
+    private final RestRequestTimeout requestTimeout;


The field requestTimeoutMs was made volatile as part of here. I don't think we need it now, but just wanted to understand why was it added over there (you can skip the explanation if it's too complex :) )

This field is final, it wouldn't make sense to mark it volatile.

The underlying mutable value that it returns from timeoutMs() is still marked volatile in the only non-test implementation here.

I didn't intend to mark the final field as volatile. I wanted to know why the field was marked as volatile in the old PR since you had made the change.

Ah, gotcha. The intent was to make it thread-safe since it's likely that writes and reads to/of that value will take place on different threads.

vamossagar12 · 2024-01-10T07:33:35Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/ConnectRestServer.java

+    private class Binder extends AbstractBinder {
+        @Override
+        protected void configure() {
+            bind(herder).to(Herder.class);
+            bind(restClient).to(RestClient.class);
+            bind(config).to(RestServerConfig.class);
+        }
+    }
+


nit: We could probably move the class definition to the bottom so that all configureXXXResources method are together

This is true, but not a blocker.

vamossagar12 · 2024-01-10T07:47:47Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/HerderRequestHandler.java

-        if (requestTimeoutMs < 1) {
-            throw new IllegalArgumentException("REST request timeout must be positive");
-        }


I am not sure if this validation was needed in the past as well, but now I don't see it being present. Do you think it's needed?

I removed it for two reasons:

This code path is only ever used in testing code

It's unclear that the validation would be safer than allowing negative values. At least on my machine, negative values don't cause any issues with, e.g., ConvertingFutureCallback::get.

vamossagar12 · 2024-01-10T07:52:17Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestRequestTimeout.java

+ */
+package org.apache.kafka.connect.runtime.rest;
+
+public interface RestRequestTimeout {


nit: Mark this interface as @FunctionalInterface?

I could see some benefit to this, but it's also not a blocker.

It's also debatable, since this interface may change in the future and we don't want to imply that it can only have exactly one abstract method.

Ok. Makes sense.

C0urante · 2024-01-10T14:02:48Z

Thanks @gharris1727!

@vamossagar12 I appreciate the review but none of these seem like blocking comments. Please try to use the "request changes" button sparingly. Thanks!

Test failures appear unrelated and all come from known flaky tests. Merging...

…15149) Reviewers: Sagar Rao <sagarmeansocean@gmail.com>, Greg Harris <greg.harris@aiven.io>

vamossagar12 · 2024-01-10T14:36:55Z

@C0urante , hmm okay. I understand those weren't blocker comments and I called them as nits.

…pache#15149) Reviewers: Sagar Rao <sagarmeansocean@gmail.com>, Greg Harris <greg.harris@aiven.io>

…15149) Reviewers: Sagar Rao <sagarmeansocean@gmail.com>, Greg Harris <greg.harris@aiven.io>

C0urante added 2 commits January 8, 2024 12:24

MINOR: Remove empty @path annotations for Connect REST server, fixing…

e0ac9ff

… spurious warnings

MINOR: Implement ContainerRequestFilter interface with no-op in Conne…

51f7627

…ct REST resources, fixing spurious warnings

gharris1727 reviewed Jan 8, 2024

View reviewed changes

...t/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectResource.java Outdated Show resolved Hide resolved

gharris1727 added the connect label Jan 8, 2024

Change to @contract annotation instead of extending ContainerRequestF…

eeebab0

…ilter

gharris1727 approved these changes Jan 9, 2024

View reviewed changes

Use AbstractBinder and @Inject annotations to correctly register Jers…

7c4ccc8

…ey resources

C0urante commented Jan 9, 2024

View reviewed changes

gharris1727 self-requested a review January 9, 2024 17:10

gharris1727 reviewed Jan 9, 2024

View reviewed changes

Address review comments

0713736

- Deduplicate RestServer instantiation and configuration of ResourceConfig instances - Remove unused RestRequestTimeout instance from ConnectorsResourceTest - Fix failing ConnectRestServerTest suite

gharris1727 approved these changes Jan 9, 2024

View reviewed changes

vamossagar12 suggested changes Jan 10, 2024

View reviewed changes

C0urante merged commit dbf00bc into apache:trunk Jan 10, 2024
1 check failed

C0urante deleted the kafka-16093 branch January 10, 2024 14:03

C0urante added a commit that referenced this pull request Jan 10, 2024

KAFKA-16093: Fix spurious REST-related warnings on Connect startup (#…

fd0d43f

…15149) Reviewers: Sagar Rao <sagarmeansocean@gmail.com>, Greg Harris <greg.harris@aiven.io>

showuon pushed a commit to showuon/kafka that referenced this pull request Jan 22, 2024

KAFKA-16093: Fix spurious REST-related warnings on Connect startup (a…

906df3b

…pache#15149) Reviewers: Sagar Rao <sagarmeansocean@gmail.com>, Greg Harris <greg.harris@aiven.io>

yyu1993 pushed a commit to yyu1993/kafka that referenced this pull request Feb 15, 2024

KAFKA-16093: Fix spurious REST-related warnings on Connect startup (a…

d428ee3

…pache#15149) Reviewers: Sagar Rao <sagarmeansocean@gmail.com>, Greg Harris <greg.harris@aiven.io>

clolov pushed a commit to clolov/kafka that referenced this pull request Apr 5, 2024

KAFKA-16093: Fix spurious REST-related warnings on Connect startup (a…

0a18837

…pache#15149) Reviewers: Sagar Rao <sagarmeansocean@gmail.com>, Greg Harris <greg.harris@aiven.io>

C0urante added a commit that referenced this pull request May 8, 2024

KAFKA-16093: Fix spurious REST-related warnings on Connect startup (#…

88dd2b1

…15149) Reviewers: Sagar Rao <sagarmeansocean@gmail.com>, Greg Harris <greg.harris@aiven.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-16093: Fix spurious REST-related warnings on Connect startup #15149

KAFKA-16093: Fix spurious REST-related warnings on Connect startup #15149

C0urante commented Jan 8, 2024

gharris1727 left a comment

gharris1727 left a comment

C0urante commented Jan 9, 2024

C0urante Jan 9, 2024

gharris1727 Jan 9, 2024

C0urante Jan 9, 2024

gharris1727 left a comment

gharris1727 Jan 9, 2024

C0urante Jan 9, 2024

gharris1727 Jan 9, 2024

gharris1727 left a comment

vamossagar12 left a comment

vamossagar12 Jan 10, 2024

C0urante Jan 10, 2024

vamossagar12 Jan 10, 2024

C0urante Jan 10, 2024

vamossagar12 Jan 10, 2024

C0urante Jan 10, 2024

vamossagar12 Jan 10, 2024

C0urante Jan 10, 2024 •

edited

vamossagar12 Jan 10, 2024

C0urante Jan 10, 2024

vamossagar12 Jan 10, 2024

C0urante commented Jan 10, 2024

vamossagar12 commented Jan 10, 2024

KAFKA-16093: Fix spurious REST-related warnings on Connect startup #15149

KAFKA-16093: Fix spurious REST-related warnings on Connect startup #15149

Conversation

C0urante commented Jan 8, 2024

Committer Checklist (excluded from commit message)

gharris1727 left a comment

Choose a reason for hiding this comment

gharris1727 left a comment

Choose a reason for hiding this comment

C0urante commented Jan 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gharris1727 left a 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

gharris1727 left a comment

Choose a reason for hiding this comment

vamossagar12 left a 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

Choose a reason for hiding this comment

C0urante Jan 10, 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

C0urante commented Jan 10, 2024

vamossagar12 commented Jan 10, 2024

C0urante Jan 10, 2024 •

edited