KAFKA-2945: CreateTopic - protocol and server side implementation#1489
KAFKA-2945: CreateTopic - protocol and server side implementation#1489granthenke wants to merge 5 commits into
Conversation
|
This pull request supersedes #626. I opened a new pull request in order to reduce confusion based on old/outdated discussion. For the latest discussion on this KIP change please see the Kafka dev. mailing list. |
8411a89 to
1acd6b1
Compare
There was a problem hiding this comment.
To have the right concurrency semantics, you should not mutate the map after assigning it to the final field.
b631126 to
04c2e43
Compare
04c2e43 to
4e1f59a
Compare
|
Pinging for review now that the vote has passed. |
| new NotControllerException("This is not the correct controller for this cluster.")), | ||
| INVALID_REQUEST(42, | ||
| new InvalidRequestException("This most likely occurs because of a request being malformed by the client library or" + | ||
| " the message was sent to an incompatible broker. See the broker logs for more details.")); |
There was a problem hiding this comment.
Since we haven't deprecated ErrorMapping, could we add those new error codes as place holders in ErrorMapping?
There was a problem hiding this comment.
I will add it.
Note: That since only a newer client (the admin client) can actually make a request that causes this error, it should never show up in the old scala clients.
There was a problem hiding this comment.
Looking closer at ErrorMapping there are many codes left out that would never be used. Do these fall under that category? Are you sure I should add them?
For now I will add the comment like the others:
// 35: UNSUPPORTED_VERSION
// 36: TOPIC_ALREADY_EXISTS
// 37: INVALID_PARTITIONS
// 38: INVALID_REPLICATION_FACTOR
// 39: INVALID_REPLICA_ASSIGNMENT
// 40: INVALID_CONFIG
// 41: NOT_CONTROLLER
// 42: INVALID_REQUESTThere was a problem hiding this comment.
Yes, we just want to prevent someone from accidentally adding the same error code for a different exception in ErrorMapping in the future.
|
@junrao I updated the patch to address your reviews. |
|
@junrao Will you have a chance to review further? Any more changes needed? |
| val zkUtils: ZkUtils) extends Logging with KafkaMetricsGroup { | ||
| this.logIdent = "[Admin Manager on Broker " + config.brokerId + "]: " | ||
|
|
||
| val topicPurgatory = DelayedOperationPurgatory[DelayedOperation]("topic", config.brokerId) |
There was a problem hiding this comment.
"topic" seems to be an ambiguous name for this purgatory. If we expect this purgatory to be specific to topic creation, this probably should be createTopic. If we expect this to be generic to all admin tasks, this probably should be admin.
There was a problem hiding this comment.
The key for this purgatory is the topic name. Items tracked in this purgatory will all try to be completed when topic metadata is changed. This will included the delete topic requests and alter topic requests. In that sense I think topicPurgatory makes sense.
I didn't call it admin purgatory since admin requests for ACLs or configs won't leverage this purgatory.
3 participants
No description provided.