You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "gomudayya (via GitHub)" <gi...@apache.org> on 2026/04/08 15:17:01 UTC
[PR] KAFKA-20391: Improve types returned by the Connect REST API [kafka]
gomudayya opened a new pull request, #22001:
URL: https://github.com/apache/kafka/pull/22001
- Replace generic `Response` return types with specific types in `LoggingResource` and `ConnectorsResource` to improve OpenAPI spec generation
- Update corresponding unit tests
- Endpoints returning non-200 status codes (201, 202, 204) or using conditional status codes were left unchanged as they require `Response` for status code control.
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "gomudayya (via GitHub)" <gi...@apache.org>.
gomudayya commented on code in PR #22001:
URL: https://github.com/apache/kafka/pull/22001#discussion_r3107140038
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -368,21 +368,19 @@ public Response alterConnectorOffsets(final @Parameter(hidden = true) @QueryPara
FutureCallback<Message> cb = new FutureCallback<>();
herder.alterConnectorOffsets(connector, offsets.toMap(), cb);
- Message msg = requestHandler.completeOrForwardRequest(cb, "/connectors/" + connector + "/offsets", "PATCH", headers, offsets,
+ return requestHandler.completeOrForwardRequest(cb, "/connectors/" + connector + "/offsets", "PATCH", headers, offsets,
new TypeReference<>() { }, new IdentityTranslator<>(), forward);
- return Response.ok().entity(msg).build();
Review Comment:
In JAX-RS, returning an entity directly results in 200 OK with the serialized body — equivalent to Response.ok(entity).build(). So the status code is preserved and the response format (status code, response body) remains identical from the client's perspective.
(ref: https://jakarta.ee/specifications/restful-ws/3.0/jakarta-restful-ws-spec-3.0.html#resource_method_return — "If the return value is not null a 200 status code is used")
Since there's no client-visible behavior change, I don't believe a KIP is required here.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/LoggingResource.java:
##########
@@ -126,11 +125,10 @@ public Response setLevel(final @PathParam("logger") String namespace,
default:
log.warn("Received invalid scope '{}' in request to adjust logging level; will default to {}", scope, WORKER_SCOPE);
case WORKER_SCOPE:
- List<String> affectedLoggers = herder.setWorkerLoggerLevel(namespace, levelString);
- return Response.ok(affectedLoggers).build();
+ return herder.setWorkerLoggerLevel(namespace, levelString);
case CLUSTER_SCOPE:
herder.setClusterLoggerLevel(namespace, levelString);
- return Response.noContent().build();
+ return null;
Review Comment:
When a JAX-RS resource method returns null, the framework automatically responds with 204 No Content.
but If a more explicit return is preferred, I'm happy to revert this to use Response.
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #22001:
URL: https://github.com/apache/kafka/pull/22001#discussion_r3529678406
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/LoggingResource.java:
##########
@@ -104,7 +103,7 @@ public Response getLogger(final @PathParam("logger") String namedLogger) {
@Path("/{logger}")
@Operation(summary = "Set the log level for the specified logger")
@SuppressWarnings("fallthrough")
- public Response setLevel(final @PathParam("logger") String namespace,
+ public List<String> setLevel(final @PathParam("logger") String namespace,
Review Comment:
How is this going to work with the no-content response for cluster scope loggers?
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on code in PR #22001:
URL: https://github.com/apache/kafka/pull/22001#discussion_r3067682802
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -368,21 +368,19 @@ public Response alterConnectorOffsets(final @Parameter(hidden = true) @QueryPara
FutureCallback<Message> cb = new FutureCallback<>();
herder.alterConnectorOffsets(connector, offsets.toMap(), cb);
- Message msg = requestHandler.completeOrForwardRequest(cb, "/connectors/" + connector + "/offsets", "PATCH", headers, offsets,
+ return requestHandler.completeOrForwardRequest(cb, "/connectors/" + connector + "/offsets", "PATCH", headers, offsets,
new TypeReference<>() { }, new IdentityTranslator<>(), forward);
- return Response.ok().entity(msg).build();
Review Comment:
With this return we are missing the status code now. Aren't these breaking changes and need a kip ?
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on code in PR #22001:
URL: https://github.com/apache/kafka/pull/22001#discussion_r3067683980
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/LoggingResource.java:
##########
@@ -69,8 +68,8 @@ public LoggingResource(Herder herder) {
*/
@GET
@Operation(summary = "List the current loggers that have their levels explicitly set and their log levels")
- public Response listLoggers() {
- return Response.ok(herder.allLoggerLevels()).build();
+ public Map<String, LoggerLevel> listLoggers() {
Review Comment:
Same here, could be better if an entity (similar to what we have in org/apache/kafka/connect/runtime/rest/entities) is returned instead of a Map
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on code in PR #22001:
URL: https://github.com/apache/kafka/pull/22001#discussion_r3067678798
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -184,13 +184,13 @@ public ConnectorStateInfo getConnectorStatus(final @PathParam("connector") Strin
@GET
@Path("/{connector}/topics")
@Operation(summary = "Get the list of topics actively used by the specified connector")
- public Response getConnectorActiveTopics(final @PathParam("connector") String connector) {
+ public Map<String, ActiveTopicsInfo> getConnectorActiveTopics(final @PathParam("connector") String connector) {
Review Comment:
To align with other endpoints, it is better to create an entity instead of Map
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "gomudayya (via GitHub)" <gi...@apache.org>.
gomudayya commented on code in PR #22001:
URL: https://github.com/apache/kafka/pull/22001#discussion_r4232126642
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/LoggingResource.java:
##########
@@ -104,7 +103,7 @@ public Response getLogger(final @PathParam("logger") String namedLogger) {
@Path("/{logger}")
@Operation(summary = "Set the log level for the specified logger")
@SuppressWarnings("fallthrough")
- public Response setLevel(final @PathParam("logger") String namespace,
+ public List<String> setLevel(final @PathParam("logger") String namespace,
Review Comment:
Sounds good. I added a comment.
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "gomudayya (via GitHub)" <gi...@apache.org>.
gomudayya commented on code in PR #22001:
URL: https://github.com/apache/kafka/pull/22001#discussion_r4227370073
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/LoggingResource.java:
##########
@@ -104,7 +103,7 @@ public Response getLogger(final @PathParam("logger") String namedLogger) {
@Path("/{logger}")
@Operation(summary = "Set the log level for the specified logger")
@SuppressWarnings("fallthrough")
- public Response setLevel(final @PathParam("logger") String namespace,
+ public List<String> setLevel(final @PathParam("logger") String namespace,
Review Comment:
Sorry for the late reply!
When the scope is cluster, the method returns null, and JAX-RS translates a null return value into a 204 No Content response, so compatibility is preserved.
> If the return value is not null a 200 status code is used, **a null return value results in a 204 status code.**
> (https://jakarta.ee/specifications/restful-ws/3.1/jakarta-restful-ws-spec-3.1.html#resource_method_return)
However, since this behavior is a bit implicit, I’m happy to follow your preference: keep `List<String>` (with a comment explaining it, if needed), or revert to the generic Response type.
---
I also rebased on the latest trunk. The only change besides the rebase is resolving a conflict in `LoggingResourceTest` (switched to the new `LogCaptureAppender.getMessages(Level.WARN)` API).
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on code in PR #22001:
URL: https://github.com/apache/kafka/pull/22001#discussion_r3067684454
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/LoggingResource.java:
##########
@@ -69,8 +68,8 @@ public LoggingResource(Herder herder) {
*/
@GET
@Operation(summary = "List the current loggers that have their levels explicitly set and their log levels")
- public Response listLoggers() {
- return Response.ok(herder.allLoggerLevels()).build();
+ public Map<String, LoggerLevel> listLoggers() {
Review Comment:
Is it ok to loose the 'ok' status code ?
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/LoggingResource.java:
##########
@@ -69,8 +68,8 @@ public LoggingResource(Herder herder) {
*/
@GET
@Operation(summary = "List the current loggers that have their levels explicitly set and their log levels")
- public Response listLoggers() {
- return Response.ok(herder.allLoggerLevels()).build();
+ public Map<String, LoggerLevel> listLoggers() {
Review Comment:
Is it ok to loose the 'ok' status code ?
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "muralibasani (via GitHub)" <gi...@apache.org>.
muralibasani commented on code in PR #22001:
URL: https://github.com/apache/kafka/pull/22001#discussion_r3067684747
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/LoggingResource.java:
##########
@@ -69,8 +68,8 @@ public LoggingResource(Herder herder) {
*/
@GET
@Operation(summary = "List the current loggers that have their levels explicitly set and their log levels")
- public Response listLoggers() {
- return Response.ok(herder.allLoggerLevels()).build();
Review Comment:
Is it ok to loose the 'ok' status code ?
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "gomudayya (via GitHub)" <gi...@apache.org>.
gomudayya commented on code in PR #22001:
URL: https://github.com/apache/kafka/pull/22001#discussion_r3107165488
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -368,21 +368,19 @@ public Response alterConnectorOffsets(final @Parameter(hidden = true) @QueryPara
FutureCallback<Message> cb = new FutureCallback<>();
herder.alterConnectorOffsets(connector, offsets.toMap(), cb);
- Message msg = requestHandler.completeOrForwardRequest(cb, "/connectors/" + connector + "/offsets", "PATCH", headers, offsets,
+ return requestHandler.completeOrForwardRequest(cb, "/connectors/" + connector + "/offsets", "PATCH", headers, offsets,
new TypeReference<>() { }, new IdentityTranslator<>(), forward);
- return Response.ok().entity(msg).build();
Review Comment:
In JAX-RS, returning an entity directly results in 200 OK with the serialized body — equivalent to Response.ok(entity).build(). So the status code is preserved and the response format (status code, response body) remains identical from the client's perspective.
(ref: https://jakarta.ee/specifications/restful-ws/3.0/jakarta-restful-ws-spec-3.0.html#resource_method_return — "If the return value is not null a 200 status code is used")
Since there's no client-visible behavior change, I don't believe a KIP is required here.
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "gomudayya (via GitHub)" <gi...@apache.org>.
gomudayya commented on code in PR #22001:
URL: https://github.com/apache/kafka/pull/22001#discussion_r3052389031
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/LoggingResource.java:
##########
@@ -126,11 +125,10 @@ public Response setLevel(final @PathParam("logger") String namespace,
default:
log.warn("Received invalid scope '{}' in request to adjust logging level; will default to {}", scope, WORKER_SCOPE);
case WORKER_SCOPE:
- List<String> affectedLoggers = herder.setWorkerLoggerLevel(namespace, levelString);
- return Response.ok(affectedLoggers).build();
+ return herder.setWorkerLoggerLevel(namespace, levelString);
case CLUSTER_SCOPE:
herder.setClusterLoggerLevel(namespace, levelString);
- return Response.noContent().build();
+ return null;
Review Comment:
When a JAX-RS resource method returns null, the framework automatically responds with 204 No Content.
but If a more explicit return is preferred, I'm happy to revert this to use Response.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -184,13 +184,13 @@ public ConnectorStateInfo getConnectorStatus(final @PathParam("connector") Strin
@GET
@Path("/{connector}/topics")
@Operation(summary = "Get the list of topics actively used by the specified connector")
- public Response getConnectorActiveTopics(final @PathParam("connector") String connector) {
+ public Map<String, ActiveTopicsInfo> getConnectorActiveTopics(final @PathParam("connector") String connector) {
Review Comment:
Changing to an entity would alter the response structure, which requires a KIP and is outside the scope of this ticket.
Using `@JsonValue` could wrap it in an entity while preserving the response format, but `@JsonValue` is easy to overlook and it's essentially just a wrapper around Map, so the benefit seems marginal.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -368,21 +368,19 @@ public Response alterConnectorOffsets(final @Parameter(hidden = true) @QueryPara
FutureCallback<Message> cb = new FutureCallback<>();
herder.alterConnectorOffsets(connector, offsets.toMap(), cb);
- Message msg = requestHandler.completeOrForwardRequest(cb, "/connectors/" + connector + "/offsets", "PATCH", headers, offsets,
+ return requestHandler.completeOrForwardRequest(cb, "/connectors/" + connector + "/offsets", "PATCH", headers, offsets,
new TypeReference<>() { }, new IdentityTranslator<>(), forward);
- return Response.ok().entity(msg).build();
Review Comment:
In JAX-RS, returning an entity directly results in 200 OK with the serialized body — equivalent to Response.ok(entity).build(). So the status code is preserved and the response format (status code, response body) remains identical from the client's perspective.
(ref: https://jakarta.ee/specifications/restful-ws/3.0/jakarta-restful-ws-spec-3.0.html#resource_method_return — "If the return value is not null a 200 status code is used")
Since there's no client-visible behavior change, I don't believe a KIP is required here.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/LoggingResource.java:
##########
@@ -69,8 +68,8 @@ public LoggingResource(Herder herder) {
*/
@GET
@Operation(summary = "List the current loggers that have their levels explicitly set and their log levels")
- public Response listLoggers() {
- return Response.ok(herder.allLoggerLevels()).build();
+ public Map<String, LoggerLevel> listLoggers() {
Review Comment:
Same as my reply on getConnectorActiveTopics — wrapping into an entity would alter the response structure and require a KIP, which is outside the scope of this PR.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/LoggingResource.java:
##########
@@ -69,8 +68,8 @@ public LoggingResource(Herder herder) {
*/
@GET
@Operation(summary = "List the current loggers that have their levels explicitly set and their log levels")
- public Response listLoggers() {
- return Response.ok(herder.allLoggerLevels()).build();
Review Comment:
Same as my reply on alterConnectorOffsets — 200 OK is preserved by default.
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #22001:
URL: https://github.com/apache/kafka/pull/22001#discussion_r4231653785
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -234,15 +234,15 @@ public Response putConnectorConfig(final @PathParam("connector") String connecto
@PATCH
@Path("/{connector}/config")
- public Response patchConnectorConfig(final @PathParam("connector") String connector,
+ public ConnectorInfo patchConnectorConfig(final @PathParam("connector") String connector,
Review Comment:
Could we add the `@Operation` annotation here?
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison merged PR #22001:
URL: https://github.com/apache/kafka/pull/22001
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "gomudayya (via GitHub)" <gi...@apache.org>.
gomudayya commented on code in PR #22001:
URL: https://github.com/apache/kafka/pull/22001#discussion_r3107124472
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -184,13 +184,13 @@ public ConnectorStateInfo getConnectorStatus(final @PathParam("connector") Strin
@GET
@Path("/{connector}/topics")
@Operation(summary = "Get the list of topics actively used by the specified connector")
- public Response getConnectorActiveTopics(final @PathParam("connector") String connector) {
+ public Map<String, ActiveTopicsInfo> getConnectorActiveTopics(final @PathParam("connector") String connector) {
Review Comment:
Changing to an entity would alter the response structure, which requires a KIP and is outside the scope of this ticket.
Using `@JsonValue` could wrap it in an entity while preserving the response format, but `@JsonValue` is easy to overlook and it's essentially just a wrapper around Map, so the benefit seems marginal.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/LoggingResource.java:
##########
@@ -69,8 +68,8 @@ public LoggingResource(Herder herder) {
*/
@GET
@Operation(summary = "List the current loggers that have their levels explicitly set and their log levels")
- public Response listLoggers() {
- return Response.ok(herder.allLoggerLevels()).build();
+ public Map<String, LoggerLevel> listLoggers() {
Review Comment:
Same as my reply on getConnectorActiveTopics — wrapping into an entity would alter the response structure and require a KIP, which is outside the scope of this PR.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/LoggingResource.java:
##########
@@ -69,8 +68,8 @@ public LoggingResource(Herder herder) {
*/
@GET
@Operation(summary = "List the current loggers that have their levels explicitly set and their log levels")
- public Response listLoggers() {
- return Response.ok(herder.allLoggerLevels()).build();
Review Comment:
Same as my reply on alterConnectorOffsets — 200 OK is preserved by default.
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "gomudayya (via GitHub)" <gi...@apache.org>.
gomudayya commented on code in PR #22001:
URL: https://github.com/apache/kafka/pull/22001#discussion_r3107165128
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -184,13 +184,13 @@ public ConnectorStateInfo getConnectorStatus(final @PathParam("connector") Strin
@GET
@Path("/{connector}/topics")
@Operation(summary = "Get the list of topics actively used by the specified connector")
- public Response getConnectorActiveTopics(final @PathParam("connector") String connector) {
+ public Map<String, ActiveTopicsInfo> getConnectorActiveTopics(final @PathParam("connector") String connector) {
Review Comment:
Changing to an entity would alter the response structure, which requires a KIP and is outside the scope of this ticket.
Using @JsonValue could wrap it in an entity while preserving the response format, but @JsonValue isn't widely known and could be confusing for future readers, and it's essentially just a wrapper around Map, so the benefit seems marginal.
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #22001:
URL: https://github.com/apache/kafka/pull/22001#discussion_r4231645739
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/LoggingResource.java:
##########
@@ -104,7 +103,7 @@ public Response getLogger(final @PathParam("logger") String namedLogger) {
@Path("/{logger}")
@Operation(summary = "Set the log level for the specified logger")
@SuppressWarnings("fallthrough")
- public Response setLevel(final @PathParam("logger") String namespace,
+ public List<String> setLevel(final @PathParam("logger") String namespace,
Review Comment:
Thanks for following up. I think we can keep `List<String>` but it may be worth adding a comment.
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "gomudayya (via GitHub)" <gi...@apache.org>.
gomudayya commented on code in PR #22001:
URL: https://github.com/apache/kafka/pull/22001#discussion_r3107166000
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/LoggingResource.java:
##########
@@ -69,8 +68,8 @@ public LoggingResource(Herder herder) {
*/
@GET
@Operation(summary = "List the current loggers that have their levels explicitly set and their log levels")
- public Response listLoggers() {
- return Response.ok(herder.allLoggerLevels()).build();
Review Comment:
Same as my reply on alterConnectorOffsets — 200 OK is preserved by default.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/LoggingResource.java:
##########
@@ -69,8 +68,8 @@ public LoggingResource(Herder herder) {
*/
@GET
@Operation(summary = "List the current loggers that have their levels explicitly set and their log levels")
- public Response listLoggers() {
- return Response.ok(herder.allLoggerLevels()).build();
+ public Map<String, LoggerLevel> listLoggers() {
Review Comment:
Same as my reply on getConnectorActiveTopics.
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "gomudayya (via GitHub)" <gi...@apache.org>.
gomudayya commented on PR #22001:
URL: https://github.com/apache/kafka/pull/22001#issuecomment-4276348950
> @gomudayya I have a few comments but main concern is by changing the return type, we are missing status code and is it ok for this ticket without kip? However to align with other resources, looks ok.
Thanks for the review! I've replied to each of your comments individually — please take a look and let me know if anything needs clarification.
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "gomudayya (via GitHub)" <gi...@apache.org>.
gomudayya commented on code in PR #22001:
URL: https://github.com/apache/kafka/pull/22001#discussion_r3107165128
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -184,13 +184,13 @@ public ConnectorStateInfo getConnectorStatus(final @PathParam("connector") Strin
@GET
@Path("/{connector}/topics")
@Operation(summary = "Get the list of topics actively used by the specified connector")
- public Response getConnectorActiveTopics(final @PathParam("connector") String connector) {
+ public Map<String, ActiveTopicsInfo> getConnectorActiveTopics(final @PathParam("connector") String connector) {
Review Comment:
Changing to an entity would alter the response structure, which requires a KIP and is outside the scope of this ticket.
Using `@JsonValue` could wrap it in an entity while preserving the response format, but `@JsonValue` isn't widely known and could be confusing for future readers, and it's essentially just a wrapper around Map, so the benefit seems marginal.
--
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-20391: Improve types returned by the Connect REST API [kafka]
Posted by "gomudayya (via GitHub)" <gi...@apache.org>.
gomudayya commented on code in PR #22001:
URL: https://github.com/apache/kafka/pull/22001#discussion_r4232159911
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -234,15 +234,15 @@ public Response putConnectorConfig(final @PathParam("connector") String connecto
@PATCH
@Path("/{connector}/config")
- public Response patchConnectorConfig(final @PathParam("connector") String connector,
+ public ConnectorInfo patchConnectorConfig(final @PathParam("connector") String connector,
Review Comment:
Added the `@Operation` annotation, with the summary and description based on the [Connect user guide](https://github.com/apache/kafka/blob/trunk/docs/kafka-connect/user-guide.md?plain=1#L300).
--
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