MINOR: Filter out quota configs for ConfigCommand using --bootstrap-server by rajinisivaram · Pull Request #9030 · apache/kafka

rajinisivaram · 2020-07-16T11:51:55Z

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

…erver

dajac

Thanks for the PR. I just left one small suggestion.

dajac · 2020-07-16T12:22:47Z


 public class RequestResponseTest {

+    private UnknownServerException unknownServerException = new UnknownServerException("secret");


nit: I would add a small comment to explain why we do this here.

@dajac Thanks for the review, added comment.

rondagostino

LGTM, left one question.

rondagostino · 2020-07-16T13:38:54Z

    }

    public DescribeClientQuotasResponse(int throttleTimeMs, Throwable e) {
+        ApiError apiError = ApiError.fromThrowable(e);


Should the same thing happen in public AlterClientQuotasResponse(Collection<ClientQuotaEntity> entities, int throttleTimeMs, Throwable e) for consistency? And also maybe in IncrementalAlterConfigsResponse (public static IncrementalAlterConfigsResponseData toResponseData)?

@rondagostino Thanks for the review. Updated AlterClientQuotasResponse. I think IncrementalAlterConfigsResponse is ok since it uses ApiError, we create the error with the right message when converting from Throwable. Can you just verify that the method with ApiError was the one you meant? Thanks,

rondagostino

LGTM!

omkreddy

@rajinisivaram Thanks for the PR. LGTM.

rajinisivaram · 2020-07-17T07:54:29Z

@dajac @rondagostino @omkreddy Thanks for the review, merging to trunk, 2.6 and 2.5.

…erver (#9030) Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>, David Jacot <djacot@confluent.io>, Ron Dagostino <rdagostino@confluent.io>

* MINOR: Filter out quota configs for ConfigCommand using --bootstrap-server (apache#9030) Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>, David Jacot <djacot@confluent.io>, Ron Dagostino <rdagostino@confluent.io> * MINOR: Fix flaky system test assertion after static member fencing (apache#9033) The test case `OffsetValidationTest.test_fencing_static_consumer` fails periodically due to this error: ``` Traceback (most recent call last): File "/home/jenkins/workspace/system-test-kafka_2.6/kafka/venv/lib/python2.7/site-packages/ducktape-0.7.8-py2.7.egg/ducktape/tests/runner_client.py", line 134, in run data = self.run_test() File "/home/jenkins/workspace/system-test-kafka_2.6/kafka/venv/lib/python2.7/site-packages/ducktape-0.7.8-py2.7.egg/ducktape/tests/runner_client.py", line 192, in run_test return self.test_context.function(self.test) File "/home/jenkins/workspace/system-test-kafka_2.6/kafka/venv/lib/python2.7/site-packages/ducktape-0.7.8-py2.7.egg/ducktape/mark/_mark.py", line 429, in wrapper return functools.partial(f, *args, **kwargs)(*w_args, **w_kwargs) File "/home/jenkins/workspace/system-test-kafka_2.6/kafka/tests/kafkatest/tests/client/consumer_test.py", line 257, in test_fencing_static_consumer assert len(consumer.dead_nodes()) == num_conflict_consumers AssertionError ``` When a consumer stops, there is some latency between when the shutdown is observed by the service and when the node is added to the dead nodes. This patch fixes the problem by giving some time for the assertion to be satisfied. Reviewers: Boyang Chen <boyang@confluent.io> * MINOR: Improve log4j for per-consumer assignment (apache#8997) Add log4j entry summarizing the assignment (previous owned and assigned) at the consumer level. Reviewers: A. Sophie Blee-Goldman <sophie@confluent.io>, Boyang Chen <boyang@confluent.io> * KAFKA-10223; Use NOT_LEADER_OR_FOLLOWER instead of non-retriable REPLICA_NOT_AVAILABLE for consumers (apache#8979) Brokers currently return NOT_LEADER_FOR_PARTITION to producers and REPLICA_NOT_AVAILABLE to consumers if a replica is not available on the broker during reassignments. Non-Java clients treat REPLICA_NOT_AVAILABLE as a non-retriable exception, Java consumers handle this error by explicitly matching the error code even though it is not an InvalidMetadataException. This PR renames NOT_LEADER_FOR_PARTITION to NOT_LEADER_OR_FOLLOWER and uses the same error for producers and consumers. This is compatible with both Java and non-Java clients since all clients handle this error code (6) as retriable exception. The PR also makes ReplicaNotAvailableException a subclass of InvalidMetadataException. - ALTER_REPLICA_LOG_DIRS continues to return REPLICA_NOT_AVAILABLE. Retained this for compatibility since this request never returned NOT_LEADER_FOR_PARTITION earlier. - MetadataRequest version 0 also returns REPLICA_NOT_AVAILABLE as topic-level error code for compatibility. Newer versions filter these out and return Errors.NONE, so didn't change this. - Partition responses in MetadataRequest return REPLICA_NOT_AVAILABLE to indicate that one of the replicas is not available. Did not change this since NOT_LEADER_FOR_PARTITION is not suitable in this case. Reviewers: Ismael Juma <ismael@juma.me.uk>, Jason Gustafson <jason@confluent.io>, Bob Barrett <bob.barrett@confluent.io> * MINOR: Improved code quality for various files. (apache#9037) Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com> * MINOR: Fixed some resource leaks. (apache#8922) Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com> * MINOR: Enable broker/client compatibility tests for 2.5.0 release - Add missing broker/client compatibility tests for 2.5.0 release Author: Manikumar Reddy <manikumar.reddy@gmail.com> Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com> Closes apache#9041 from omkreddy/compat * KAFKA-10286: Connect system tests should wait for workers to join group (apache#9040) Currently, the system tests `connect_distributed_test` and `connect_rest_test` only wait for the REST api to come up. The startup of the worker includes an asynchronous process for joining the worker group and syncing with other workers. There are some situations in which this sync takes an unusually long time, and the test continues without all workers up. This leads to flakey test failures, as worker joins are not given sufficient time to timeout and retry without waiting explicitly. This changes the `ConnectDistributedTest` to wait for the Joined group message to be printed to the logs before continuing with tests. I've activated this behavior by default, as it's a superset of the checks that were performed by default before. This log message is present in every version of DistributedHerder that I could find, in slightly different forms, but always with `Joined group` at the beginning of the log message. This change should be safe to backport to any branch. Signed-off-by: Greg Harris <gregh@confluent.io> Author: Greg Harris <gregh@confluent.io> Reviewer: Randall Hauch <rhauch@gmail.com> * KAFKA-10295: Wait for connector recovery in test_bounce (apache#9043) Signed-off-by: Greg Harris <gregh@confluent.io> Co-authored-by: Rajini Sivaram <rajinisivaram@googlemail.com> Co-authored-by: Jason Gustafson <jason@confluent.io> Co-authored-by: Guozhang Wang <wangguoz@gmail.com> Co-authored-by: Leonard Ge <62600326+leonardge@users.noreply.github.com> Co-authored-by: Manikumar Reddy <manikumar.reddy@gmail.com> Co-authored-by: Greg Harris <gregh@confluent.io>

MINOR: Filter out quota configs for ConfigCommand using --bootstrap-s…

1acc25d

…erver

dajac reviewed Jul 16, 2020

View reviewed changes

rondagostino reviewed Jul 16, 2020

View reviewed changes

Address review comments

c150a16

rajinisivaram requested a review from omkreddy July 16, 2020 18:04

rondagostino reviewed Jul 16, 2020

View reviewed changes

omkreddy approved these changes Jul 17, 2020

View reviewed changes

rajinisivaram merged commit 99c6482 into apache:trunk Jul 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINOR: Filter out quota configs for ConfigCommand using --bootstrap-server#9030

MINOR: Filter out quota configs for ConfigCommand using --bootstrap-server#9030
rajinisivaram merged 2 commits intoapache:trunkfrom
rajinisivaram:MINOR-quota-api

rajinisivaram commented Jul 16, 2020

Uh oh!

dajac left a comment

Uh oh!

dajac Jul 16, 2020

Uh oh!

rajinisivaram Jul 16, 2020

Uh oh!

rondagostino left a comment

Uh oh!

rondagostino Jul 16, 2020

Uh oh!

rajinisivaram Jul 16, 2020

Uh oh!

rondagostino left a comment

Uh oh!

omkreddy left a comment

Uh oh!

rajinisivaram commented Jul 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		public class RequestResponseTest {

		private UnknownServerException unknownServerException = new UnknownServerException("secret");

Conversation

rajinisivaram commented Jul 16, 2020

Committer Checklist (excluded from commit message)

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

dajac Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

rajinisivaram Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

rondagostino left a comment

Choose a reason for hiding this comment

Uh oh!

rondagostino Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

rajinisivaram Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

rondagostino left a comment

Choose a reason for hiding this comment

Uh oh!

omkreddy left a comment

Choose a reason for hiding this comment

Uh oh!

rajinisivaram commented Jul 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants