KAFKA-8988. Replace CreatePartitions Request/Response with automated protocol #7493
Conversation
|
CC: @ijuma, @mumrah, @hachikuji, @cmccabe, @jsancio |
soondenana
force-pushed
the
CreatePartitionsAutotmatedProtocol
branch
from
06fc308
to
60dde86
Compare
soondenana
force-pushed
the
CreatePartitionsAutotmatedProtocol
branch
from
60dde86
to
d312439
Compare
|
SaslSslAdminClientIntegrationTest.testCreatePartitions failed for JDK 8 Ran them locally (with JDK 8) to make sure that they pass. |
clients/src/main/java/org/apache/kafka/common/requests/CreatePartitionsRequest.java
Outdated
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/common/requests/CreatePartitionsRequest.java
Outdated
Show resolved
Hide resolved
| return new CreatePartitionsRequest.Builder(requestMap, timeoutMs, options.validateOnly()); | ||
| CreatePartitionsRequestData requestData = new CreatePartitionsRequestData() | ||
| .setTopics(topics) | ||
| .setValidateOnly(options.validateOnly()) |
There was a problem hiding this comment.
Leaving this here for lack of an alternative. I was looking at the defaults used in the schema definitions. We do not override them in any cases, but I wonder if it would make sense in the case of validateOnly. If we do not override, the default would be false, but maybe true is a safer value?
Another thing I was looking at is how we handle the ErrorMessage field in the response. The previous serialization logic used ApiError which attempts to avoid serialization when the error message matches the standard value. I suspect that we might have broken this optimization in other cases as well, though I'm not sure how much it matters. Also, the old code used null as the default instead of the empty string. Perhaps we should do the same?
There was a problem hiding this comment.
I agree that its safer to have validateOnly default to true, but I think the common case is to have it false. If I am user of this api, I will assume the api call to create partitions by default. And if there is a need for validation only, only then I will look deeper into api to find a flag for doing that. So in that sense it matches user expectation.
Interesting comment about the ErrorMessage. But when creating CreatePartitionsResponse in CreatePartitionsRequest::getErrorResponse, we use apiError.message to populate the error message, and the method returns null if the message is default. It seems if we use apiError.messageWithFallback then we will get into situation that you are describing. I may be missing something here.
Updated json to set null for ErrorMessage field. Thanks for pointing that out.
soondenana
force-pushed
the
CreatePartitionsAutotmatedProtocol
branch
from
d312439
to
36d8092
Compare
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
Outdated
Show resolved
Hide resolved
…protocol This change update the CreatePartitions request and response api objects to use automated protocol. The change involved updating all client code to use the newly updated CreatePartitionsRequest and CreatePartitionsResponse classes. Updated relevant tests. All tests pass.
The commit contain three changes: 1. Rename "integers" variable to "brokerIds" 2. Take out "()" from method calls as per coding convention 3. Refactor AdminManager::createPartitions to get rid of chained maps.
soondenana
force-pushed
the
CreatePartitionsAutotmatedProtocol
branch
from
721dc10
to
6d469f0
Compare
|
retest this please |
Empty reassignment was treated as error condition before, filtering it out now ignores this case and creates new partiton using default assignment. This change removes that filter, so that the error condition is triggered again and `InvalidReplicaAssignmentException` gets thrown.
|
|
|
retest this please |
2 similar comments
|
retest this please |
|
retest this please |
|
On 2.12 the |
This change update the CreatePartitions request and response api objects
to use automated protocol. The change involved updating all client code
to use the newly updated CreatePartitionsRequest and
CreatePartitionsResponse classes.
Updated relevant tests. All tests pass.
More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.
Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.
Committer Checklist (excluded from commit message)