KAFKA-9026: Use automatic RPC generation in DescribeAcls by mimaison · Pull Request #7560 · apache/kafka

mimaison · 2019-10-19T17:08:40Z

Committer Checklist (excluded from commit message)

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

ijuma · 2019-11-20T14:11:22Z

Thanks for the PR. Before I start the review, would you mind fixing the conflict please?

mimaison · 2019-11-20T18:42:45Z

Thanks @ijuma, I've rebased on trunk

ijuma · 2019-11-24T01:35:47Z

Why are we validating in the builder instead of the request?

I think it's important to be able to create bogus Requests/Responses in order to validate error handling on the broker side. By validating in the Builder, we allow creating arbitrary objects via the constructor. I expect all callers to use the Builder.

This is what a few requests already do (CreateTopicsRequest, FindCoordinatorRequest, HeartbeatRequest) and I liked that pattern.

I would say it's important not to be able to create bogus requests. ;) We can introduce specific mechanisms for testing, but a public constructor for a request should do its own validation.

Right! 👍

ijuma

Thanks for the PR, I left a few initial comments.

ijuma · 2019-11-24T01:36:16Z

This is not necessary, right?

ijuma · 2019-11-24T01:41:06Z

Nit: no need for ()

ijuma · 2019-11-24T01:42:57Z

We should probably remove this method. It makes it too easy to write highly inefficient code. See #7725 for an alternative approach.

Obsolutely! I've updated the logic to avoid that.

mimaison · 2019-11-25T13:16:20Z

Thanks @ijuma for the review. I've pushed an update

ijuma

Thanks for the updates. A few more comments.

ijuma · 2019-11-25T14:59:26Z

It's pretty inefficient to create all these AclBinding instances, just to check for unknowns. Is it possible to avoid that?

ijuma · 2019-11-25T15:03:36Z

I think we should convert these tests not to rely on AclBinding unless they are testing prepareResponse specifically.

ijuma · 2019-11-25T15:04:39Z

Since this is a serialization/deserialization test, I think it should not rely on prepareResponse.

ijuma · 2019-11-25T15:05:33Z

This is existing code, but is there a reason why we can't just do asJava instead of this?

ijuma · 2019-11-25T15:06:18Z

Maybe this should be called fromBindings.

Many responses have a prepareResponse() method, so I thought it made sense to call it this way for consistency. If fromBinding() is clearer, I'm happy to update it

ijuma · 2019-11-26T15:26:11Z

One additional suggestion: let's bump the protocol version and add support for flexible versions as per KIP-482.

mimaison · 2019-12-05T18:10:47Z

+    private void validate(short version) {
+        if (version == 0) {
+            if (data.resourcePatternType() == PatternType.ANY.code()) {
+                data.setResourcePatternType(PatternType.LITERAL.code());


I tried changing the JSON request definition but I couldn't get it to generate the right Data class so I've had to put this logic :(
As far as I can tell DeleteAclsRequest has the same requirement.

mimaison · 2019-12-05T18:11:24Z

Thanks @ijuma for the feedback, I've pushed another update.

ijuma · 2020-01-29T10:21:21Z

retest this please

ijuma · 2020-01-29T20:43:09Z

One job passed and the other had a couple of unrelated flaky failures:

kafka.admin.ResetConsumerGroupOffsetTest.testResetOffsetsExportImportPlan
org.apache.kafka.connect.mirror.MirrorConnectorsIntegrationTest.testReplication

ijuma

I think this is in a good enough state to be merged. I'll then rebase my create/delete acls PR and we can make additional changes for consistency as part of that PR.

mimaison · 2020-01-29T20:46:41Z

Thanks @ijuma

Conflicts and/or compiler errors due to the fact that we temporarily reverted the commit that removes Scala 2.11 support: * SslAdminIntegrationTest: keep using JAdminClient, take upstream changes otherwise. * ReassignPartitionsClusterTest: keep using JAdminClient, take upstream changes otherwise. * KafkaApis: use `asScala.foreach` instead of `forEach`. # By Ismael Juma (3) and others # Via GitHub * apache-github/trunk: (22 commits) KAFKA-9437; Make the Kafka Protocol Friendlier with L7 Proxies [KIP-559] (apache#7994) KAFKA-9375: Add names to all Connect threads (apache#7901) MINOR: Introduce 2.5-IV0 IBP (apache#8010) KAFKA-8503; Add default api timeout to AdminClient (KIP-533) (apache#8011) Add retries to release.py script (apache#8021) KAFKA-8162: IBM JDK Class not found error when handling SASL (apache#6524) MINOR: Add explicit result type in public defs/vals (apache#7993) KAFKA-9408: Use StandardCharsets.UTF-8 instead of "UTF-8" (apache#7940) KAFKA-9474: Adds 'float64' to the RPC protocol types (apache#8012) KAFKA-9360: Allow disabling MM2 heartbeat and checkpoint emissions (apache#7887) KAFKA-7658: Add KStream#toTable to the Streams DSL (apache#7985) KAFKA-9445: Allow adding changes to allow serving from a specific partition (apache#7984) KAFKA-9422: Track the set of topics a connector is using (KIP-558) (apache#8017) KAFKA-9040; Add --all option to config command (apache#7607) KAFKA-4203: Align broker default for max.message.bytes with Java producer default (apache#4154) KAFKA-9426: Use switch instead of chained if/else in OffsetsForLeaderEpochClient (apache#7959) KAFKA-9405: Use Map.computeIfAbsent where applicable (apache#7937) KAFKA-9026: Use automatic RPC generation in DescribeAcls (apache#7560) MINOR: Remove unused fields in StreamsMetricsImpl (apache#7992) KAFKA-9460: Enable only TLSv1.2 by default and disable other TLS protocol versions (KIP-553) (apache#7998) ...

mimaison force-pushed the KAFKA-9026 branch from 32bf382 to 74900bc Compare November 20, 2019 18:42

ijuma reviewed Nov 24, 2019

View reviewed changes

ijuma reviewed Nov 25, 2019

View reviewed changes

mimaison added 3 commits December 5, 2019 17:30

KAFKA-9026: Use automatic RPC generation in DescribeAcls

1469d70

Addressed reviews

a63ab2a

2nd round of reviews

e951dad

mimaison force-pushed the KAFKA-9026 branch from 9722a17 to e951dad Compare December 5, 2019 18:05

mimaison commented Dec 5, 2019

View reviewed changes

ijuma approved these changes Jan 29, 2020

View reviewed changes

ijuma merged commit 40b3517 into apache:trunk Jan 29, 2020

mimaison deleted the KAFKA-9026 branch January 29, 2020 20:46

omkreddy mentioned this pull request Feb 8, 2020

MINOR: Update schema field names in DescribeAcls Request/Response #8075

Closed

3 tasks

Conversation

mimaison commented Oct 19, 2019

Committer Checklist (excluded from commit message)

Uh oh!

ijuma commented Nov 20, 2019

Uh oh!

mimaison commented Nov 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimaison commented Nov 25, 2019

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Nov 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimaison commented Dec 5, 2019

Uh oh!

ijuma commented Jan 29, 2020

Uh oh!

ijuma commented Jan 29, 2020

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

mimaison commented Jan 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants