KAFKA-15831: KIP-1000 protocol and admin client by AndrewJSchofield · Pull Request #14811 · apache/kafka

AndrewJSchofield · 2023-11-21T13:34:51Z

This adds the new ListClientMetricsResources RPC to the Kafka protocol and puts support
into the Kafka admin client. The broker-side implementation in this PR is just to return an empty
list. A future PR will obtain the list from the config store.

Includes a few unit tests for what is a very simple RPC. There are additional tests already written and
waiting for the PR that delivers the kafka-client-metrics.sh tool which builds on this PR.

Committer Checklist (excluded from commit message)

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

junrao

@AndrewJSchofield : Thanks for the PR. Just a few minor comments.

junrao · 2023-11-21T23:26:45Z

  }

+  // Just a placeholder for now.
+  def handleListClientMetricsResources(request: RequestChannel.Request): CompletableFuture[Unit] = {


Why does this return CompletableFuture[Unit] instead of Unit?

Just giving the option of asynchronous completion. However, since I don't strictly need it now, I am simplifying it to Unit.

junrao · 2023-11-21T23:31:15Z

+     */
+    public KafkaFuture<Collection<ClientMetricsResourceListing>> all() {
+        final KafkaFutureImpl<Collection<ClientMetricsResourceListing>> result = new KafkaFutureImpl<>();
+        this.future.whenComplete((listings, throwable) -> {


Do we need this?

No. Removed.

junrao · 2023-11-21T23:44:08Z

+    }
+
+    @Test
+    public void testListClientMetricsResourcesNotSupported() throws Exception {


This method doesn't seem to throw.

That appears true to me too, but all of the other tests in that source file are the same pattern. I am going for consistency here.

AndrewJSchofield · 2023-11-27T22:20:37Z

Build is almost green. A small number of test failures unrelated to this PR.

apoorvmittal10

Thanks for the PR @AndrewJSchofield, LGTM. Minor comments/nit.

junrao

@AndrewJSchofield : Thanks for the updated PR. The code LGTM. Are the 33 test failures related?

junrao · 2023-11-28T17:42:50Z

Also, you could trigger a rerun of the tests by closing the PR, waiting for 20 secs and reopening it.

AndrewJSchofield · 2023-11-28T17:46:26Z

The test failures are unrelated. I will rerun the tests as requested.

AndrewJSchofield · 2023-11-28T17:46:53Z

Closing to rerun tests

junrao · 2023-11-29T15:59:15Z

@AndrewJSchofield : Are the 40 failed tests related? Could you file jiras for failed tests not being tracked yet?

AndrewJSchofield · 2023-11-29T17:15:30Z

@junrao The failing tests are not related. Here's a summary:

Kafka Streams EOS integration tests
KRaft controller tests
Consumer coordinator failover test
Fetch from follower integration tests
Mirroring integration tests
Transactions with tiered storage tests

I will file jiras for the 7 failing tests which are not currently being tracked.

None of the failures appear related to the code in this PR.

junrao · 2023-11-29T17:28:53Z

Thanks for the analysis, @AndrewJSchofield. Does the following mean that some of the tests didn't run? Should we rerun the tests?

Build / JDK 21 and Scala 2.13 / Gradle Test Executor 92.failed to execute tests
org.gradle.api.internal.tasks.testing.TestSuiteExecutionException: Could not complete execution for Gradle Test Executor 92.
	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:64)
	at java.base@21.0.1/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base@21.0.1/java.lang.reflect.Method.invoke(Method.java:580)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
	at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
	at jdk.proxy1/jdk.proxy1.$Proxy2.stop(Unknown Source)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker$3.run(TestWorker.java:193)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.executeAndMaintainThreadName(TestWorker.java:129)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:100)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:60)
	at org.gradle.process.internal.worker.child.ActionExecutionWorker.execute(ActionExecutionWorker.java:56)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:113)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:65)
	at app//worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:69)
	at app//worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:74)
Caused by: java.lang.OutOfMemoryError: GC overhead limit exceeded

junrao · 2023-11-30T18:06:30Z

@AndrewJSchofield : It seems that the last build timed out?

AndrewJSchofield · 2023-11-30T18:24:44Z

I think so. After 8 hours. I will try resubmitting now (which means it will run overnight for me). In the event that it's still problematic, I'll make a fresh PR and try again.

AndrewJSchofield · 2023-12-01T07:31:51Z

@junrao This time Java 8, 11 and 21 completed with a handful of unrelated flaky test failures, but Java 17 timed out. The CI system seems to be a dice roll at the moment.

junrao · 2023-12-05T00:06:12Z

Just merged a related PR #14767. Re-triggering the tests to make sure there are no new issues.

junrao

@AndrewJSchofield : Thanks for the PR. The test failures seem unrelated. LGTM

This adds the new ListClientMetricsResources RPC to the Kafka protocol and puts support into the Kafka admin client. The broker-side implementation in this PR is just to return an empty list. A future PR will obtain the list from the config store. Includes a few unit tests for what is a very simple RPC. There are additional tests already written and waiting for the PR that delivers the kafka-client-metrics.sh tool which builds on this PR. Reviewers: Jun Rao <junrao@gmail.com>

AndrewJSchofield force-pushed the KAFKA-15831 branch from ce4d98c to 69d55f6 Compare November 21, 2023 14:34

junrao reviewed Nov 21, 2023

View reviewed changes

AndrewJSchofield requested a review from junrao November 24, 2023 20:39

mjsax added core Kafka Broker kip Requires or implements a KIP admin AdminClient labels Nov 27, 2023

apoorvmittal10 approved these changes Nov 28, 2023

View reviewed changes

junrao reviewed Nov 28, 2023

View reviewed changes

AndrewJSchofield closed this Nov 28, 2023

AndrewJSchofield reopened this Nov 28, 2023

AndrewJSchofield closed this Nov 28, 2023

AndrewJSchofield reopened this Nov 28, 2023

AndrewJSchofield closed this Nov 29, 2023

junrao reopened this Nov 29, 2023

AndrewJSchofield closed this Nov 30, 2023

AndrewJSchofield reopened this Nov 30, 2023

AndrewJSchofield closed this Nov 30, 2023

AndrewJSchofield reopened this Nov 30, 2023

AndrewJSchofield mentioned this pull request Dec 1, 2023

KAFKA-15831: KIP-1000 protocol and admin client #14894

Closed

3 tasks

AndrewJSchofield force-pushed the KAFKA-15831 branch from 7c28284 to 00c81f0 Compare December 3, 2023 16:41

AndrewJSchofield force-pushed the KAFKA-15831 branch from 00c81f0 to b84fc73 Compare December 4, 2023 13:17

AndrewJSchofield added 5 commits December 4, 2023 18:02

KAFKA-15831: KIP-1000 protocol and admin client

2cd4c35

Checkstyle fixes

051e0fe

Address review comments

9bc1103

Fix test failures

2d8d859

Review comments

c50079e

AndrewJSchofield force-pushed the KAFKA-15831 branch from b84fc73 to c50079e Compare December 4, 2023 18:02

junrao closed this Dec 5, 2023

junrao reopened this Dec 5, 2023

junrao approved these changes Dec 5, 2023

View reviewed changes

junrao merged commit 587f50d into apache:trunk Dec 5, 2023

AndrewJSchofield deleted the KAFKA-15831 branch December 5, 2023 15:24

Uh oh!

Conversation

AndrewJSchofield commented Nov 21, 2023

Committer Checklist (excluded from commit message)

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

junrao Nov 21, 2023

Choose a reason for hiding this comment

Uh oh!

AndrewJSchofield Nov 24, 2023

Choose a reason for hiding this comment

Uh oh!

junrao Nov 21, 2023

Choose a reason for hiding this comment

Uh oh!

AndrewJSchofield Nov 24, 2023

Choose a reason for hiding this comment

Uh oh!

junrao Nov 21, 2023

Choose a reason for hiding this comment

Uh oh!

AndrewJSchofield Nov 24, 2023

Choose a reason for hiding this comment

Uh oh!

AndrewJSchofield commented Nov 27, 2023

Uh oh!

apoorvmittal10 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

junrao commented Nov 28, 2023

Uh oh!

AndrewJSchofield commented Nov 28, 2023

Uh oh!

AndrewJSchofield commented Nov 28, 2023

Uh oh!

junrao commented Nov 29, 2023

Uh oh!

AndrewJSchofield commented Nov 29, 2023

Uh oh!

junrao commented Nov 29, 2023

Uh oh!

junrao commented Nov 30, 2023

Uh oh!

AndrewJSchofield commented Nov 30, 2023

Uh oh!

AndrewJSchofield commented Dec 1, 2023

Uh oh!

junrao commented Dec 5, 2023

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

apoorvmittal10 left a comment •

edited

Loading