C0urante commented on code in PR #13915:
URL: https://github.com/apache/kafka/pull/13915#discussion_r1242245895
##########
docs/connect.html:
##########
@@ -301,7 +301,7 @@ <h4><a id="connect_rest" href="#connect_rest">REST
API</a></h4>
<li><code>GET /connectors/{name}/tasks</code> - get a list of tasks
currently running for a connector</li>
<li><code>GET /connectors/{name}/tasks/{taskid}/status</code> - get
current status of the task, including if it is running, failed, paused, etc.,
which worker it is assigned to, and error information if it has failed</li>
<li><code>PUT /connectors/{name}/pause</code> - pause the connector
and its tasks, which stops message processing until the connector is resumed.
Any resources claimed by its tasks are left allocated, which allows the
connector to begin processing data quickly once it is resumed.</li>
- <li><code>PUT /connectors/{name}/stop</code> - stop the connector and
shut down its tasks, deallocating any resources claimed by its tasks. This is
more efficient from a resource usage standpoint than pausing the connector, but
can cause it to take longer to begin processing data once resumed.</li>
+ <li><code>PUT /connectors/{name}/stop</code> - stop the connector and
shut down its tasks, deallocating any resources claimed by its tasks. This is
more efficient from a resource usage standpoint than pausing the connector, but
can cause it to take longer to begin processing data once resumed. Note that
the offsets for a connector can be modified via the offsets management REST
APIs only if it is in the stopped state.</li>
Review Comment:
```suggestion
<li><code>PUT /connectors/{name}/stop</code> - stop the connector
and shut down its tasks, deallocating any resources claimed by its tasks. This
is more efficient from a resource usage standpoint than pausing the connector,
but can cause it to take longer to begin processing data once resumed. Note
that the offsets for a connector can be only modified via the offsets
management endpoints if it is in the stopped state.</li>
```
##########
docs/connect.html:
##########
@@ -313,7 +313,13 @@ <h4><a id="connect_rest" href="#connect_rest">REST
API</a></h4>
<li><code>DELETE /connectors/{name}</code> - delete a connector,
halting all tasks and deleting its configuration</li>
<li><code>GET /connectors/{name}/topics</code> - get the set of topics
that a specific connector is using since the connector was created or since a
request to reset its set of active topics was issued</li>
<li><code>PUT /connectors/{name}/topics/reset</code> - send a request
to empty the set of active topics of a connector</li>
- <li><code>GET /connectors/{name}/offsets</code> - get the current
offsets for a connector (see <a
href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-875%3A+First-class+offsets+support+in+Kafka+Connect";>KIP-875</a>
for more details)</li>
+ <li>Offsets management REST APIs (see <a
href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-875%3A+First-class+offsets+support+in+Kafka+Connect";>KIP-875</a>
for more details):
+ <ul>
+ <li><code>GET /connectors/{name}/offsets</code> - get the
current offsets for a connector</li>
+ <li><code>DELETE /connectors/{name}/offsets</code> - reset the
offsets for a connector. The connector must exist and must be in the stopped
state.</li>
Review Comment:
Nit: for consistency, we should leave out the period from the last sentence
of each item
```suggestion
<li><code>DELETE /connectors/{name}/offsets</code> - reset
the offsets for a connector. The connector must exist and must be in the
stopped state</li>
```
Also, would it be possible to link to the docs for the `PUT
/connectors/{name}/stop` endpoint when we refer to it here?
##########
docs/connect.html:
##########
@@ -313,7 +313,13 @@ <h4><a id="connect_rest" href="#connect_rest">REST
API</a></h4>
<li><code>DELETE /connectors/{name}</code> - delete a connector,
halting all tasks and deleting its configuration</li>
<li><code>GET /connectors/{name}/topics</code> - get the set of topics
that a specific connector is using since the connector was created or since a
request to reset its set of active topics was issued</li>
<li><code>PUT /connectors/{name}/topics/reset</code> - send a request
to empty the set of active topics of a connector</li>
- <li><code>GET /connectors/{name}/offsets</code> - get the current
offsets for a connector (see <a
href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-875%3A+First-class+offsets+support+in+Kafka+Connect";>KIP-875</a>
for more details)</li>
+ <li>Offsets management REST APIs (see <a
href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-875%3A+First-class+offsets+support+in+Kafka+Connect";>KIP-875</a>
for more details):
+ <ul>
+ <li><code>GET /connectors/{name}/offsets</code> - get the
current offsets for a connector</li>
+ <li><code>DELETE /connectors/{name}/offsets</code> - reset the
offsets for a connector. The connector must exist and must be in the stopped
state.</li>
+ <li><code>PATCH /connectors/{name}/offsets</code> - alter the
offsets for a connector. The connector must exist and must be in the stopped
state. The request body should be a JSON object containing a JSON array
<code>offsets</code> field, similar to the response body of the <code>GET
/connectors/{name}/offsets</code> REST API.</li>
Review Comment:
Same nit RE trailing period, and same question about linking to the docs for
the `PUT /connectors/{name}/stop` endpoint:
```suggestion
<li><code>PATCH /connectors/{name}/offsets</code> - alter
the offsets for a connector. The connector must exist and must be in the
stopped state. The request body should be a JSON object containing a JSON array
<code>offsets</code> field, similar to the response body of the <code>GET
/connectors/{name}/offsets</code> endpoint</li>
```
##########
docs/connect.html:
##########
@@ -313,7 +313,13 @@ <h4><a id="connect_rest" href="#connect_rest">REST
API</a></h4>
<li><code>DELETE /connectors/{name}</code> - delete a connector,
halting all tasks and deleting its configuration</li>
<li><code>GET /connectors/{name}/topics</code> - get the set of topics
that a specific connector is using since the connector was created or since a
request to reset its set of active topics was issued</li>
<li><code>PUT /connectors/{name}/topics/reset</code> - send a request
to empty the set of active topics of a connector</li>
- <li><code>GET /connectors/{name}/offsets</code> - get the current
offsets for a connector (see <a
href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-875%3A+First-class+offsets+support+in+Kafka+Connect";>KIP-875</a>
for more details)</li>
+ <li>Offsets management REST APIs (see <a
href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-875%3A+First-class+offsets+support+in+Kafka+Connect";>KIP-875</a>
for more details):
+ <ul>
+ <li><code>GET /connectors/{name}/offsets</code> - get the
current offsets for a connector</li>
+ <li><code>DELETE /connectors/{name}/offsets</code> - reset the
offsets for a connector. The connector must exist and must be in the stopped
state.</li>
+ <li><code>PATCH /connectors/{name}/offsets</code> - alter the
offsets for a connector. The connector must exist and must be in the stopped
state. The request body should be a JSON object containing a JSON array
<code>offsets</code> field, similar to the response body of the <code>GET
/connectors/{name}/offsets</code> REST API.</li>
Review Comment:
Also, do you think we can include either a link to the generated OpenAPI
docs for this endpoint, or a fleshed-out example of a request for this endpoint?
I like that we're calling out the symmetry between the request format for
this endpoint and the response format for the `GET /connectors/{name}/offsets`
endpoint, but I suspect it may be easier on users if we also provide examples
for altering the offsets of a sink and source connector (including some null
partitions to denote resets).
Possibly creeping out of scope for this PR, but we might have an additional
docs section that goes into detail on managing offsets with connectors
(including example requests/responses) and then link to that section from each
of these list items, instead of trying to put all of the information for these
endpoints into this section. This is definitely not a blocker, though.
##########
docs/connect.html:
##########
@@ -313,7 +313,13 @@ <h4><a id="connect_rest" href="#connect_rest">REST
API</a></h4>
<li><code>DELETE /connectors/{name}</code> - delete a connector,
halting all tasks and deleting its configuration</li>
<li><code>GET /connectors/{name}/topics</code> - get the set of topics
that a specific connector is using since the connector was created or since a
request to reset its set of active topics was issued</li>
<li><code>PUT /connectors/{name}/topics/reset</code> - send a request
to empty the set of active topics of a connector</li>
- <li><code>GET /connectors/{name}/offsets</code> - get the current
offsets for a connector (see <a
href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-875%3A+First-class+offsets+support+in+Kafka+Connect";>KIP-875</a>
for more details)</li>
+ <li>Offsets management REST APIs (see <a
href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-875%3A+First-class+offsets+support+in+Kafka+Connect";>KIP-875</a>
for more details):
Review Comment:
Nit:
```suggestion
<li>Offsets management endpoints (see <a
href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-875%3A+First-class+offsets+support+in+Kafka+Connect";>KIP-875</a>
for more details):
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
