KAFKA-4591: Create Topic Policy follow-up#2388
Conversation
1. Added javadoc to public classes 2. Removed `s` from config name for consistency with interface name 3. The policy interface now implements Configurable and AutoCloseable as per the KIP 4. Use `null` instead of `-1` in `RequestMetadata`
|
Review by @hachikuji. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
| } else { | ||
| AdminUtils.assignReplicasToBrokers(brokers, arguments.numPartitions, arguments.replicationFactor) | ||
| else if (!arguments.replicasAssignments.isEmpty) { | ||
| // Note: we don't check that replicaAssignment doesn't contain unknown brokers - unlike in add-partitions case, |
There was a problem hiding this comment.
nit: I know it's not from this patch, but the double negatives make this a bit tough to parse. I think it's just saying that we don't bother checking for unknown brokers? By the way, is there a good reason for not doing this (other than for consistency with TopicCommand)?
There was a problem hiding this comment.
Removed the double negative. I am guessing it's this way because you may have a broker that is down temporarily, but I am not really sure.
| "if a majority of replicas do not receive a write." | ||
|
|
||
| val CreateTopicsPolicyClassNameDoc = "The create topics policy class that should be used for validation. The class should " + | ||
| val CreateTopicPolicyClassNameDoc = "The create topics policy class that should be used for validation. The class should " + |
There was a problem hiding this comment.
Do you want to change the "topics" to "topic" in the comment as well?
I didn’t mean to commit it.
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Logging changes LGTM. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
1. Added javadoc to public classes 2. Removed `s` from config name for consistency with interface name 3. The policy interface now implements Configurable and AutoCloseable as per the KIP 4. Use `null` instead of `-1` in `RequestMetadata` 5. Perform all broker validation before invoking the policy 6. Add tests Author: Ismael Juma <ismael@juma.me.uk> Reviewers: Jason Gustafson <jason@confluent.io> Closes #2388 from ijuma/create-topic-policy-docs-and-config-name-change (cherry picked from commit fd6d7bc) Signed-off-by: Ismael Juma <ismael@juma.me.uk>
1. Added javadoc to public classes 2. Removed `s` from config name for consistency with interface name 3. The policy interface now implements Configurable and AutoCloseable as per the KIP 4. Use `null` instead of `-1` in `RequestMetadata` 5. Perform all broker validation before invoking the policy 6. Add tests Author: Ismael Juma <ismael@juma.me.uk> Reviewers: Jason Gustafson <jason@confluent.io> Closes apache#2388 from ijuma/create-topic-policy-docs-and-config-name-change
3 participants
sfrom config name for consistency with interface namenullinstead of-1inRequestMetadata