You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/08/16 14:17:46 UTC
[GitHub] [kafka] mimaison commented on a diff in pull request #12306: KAFKA-13976: Improvements for OpenAPI specs
mimaison commented on code in PR #12306:
URL: https://github.com/apache/kafka/pull/12306#discussion_r946792078
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorPluginsResource.java:
##########
@@ -112,7 +119,25 @@ public void requestTimeout(long requestTimeoutMs) {
@PUT
@Path("/{pluginName}/config/validate")
- @Operation(summary = "Validate the provided configuration against the configuration definition for the specified pluginName")
+ @Operation(
+ summary = "Validate the provided configuration against the configuration definition for the specified pluginName",
+ requestBody = @RequestBody(
+ required = true,
+ content = @Content(
+ mediaType = "application/json",
+ schema = @Schema(implementation = MapSchema.class),
Review Comment:
This generates a ton of lines in the spec file which I'm not sure totally make sense. Have you tried using the spec file to generate code?
Same for all the other places where you put `MapSchema`.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -107,7 +117,23 @@ public void requestTimeout(long requestTimeoutMs) {
@GET
@Path("/")
- @Operation(summary = "List all active connectors")
+ @Operation(
+ summary = "List all active connectors",
+ parameters = @Parameter(
Review Comment:
Could we document this query parameter like we've done for `connectorsOnly`? See https://github.com/apache/kafka/blob/trunk/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorPluginsResource.java#L148
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -179,7 +237,27 @@ public ConnectorInfo getConnector(final @PathParam("connector") String connector
@GET
@Path("/{connector}/config")
- @Operation(summary = "Get the configuration for the specified connector")
+ @Operation(
+ summary = "Get the configuration for the specified connector",
+ parameters = {
+ @Parameter(in = ParameterIn.QUERY, hidden = true, schema = @Schema(implementation = BooleanSchema.class), name = "forward")
+ }
+ )
+ @ApiResponses(value = {
+ @ApiResponse(
+ responseCode = "200",
+ description = "Connector config details retrieved successfully",
Review Comment:
Can we use `Connector configuration retrieved successfully`?
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -145,7 +171,26 @@ public Response listConnectors(
@POST
@Path("/")
- @Operation(summary = "Create a new connector")
+ @Operation(
+ summary = "Create a new connector",
+ requestBody = @RequestBody(
+ content = @Content(mediaType = "application/json", schema = @Schema(implementation = CreateConnectorRequest.class)),
+ required = true,
+ description = "Name and configuration parameters of the new connector. All values in config should be strings."
+ ),
+ parameters = @Parameter(in = ParameterIn.QUERY, hidden = true, schema = @Schema(implementation = BooleanSchema.class))
Review Comment:
Do we need this? I'm assuming it's about the `forward` query parameter but this does not even include the name! Also `forward` is already documented below.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -260,7 +417,42 @@ public Response putConnectorConfig(final @PathParam("connector") String connecto
@POST
@Path("/{connector}/restart")
- @Operation(summary = "Restart the specified connector")
+ @Operation(
+ summary = "Restart the specified connector",
+ parameters = {
+ @Parameter(in = ParameterIn.QUERY, hidden = true, schema = @Schema(implementation = BooleanSchema.class), name = "forward")
+ }
+ )
+ @ApiResponses(value = {
+ @ApiResponse(
+ responseCode = "200",
+ description = "Named connector exists and the server has successfully stopped and begun restarting only the Connector object"
+ ),
+ @ApiResponse(
+ responseCode = "202",
+ description = "Named connector exists and the server has successfully and durably recorded the request to stop and " +
+ "begin restarting at least one failed or running Connector object and Task instances",
+ content = @Content(mediaType = "application/json", schema = @Schema(implementation = ConnectorStateInfo.class))
+ ),
+ @ApiResponse(
+ responseCode = "204",
+ description = "Operation succeeded, but there is no content in the response"
+ ),
+ @ApiResponse(
+ responseCode = "404",
+ description = "Named connector does not exist"
+ ),
+ @ApiResponse(
+ responseCode = "409",
+ description = "A rebalance is needed, forthcoming, or underway while restarting any of the Connector and/or Task objects"
Review Comment:
We should use original descriptions rather than copying from the Confluent docs!
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -237,7 +371,30 @@ public Response resetConnectorActiveTopics(final @PathParam("connector") String
@PUT
@Path("/{connector}/config")
- @Operation(summary = "Create or reconfigure the specified connector")
+ @Operation(
+ summary = "Create or reconfigure the specified connector",
+ parameters = {
+ @Parameter(in = ParameterIn.QUERY, hidden = true, schema = @Schema(implementation = BooleanSchema.class), name = "forward")
+ },
+ requestBody = @RequestBody(
+ content = @Content(
+ mediaType = "application/json",
+ schema = @Schema(implementation = MapSchema.class),
+ additionalPropertiesSchema = @Schema(type = "string")
+ )
+ )
+ )
+ @ApiResponses(value = {
+ @ApiResponse(
+ responseCode = "201",
Review Comment:
Is it 201 or 200?
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -345,6 +565,14 @@ public void fenceZombies(final @PathParam("connector") String connector,
@GET
@Path("/{connector}/tasks/{task}/status")
@Operation(summary = "Get the state of the specified task for the specified connector")
+ @ApiResponses(value = {
+ @ApiResponse(
+ responseCode = "200",
+ content = @Content(mediaType = "application/json", array = @ArraySchema(schema = @Schema(implementation = ConnectorStateInfo.TaskState.class))),
+ description = "Task status fetched successfully for specified connector"
Review Comment:
We've used `retrieved` everywhere so let's stick to that instead of introducing `fetched` here. We can also drop the `for specified connector` ending.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -260,7 +417,42 @@ public Response putConnectorConfig(final @PathParam("connector") String connecto
@POST
@Path("/{connector}/restart")
- @Operation(summary = "Restart the specified connector")
+ @Operation(
+ summary = "Restart the specified connector",
+ parameters = {
+ @Parameter(in = ParameterIn.QUERY, hidden = true, schema = @Schema(implementation = BooleanSchema.class), name = "forward")
+ }
+ )
+ @ApiResponses(value = {
+ @ApiResponse(
+ responseCode = "200",
+ description = "Named connector exists and the server has successfully stopped and begun restarting only the Connector object"
+ ),
+ @ApiResponse(
+ responseCode = "202",
+ description = "Named connector exists and the server has successfully and durably recorded the request to stop and " +
+ "begin restarting at least one failed or running Connector object and Task instances",
+ content = @Content(mediaType = "application/json", schema = @Schema(implementation = ConnectorStateInfo.class))
+ ),
+ @ApiResponse(
+ responseCode = "204",
+ description = "Operation succeeded, but there is no content in the response"
+ ),
+ @ApiResponse(
+ responseCode = "404",
+ description = "Named connector does not exist"
+ ),
+ @ApiResponse(
+ responseCode = "409",
+ description = "A rebalance is needed, forthcoming, or underway while restarting any of the Connector and/or Task objects"
Review Comment:
If other descriptions have been copied, let's update them too
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -319,7 +531,11 @@ public List<TaskInfo> getTaskConfigs(final @PathParam("connector") String connec
@POST
@Path("/{connector}/tasks")
- @Operation(hidden = true, summary = "This operation is only for inter-worker communications")
+ @Operation(
Review Comment:
We don't need to document the request body since it's not a public API. Let's undo this change.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -188,9 +266,30 @@ public Map<String, String> getConnectorConfig(final @PathParam("connector") Stri
return completeOrForwardRequest(cb, "/connectors/" + connector + "/config", "GET", headers, null, forward);
}
+
Review Comment:
We don't need this extra line
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -366,7 +600,19 @@ public void restartTask(final @PathParam("connector") String connector,
@DELETE
@Path("/{connector}")
- @Operation(summary = "Delete the specified connector")
+ @Operation(
+ summary = "Delete the specified connector",
+ parameters = {
+ @Parameter(in = ParameterIn.QUERY, schema = @Schema(implementation = BooleanSchema.class), name = "forward", hidden = true)
+ }
+ )
+ @ApiResponses(value = {
+ @ApiResponse(responseCode = "204", description = "Connector deleted successfully"),
+ @ApiResponse(
+ responseCode = "409",
+ description = "Error while deleting new connector. Either rabalance is in progress or the connector already exists"
Review Comment:
`or the connector already exists` does not seem quite right!
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -237,7 +371,30 @@ public Response resetConnectorActiveTopics(final @PathParam("connector") String
@PUT
@Path("/{connector}/config")
- @Operation(summary = "Create or reconfigure the specified connector")
+ @Operation(
+ summary = "Create or reconfigure the specified connector",
+ parameters = {
+ @Parameter(in = ParameterIn.QUERY, hidden = true, schema = @Schema(implementation = BooleanSchema.class), name = "forward")
+ },
+ requestBody = @RequestBody(
+ content = @Content(
+ mediaType = "application/json",
+ schema = @Schema(implementation = MapSchema.class),
+ additionalPropertiesSchema = @Schema(type = "string")
+ )
+ )
+ )
+ @ApiResponses(value = {
+ @ApiResponse(
+ responseCode = "201",
+ description = "New connector created successfully",
Review Comment:
What about `Connector created or updated successfully`?
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -188,9 +266,30 @@ public Map<String, String> getConnectorConfig(final @PathParam("connector") Stri
return completeOrForwardRequest(cb, "/connectors/" + connector + "/config", "GET", headers, null, forward);
}
+
@GET
@Path("/{connector}/tasks-config")
- @Operation(summary = "Get the configuration of all tasks for the specified connector")
+ @Operation(
+ summary = "Get the configuration of all tasks for the specified connector",
+ parameters = {
+ @Parameter(in = ParameterIn.QUERY, hidden = true, schema = @Schema(implementation = BooleanSchema.class), name = "forward")
+ }
+ )
+ @ApiResponses(value = {
+ @ApiResponse(
+ responseCode = "200",
+ description = "Connector tasks config details retrieved successfully",
Review Comment:
What about `Connector tasks configuration retrieved successfully`?
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -237,7 +371,30 @@ public Response resetConnectorActiveTopics(final @PathParam("connector") String
@PUT
@Path("/{connector}/config")
- @Operation(summary = "Create or reconfigure the specified connector")
+ @Operation(
+ summary = "Create or reconfigure the specified connector",
+ parameters = {
+ @Parameter(in = ParameterIn.QUERY, hidden = true, schema = @Schema(implementation = BooleanSchema.class), name = "forward")
+ },
+ requestBody = @RequestBody(
+ content = @Content(
+ mediaType = "application/json",
+ schema = @Schema(implementation = MapSchema.class),
+ additionalPropertiesSchema = @Schema(type = "string")
+ )
+ )
+ )
+ @ApiResponses(value = {
+ @ApiResponse(
+ responseCode = "201",
+ description = "New connector created successfully",
+ content = @Content(mediaType = "application/json", schema = @Schema(implementation = ConnectorInfo.class))
+ ),
+ @ApiResponse(
+ responseCode = "409",
+ description = "Error while creating new connector. Either rabalance is in progress or the connector already exists"
Review Comment:
This message is not correct. Let's use `Error while creating or updating connector. A rebalance is in progress`.
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -299,15 +494,32 @@ public Response pauseConnector(@PathParam("connector") String connector, final @
@PUT
@Path("/{connector}/resume")
@Operation(summary = "Resume the specified connector",
- description = "This operation is idempotent and has no effects if the connector is already running")
+ description = "This operation is idempotent and has no effects if the connector is already running")
+ @ApiResponses(value = {
+ @ApiResponse(responseCode = "202", description = "Connector resume request accepted"),
+ @ApiResponse(responseCode = "404", description = "Named connector doesn't exist")
+ })
public Response resumeConnector(@PathParam("connector") String connector) {
herder.resumeConnector(connector);
return Response.accepted().build();
}
@GET
@Path("/{connector}/tasks")
- @Operation(summary = "List all tasks for the specified connector")
+ @Operation(
+ summary = "List all tasks for the specified connector",
+ parameters = {
+ @Parameter(in = ParameterIn.QUERY, hidden = true, schema = @Schema(implementation = BooleanSchema.class), name = "forward")
+ }
+ )
+ @ApiResponses(value = {
+ @ApiResponse(
+ responseCode = "200",
+ content = @Content(mediaType = "application/json", array = @ArraySchema(schema = @Schema(implementation = TaskInfo.class))),
+ description = "Connector resume request accepted"
Review Comment:
That's not the right description
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -145,7 +171,26 @@ public Response listConnectors(
@POST
@Path("/")
- @Operation(summary = "Create a new connector")
+ @Operation(
+ summary = "Create a new connector",
+ requestBody = @RequestBody(
+ content = @Content(mediaType = "application/json", schema = @Schema(implementation = CreateConnectorRequest.class)),
+ required = true,
+ description = "Name and configuration parameters of the new connector. All values in config should be strings."
+ ),
+ parameters = @Parameter(in = ParameterIn.QUERY, hidden = true, schema = @Schema(implementation = BooleanSchema.class))
Review Comment:
Same a few more times below
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -222,6 +346,16 @@ public Response getConnectorActiveTopics(final @PathParam("connector") String co
@PUT
@Path("/{connector}/topics/reset")
@Operation(summary = "Reset the list of topics actively used by the specified connector")
+ @ApiResponses(value = {
+ @ApiResponse(
+ responseCode = "200",
+ description = "Connector status retrieved successfully",
Review Comment:
Again not the right description
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -202,14 +301,39 @@ public Map<ConnectorTaskId, Map<String, String>> getTasksConfig(
@GET
@Path("/{connector}/status")
- @Operation(summary = "Get the status for the specified connector")
+ @Operation(
+ summary = "Get the status for the specified connector"
+ )
+ @ApiResponses(value = {
+ @ApiResponse(
+ responseCode = "200",
+ description = "Connector status retrieved successfully",
+ content = @Content(mediaType = "application/json", schema = @Schema(implementation = ConnectorStateInfo.class))
+ ),
+ @ApiResponse(responseCode = "404", description = "Connector not found")
+ })
public ConnectorStateInfo getConnectorStatus(final @PathParam("connector") String connector) {
return herder.connectorStatus(connector);
}
@GET
@Path("/{connector}/topics")
@Operation(summary = "Get the list of topics actively used by the specified connector")
+ @ApiResponses(value = {
+ @ApiResponse(
+ responseCode = "200",
+ description = "Connector status retrieved successfully",
Review Comment:
It's not the right description
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -145,7 +171,26 @@ public Response listConnectors(
@POST
@Path("/")
- @Operation(summary = "Create a new connector")
+ @Operation(
+ summary = "Create a new connector",
+ requestBody = @RequestBody(
+ content = @Content(mediaType = "application/json", schema = @Schema(implementation = CreateConnectorRequest.class)),
+ required = true,
+ description = "Name and configuration parameters of the new connector. All values in config should be strings."
+ ),
+ parameters = @Parameter(in = ParameterIn.QUERY, hidden = true, schema = @Schema(implementation = BooleanSchema.class))
+ )
+ @ApiResponses(value = {
+ @ApiResponse(
+ responseCode = "201",
+ description = "New connector created successfully",
+ content = @Content(mediaType = "application/json", schema = @Schema(implementation = ConnectorInfo.class))
+ ),
+ @ApiResponse(
+ responseCode = "409",
+ description = "Error while creating new connector. Either rabalance is in progress or the connector already exists"
Review Comment:
Typo in `rebalance`. Same a few more times below.
Also can we rephrase it to: `Either a rebalance is in progress or the connector already exists`?
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java:
##########
@@ -332,7 +548,11 @@ public void putTaskConfigs(final @PathParam("connector") String connector,
@PUT
@Path("/{connector}/fence")
- @Operation(hidden = true, summary = "This operation is only for inter-worker communications")
+ @Operation(
Review Comment:
Not a public API, let's undo too
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/LoggingResource.java:
##########
@@ -128,7 +146,17 @@ public Response getLogger(final @PathParam("logger") String namedLogger) {
*/
@PUT
@Path("/{logger}")
- @Operation(summary = "Set the level for the specified logger")
+ @Operation(
+ summary = "Set the level for the specified logger",
+ requestBody = @RequestBody(
+ required = true,
+ content = @Content(
+ mediaType = "application/json",
+ schema = @Schema(implementation = MapSchema.class),
Review Comment:
Should we cover the return type too? Currently it generates
```
responses:
default:
content:
application/json: {}
description: default response
```
which is not great
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorPluginsResource.java:
##########
@@ -112,7 +119,25 @@ public void requestTimeout(long requestTimeoutMs) {
@PUT
@Path("/{pluginName}/config/validate")
- @Operation(summary = "Validate the provided configuration against the configuration definition for the specified pluginName")
+ @Operation(
+ summary = "Validate the provided configuration against the configuration definition for the specified pluginName",
+ requestBody = @RequestBody(
+ required = true,
+ content = @Content(
+ mediaType = "application/json",
+ schema = @Schema(implementation = MapSchema.class),
+ additionalPropertiesSchema = @Schema(type = "string")
+ )
+ )
+ )
+ @ApiResponses(
+ value = {
+ @ApiResponse(
+ responseCode = "201",
Review Comment:
Isn't it returning 200 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org