KREST-3009 Add ability to update partition count on a topic #1084

ehumber · 2022-11-22T11:56:45Z

Call looks like

curl -X PATCH http://localhost:8082/v3/clusters/3W-2oFuqSYuErBzrRrTKyw/topics/bob  -H "Content-Type: application/json" -d '{"partition_count":"10"}'  | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   688    0   664  100    24   6036    218 --:--:-- --:--:-- --:--:--  6254
{
  "kind": "KafkaTopic",
  "metadata": {
    "self": "http://localhost:8082/v3/clusters/3W-2oFuqSYuErBzrRrTKyw/topics/bob",
    "resource_name": "crn:///kafka=3W-2oFuqSYuErBzrRrTKyw/topic=bob"
  },
  "cluster_id": "3W-2oFuqSYuErBzrRrTKyw",
  "topic_name": "bob",
  "is_internal": false,
  "replication_factor": 1,
  "partitions_count": 10,
  "partitions": {
    "related": "http://localhost:8082/v3/clusters/3W-2oFuqSYuErBzrRrTKyw/topics/bob/partitions"
  },
  "configs": {
    "related": "http://localhost:8082/v3/clusters/3W-2oFuqSYuErBzrRrTKyw/topics/bob/configs"
  },
  "partition_reassignments": {
    "related": "http://localhost:8082/v3/clusters/3W-2oFuqSYuErBzrRrTKyw/topics/bob/partitions/-/reassignment"
  },
  "authorized_operations": []
}

Error messages are shown as returned by kafka

{"error_code":40002,"message":"Topic already has 2 partitions."}%  
{"error_code":40002,"message":"Topic currently has 2 partitions, which is higher than the requested 1."}%

I don't think we have a PATCH call anywhere else in the code. I think it's right here - we are modifying but not fully replacing an existing topic object's information.

The flip of that could be that it should be a PUT as we are fully and idempotently replacing the whole of the topic's partition_count object, but patch felt more future safe, in case we add other things to the request body in future?

msn-tldr · 2022-11-22T13:29:18Z

I don't think we have a PATCH call anywhere else in the code. I think it's right here - we are modifying but not fully replacing an existing topic object's information.

This does make sense.

msn-tldr

Minor nits.

api/v3/openapi.yaml

kafka-rest/src/main/java/io/confluent/kafkarest/resources/v3/TopicsResource.java

kafka-rest/src/test/java/io/confluent/kafkarest/controllers/TopicManagerImplTest.java

msn-tldr · 2022-11-22T13:20:06Z

kafka-rest/src/test/java/io/confluent/kafkarest/controllers/TopicManagerImplTest.java

@@ -451,6 +454,8 @@ public class TopicManagerImplTest {

  @Mock private DeleteTopicsResult deleteTopicsResult;

+  @Mock CreatePartitionsResult createPartitionsResult;


should this be private?

dimitarndimitrov

+1.
I think we should add a couple of integration tests and we'll be golden.

dimitarndimitrov · 2022-11-22T14:54:56Z

api/v3/openapi.yaml

+      summary: 'Update partition count'
+      operationId: updatePartitionCountKafkaV3Topic
+      description: |-
+        [![Generally Available](https://img.shields.io/badge/Lifecycle%20Stage-Generally%20Available-%2345c6e8)](#section/Versioning/API-Lifecycle-Policy)


I'd suggest starting with a non-GA status initially, unless there are clear reasons why this should be immediately considered GA.

@axisc Thoughts? I think this is simple enough to go straight to GA, but it's safer to EA first. Do Terraform's requirements have an impact on this decision?

dimitarndimitrov · 2022-11-22T14:56:02Z

api/v3/openapi.yaml

@@ -1112,6 +1135,7 @@ paths:
          $ref: '#/components/responses/TooManyRequestsErrorResponse'
        '5XX':
          $ref: '#/components/responses/ServerErrorResponse'
+


nit: Redundant blank line.

dimitarndimitrov · 2022-11-22T14:59:57Z

api/v3/openapi.yaml

+      properties:
+        partition_count:
+          type: integer


nit: Specify that this should be a 32-bit int with format: int32.

dimitarndimitrov · 2022-11-22T15:10:18Z

kafka-rest/src/main/java/io/confluent/kafkarest/resources/v3/TopicsResource.java

+                    topicManager
+                        .get()
+                        .getTopic(clusterId, topicName)
+                        .thenApply(topic -> topic.orElseThrow(NotFoundException::new))


Is this practically guaranteed not to throw if the update operation succeeded? It should still stay there to address edge cases like racing delete and update, I just want to understand the failure path.

Yeah, I did go through that thought process, it's borrowed straight from the get topic code but I thought to leave it matching. But you're right, there's only a really tiny timing window where the topic could be deleted. Overkill though, so happy to take it off.

kafka-rest/src/main/java/io/confluent/kafkarest/resources/v3/TopicsResource.java

ehumber · 2022-11-22T17:49:03Z

+1. I think we should add a couple of integration tests and we'll be golden.

Yup, totally agree. Half written but fighting back atm. We may still need this PR in in a hurry, so will do it as a follow up.

Squashed commit so I can cherry pick more easily to the cloud release branch

ehumber requested a review from a team as a code owner November 22, 2022 11:56

ehumber requested review from msn-tldr, dimitarndimitrov and TemirAskhat November 22, 2022 11:57

msn-tldr reviewed Nov 22, 2022

View reviewed changes

msn-tldr approved these changes Nov 22, 2022

View reviewed changes

dimitarndimitrov approved these changes Nov 22, 2022

View reviewed changes

ehumber requested review from dimitarndimitrov and msn-tldr November 23, 2022 06:45

ehumber force-pushed the KREST-increase-partitions branch 5 times, most recently from 6c4eb63 to 8f94202 Compare November 29, 2022 14:54

KREST-3009 Add the ability to increase num.partitions on a topic

eba75b9

Squashed commit so I can cherry pick more easily to the cloud release branch

ehumber force-pushed the KREST-increase-partitions branch from 8f94202 to eba75b9 Compare November 29, 2022 14:58

KREST-3009 address Dimitar's review comments

7dd407c

ehumber merged commit d9c6b78 into confluentinc:master Nov 30, 2022

ehumber deleted the KREST-increase-partitions branch November 30, 2022 11:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KREST-3009 Add ability to update partition count on a topic #1084

KREST-3009 Add ability to update partition count on a topic #1084

ehumber commented Nov 22, 2022 •

edited

msn-tldr commented Nov 22, 2022

msn-tldr left a comment

msn-tldr Nov 22, 2022

dimitarndimitrov left a comment

dimitarndimitrov Nov 22, 2022

ehumber Nov 22, 2022

dimitarndimitrov Nov 22, 2022

dimitarndimitrov Nov 22, 2022

dimitarndimitrov Nov 22, 2022

ehumber Nov 22, 2022

ehumber commented Nov 22, 2022

		@@ -451,6 +454,8 @@ public class TopicManagerImplTest {

		@Mock private DeleteTopicsResult deleteTopicsResult;

		@Mock CreatePartitionsResult createPartitionsResult;

KREST-3009 Add ability to update partition count on a topic #1084

KREST-3009 Add ability to update partition count on a topic #1084

Conversation

ehumber commented Nov 22, 2022 • edited

msn-tldr commented Nov 22, 2022

msn-tldr left a comment

Choose a reason for hiding this comment

msn-tldr Nov 22, 2022

Choose a reason for hiding this comment

dimitarndimitrov left a comment

Choose a reason for hiding this comment

dimitarndimitrov Nov 22, 2022

Choose a reason for hiding this comment

ehumber Nov 22, 2022

Choose a reason for hiding this comment

dimitarndimitrov Nov 22, 2022

Choose a reason for hiding this comment

dimitarndimitrov Nov 22, 2022

Choose a reason for hiding this comment

dimitarndimitrov Nov 22, 2022

Choose a reason for hiding this comment

ehumber Nov 22, 2022

Choose a reason for hiding this comment

ehumber commented Nov 22, 2022

ehumber commented Nov 22, 2022 •

edited