Re: [PR] KAFKA-16093: Fix spurious REST-related warnings on Connect startup [kafka]
C0urante commented on code in PR #15149:
URL: https://github.com/apache/kafka/pull/15149#discussion_r1447487472
##
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;
Review Comment:
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.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16093: Fix spurious REST-related warnings on Connect startup [kafka]
vamossagar12 commented on code in PR #15149:
URL: https://github.com/apache/kafka/pull/15149#discussion_r1447480579
##
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestRequestTimeout.java:
##
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.connect.runtime.rest;
+
+public interface RestRequestTimeout {
Review Comment:
Ok. Makes sense.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16093: Fix spurious REST-related warnings on Connect startup [kafka]
vamossagar12 commented on PR #15149: URL: https://github.com/apache/kafka/pull/15149#issuecomment-1884969533 @C0urante , hmm okay. I understand those weren't blocker comments and I called them as nits. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16093: Fix spurious REST-related warnings on Connect startup [kafka]
vamossagar12 commented on code in PR #15149:
URL: https://github.com/apache/kafka/pull/15149#discussion_r1447468334
##
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;
Review Comment:
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.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16093: Fix spurious REST-related warnings on Connect startup [kafka]
C0urante merged PR #15149: URL: https://github.com/apache/kafka/pull/15149 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16093: Fix spurious REST-related warnings on Connect startup [kafka]
C0urante commented on code in PR #15149:
URL: https://github.com/apache/kafka/pull/15149#discussion_r1447427203
##
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;
-public HerderRequestHandler(RestClient restClient, long requestTimeoutMs) {
+public HerderRequestHandler(RestClient restClient, RestRequestTimeout
requestTimeout) {
this.restClient = restClient;
-this.requestTimeoutMs = requestTimeoutMs;
-}
-
-public void requestTimeoutMs(long requestTimeoutMs) {
-if (requestTimeoutMs < 1) {
-throw new IllegalArgumentException("REST request timeout must be
positive");
-}
Review Comment:
I removed it for two reasons:
1. This code path is only ever used in testing code
2. 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`.
##
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestRequestTimeout.java:
##
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.connect.runtime.rest;
+
+public interface RestRequestTimeout {
Review Comment:
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.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16093: Fix spurious REST-related warnings on Connect startup [kafka]
C0urante commented on code in PR #15149:
URL: https://github.com/apache/kafka/pull/15149#discussion_r1447427024
##
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;
Review Comment:
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](https://github.com/C0urante/kafka/blob/0713736b9884084c360126558e4b6d92d4435bfa/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestServer.java#L533).
##
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/ConnectRestServer.java:
##
@@ -45,25 +45,40 @@ public void initializeResources(Herder herder) {
}
@Override
-protected Collection regularResources() {
+protected Collection> regularResources() {
return Arrays.asList(
-new RootResource(herder),
-new ConnectorsResource(herder, config, restClient),
-new InternalConnectResource(herder, restClient),
-new ConnectorPluginsResource(herder)
+RootResource.class,
+ConnectorsResource.class,
+InternalConnectResource.class,
+ConnectorPluginsResource.class
);
}
@Override
-protected Collection adminResources() {
+protected Collection> adminResources() {
return Arrays.asList(
-new LoggingResource(herder)
+LoggingResource.class
);
}
@Override
protected void configureRegularResources(ResourceConfig resourceConfig) {
registerRestExtensions(herder, resourceConfig);
+resourceConfig.register(new Binder());
+}
+
+private class Binder extends AbstractBinder {
+@Override
+protected void configure() {
+bind(herder).to(Herder.class);
+bind(restClient).to(RestClient.class);
+bind(config).to(RestServerConfig.class);
+}
+}
+
Review Comment:
This is true, but not a blocker.
##
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;
-public HerderRequestHandler(RestClient restClient, long requestTimeoutMs) {
+public HerderRequestHandler(RestClient restClient, RestRequestTimeout
requestTimeout) {
this.restClient = restClient;
-this.requestTimeoutMs = requestTimeoutMs;
-}
-
-public void requestTimeoutMs(long requestTimeoutMs) {
-if (requestTimeoutMs < 1) {
-throw new IllegalArgumentException("REST request timeout must be
positive");
-}
Review Comment:
I removed it for two reasons:
1. This code path is only ever used in testing code
2. It's unclear that the validation would be safer than allowing negative
values. At least on my machine, it doesn't cause any issues with, e.g.,
`ConvertingFutureCallback::get`.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16093: Fix spurious REST-related warnings on Connect startup [kafka]
C0urante commented on PR #15149: URL: https://github.com/apache/kafka/pull/15149#issuecomment-1884907770 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... -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16093: Fix spurious REST-related warnings on Connect startup [kafka]
vamossagar12 commented on code in PR #15149:
URL: https://github.com/apache/kafka/pull/15149#discussion_r1446980325
##
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;
Review Comment:
The field `requestTimeoutMs` was made volatile as part of
[here](https://github.com/apache/kafka/pull/14562/files#diff-f2311f0c356f882d7768b97cb5f0054dc7040d29d455eab74ab585725454488aR44).
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 :) )
##
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;
-public HerderRequestHandler(RestClient restClient, long requestTimeoutMs) {
+public HerderRequestHandler(RestClient restClient, RestRequestTimeout
requestTimeout) {
this.restClient = restClient;
-this.requestTimeoutMs = requestTimeoutMs;
-}
-
-public void requestTimeoutMs(long requestTimeoutMs) {
-if (requestTimeoutMs < 1) {
-throw new IllegalArgumentException("REST request timeout must be
positive");
-}
Review Comment:
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?
##
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/ConnectRestServer.java:
##
@@ -45,25 +45,40 @@ public void initializeResources(Herder herder) {
}
@Override
-protected Collection regularResources() {
+protected Collection> regularResources() {
return Arrays.asList(
-new RootResource(herder),
-new ConnectorsResource(herder, config, restClient),
-new InternalConnectResource(herder, restClient),
-new ConnectorPluginsResource(herder)
+RootResource.class,
+ConnectorsResource.class,
+InternalConnectResource.class,
+ConnectorPluginsResource.class
);
}
@Override
-protected Collection adminResources() {
+protected Collection> adminResources() {
return Arrays.asList(
-new LoggingResource(herder)
+LoggingResource.class
);
}
@Override
protected void configureRegularResources(ResourceConfig resourceConfig) {
registerRestExtensions(herder, resourceConfig);
+resourceConfig.register(new Binder());
+}
+
+private class Binder extends AbstractBinder {
+@Override
+protected void configure() {
+bind(herder).to(Herder.class);
+bind(restClient).to(RestClient.class);
+bind(config).to(RestServerConfig.class);
+}
+}
+
Review Comment:
nit: We could probably move the class definition to the bottom so that all
`configureXXXResources` method are together
##
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestRequestTimeout.java:
##
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.connect.runtime.rest;
+
+public interface RestRequestTimeout {
Review Comment:
nit: Mark this interface as `@FunctionalInterface`?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16093: Fix spurious REST-related warnings on Connect startup [kafka]
C0urante commented on code in PR #15149:
URL: https://github.com/apache/kafka/pull/15149#discussion_r1446506255
##
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestServer.java:
##
@@ -226,20 +235,20 @@ protected final void initializeResources() {
if (adminListeners == null) {
log.info("Adding admin resources to main listener");
adminResourceConfig = resourceConfig;
-Collection adminResources = adminResources();
-resources.addAll(adminResources);
+Collection> adminResources = adminResources();
adminResources.forEach(adminResourceConfig::register);
configureAdminResources(adminResourceConfig);
} else if (adminListeners.size() > 0) {
// TODO: we need to check if these listeners are same as
'listeners'
// TODO: the following code assumes that they are different
log.info("Adding admin resources to admin listener");
adminResourceConfig = new ResourceConfig();
+adminResourceConfig.register(requestTimeout.binder());
adminResourceConfig.register(new JacksonJsonProvider());
-Collection adminResources = adminResources();
-resources.addAll(adminResources);
+Collection> adminResources = adminResources();
adminResources.forEach(adminResourceConfig::register);
adminResourceConfig.register(ConnectExceptionMapper.class);
+
adminResourceConfig.property(ServerProperties.WADL_FEATURE_DISABLE, true);
Review Comment:
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
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16093: Fix spurious REST-related warnings on Connect startup [kafka]
C0urante commented on code in PR #15149:
URL: https://github.com/apache/kafka/pull/15149#discussion_r1446490147
##
connect/runtime/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;
Review Comment:
Good catch, thanks
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16093: Fix spurious REST-related warnings on Connect startup [kafka]
gharris1727 commented on code in PR #15149:
URL: https://github.com/apache/kafka/pull/15149#discussion_r1446377748
##
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestServer.java:
##
@@ -226,20 +235,20 @@ protected final void initializeResources() {
if (adminListeners == null) {
log.info("Adding admin resources to main listener");
adminResourceConfig = resourceConfig;
-Collection adminResources = adminResources();
-resources.addAll(adminResources);
+Collection> adminResources = adminResources();
adminResources.forEach(adminResourceConfig::register);
configureAdminResources(adminResourceConfig);
} else if (adminListeners.size() > 0) {
// TODO: we need to check if these listeners are same as
'listeners'
// TODO: the following code assumes that they are different
log.info("Adding admin resources to admin listener");
adminResourceConfig = new ResourceConfig();
+adminResourceConfig.register(requestTimeout.binder());
adminResourceConfig.register(new JacksonJsonProvider());
-Collection adminResources = adminResources();
-resources.addAll(adminResources);
+Collection> adminResources = adminResources();
adminResources.forEach(adminResourceConfig::register);
adminResourceConfig.register(ConnectExceptionMapper.class);
+
adminResourceConfig.property(ServerProperties.WADL_FEATURE_DISABLE, true);
Review Comment:
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.
##
connect/runtime/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;
Review Comment:
nit: unused
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16093: Fix spurious REST-related warnings on Connect startup [kafka]
C0urante commented on code in PR #15149:
URL: https://github.com/apache/kafka/pull/15149#discussion_r1446244411
##
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestServer.java:
##
@@ -226,20 +235,20 @@ protected final void initializeResources() {
if (adminListeners == null) {
log.info("Adding admin resources to main listener");
adminResourceConfig = resourceConfig;
-Collection adminResources = adminResources();
-resources.addAll(adminResources);
+Collection> adminResources = adminResources();
adminResources.forEach(adminResourceConfig::register);
configureAdminResources(adminResourceConfig);
} else if (adminListeners.size() > 0) {
// TODO: we need to check if these listeners are same as
'listeners'
// TODO: the following code assumes that they are different
log.info("Adding admin resources to admin listener");
adminResourceConfig = new ResourceConfig();
+adminResourceConfig.register(requestTimeout.binder());
adminResourceConfig.register(new JacksonJsonProvider());
-Collection adminResources = adminResources();
-resources.addAll(adminResources);
+Collection> adminResources = adminResources();
adminResources.forEach(adminResourceConfig::register);
adminResourceConfig.register(ConnectExceptionMapper.class);
+
adminResourceConfig.property(ServerProperties.WADL_FEATURE_DISABLE, true);
Review Comment:
This fixes another spurious warning message that gets emitted if the worker
is configured with the `admin.listeners` property.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16093: Fix spurious REST-related warnings on Connect startup [kafka]
C0urante commented on PR #15149: URL: https://github.com/apache/kafka/pull/15149#issuecomment-1883267414 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. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16093: Fix spurious REST-related warnings on Connect startup [kafka]
gharris1727 commented on code in PR #15149:
URL: https://github.com/apache/kafka/pull/15149#discussion_r1445493874
##
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectResource.java:
##
@@ -16,12 +16,15 @@
*/
package org.apache.kafka.connect.runtime.rest.resources;
+import javax.ws.rs.container.ContainerRequestContext;
+import javax.ws.rs.container.ContainerRequestFilter;
+import java.io.IOException;
import java.util.concurrent.TimeUnit;
/**
* This interface defines shared logic for all Connect REST resources.
*/
-public interface ConnectResource {
+public interface ConnectResource extends ContainerRequestFilter {
Review Comment:
> I did some local experimenting and it seems like the @ConstrainedTo
annotation isn't actually necessary--let me know if I'm missing something,
though.
I think you're right! I think that was from a slightly earlier iteration I
had where i was trying to target [this first
condition](https://github.com/eclipse-ee4j/jersey/blob/74bf3220a2dd5f2067e60c110d4981591e87604e/core-common/src/main/java/org/glassfish/jersey/internal/inject/Providers.java#L439)
until I realized I couldn't get that one to work, and started targeting [this
second
condition](https://github.com/eclipse-ee4j/jersey/blob/74bf3220a2dd5f2067e60c110d4981591e87604e/core-common/src/main/java/org/glassfish/jersey/internal/inject/Providers.java#L443)
instead.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16093: Fix spurious REST-related warnings on Connect startup [kafka]
C0urante commented on code in PR #15149:
URL: https://github.com/apache/kafka/pull/15149#discussion_r1445387176
##
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectResource.java:
##
@@ -16,12 +16,15 @@
*/
package org.apache.kafka.connect.runtime.rest.resources;
+import javax.ws.rs.container.ContainerRequestContext;
+import javax.ws.rs.container.ContainerRequestFilter;
+import java.io.IOException;
import java.util.concurrent.TimeUnit;
/**
* This interface defines shared logic for all Connect REST resources.
*/
-public interface ConnectResource {
+public interface ConnectResource extends ContainerRequestFilter {
Review Comment:
They're both hacks, and both require implementing classes to be annotated
with `@Singleton` to avoid unnecessary warnings, but this seems slightly
cleaner (especially since it doesn't automatically implement the request filter
interface that we may want to use for real later on).
I did some local experimenting and it seems like the `@ConstrainedTo`
annotation isn't actually necessary--let me know if I'm missing something,
though.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16093: Fix spurious REST-related warnings on Connect startup [kafka]
gharris1727 commented on code in PR #15149:
URL: https://github.com/apache/kafka/pull/15149#discussion_r1445300416
##
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectResource.java:
##
@@ -16,12 +16,15 @@
*/
package org.apache.kafka.connect.runtime.rest.resources;
+import javax.ws.rs.container.ContainerRequestContext;
+import javax.ws.rs.container.ContainerRequestFilter;
+import java.io.IOException;
import java.util.concurrent.TimeUnit;
/**
* This interface defines shared logic for all Connect REST resources.
*/
-public interface ConnectResource {
+public interface ConnectResource extends ContainerRequestFilter {
Review Comment:
I found an alternative workaround, it's up to you if you like it better or
worse than adding the no-op filter method.
The logic for emitting that warning in `Providers.checkProviderRuntime` is
checking that there is something to constrain the component being checked to
the current runtime (client vs server). The ContainerRequestFilter is marked as
introducing a SERVER constraint in `Providers.getJaxRsProviderInterfaces`, and
by extending it we are just inheriting that constraint.
Instead we can use the annotation mechanism intended for adding constraints
to third-party SPIs to make the ConnectResource itself look like an SPI to
Jersey. I tried adding this to the classes themselves next to the `Singleton`
annotations, and it exploded. But it seems to work when the annotations are
here on the interface instead.
```suggestion
@Contract
@ConstrainedTo(RuntimeType.SERVER)
public interface ConnectResource {
```
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
