KAFKA-9027, KAFKA-9028: Convert create/delete acls requests/response to use generated protocol by ijuma · Pull Request #7725 · apache/kafka

ijuma · 2019-11-21T04:42:47Z

Also add support for flexible versions to both protocol types.

Co-authored-by: Rajini Sivaram rajinisivaram@googlemail.com
Co-authored-by: Jason Gustafson jason@confluent.io

Committer Checklist (excluded from commit message)

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

cmccabe · 2019-12-06T00:15:59Z

Hmm. This part isn't quite right. We should be iterating over aclCreations, since that is what we actually sent out on the wire. Entries in acls may or may not have been sent out on the wire (if they were invalid, they weren't.)

I think we should remove the TODO comment about "duplicates"... since duplicates never get sent on the wire, they don't need to be handled here (if we iterate over the right collection)

I think you assumed this was complete, it's WIP as the commit message says: 755a6f2

The TODO is also a hint of the same. :)

cmccabe · 2019-12-06T00:18:18Z

This is another case that needs to be fixed. The simplest way would be just creating a list which contains all the AclBindingFilter objects that were sent on the wire (it seems that the incorrect decision to use filters in this PR was caused by the change in type in filterList from AclBindingFilter to DeleteAclsRequestData.DeleteAclsFilter)

Again, this was not complete and hence why there was a FIXME in it. ;)

cmccabe · 2019-12-06T00:25:29Z

I was surprised by this when I read it, and had to think back to why we did this :) It would be good to add a comment about why we're doing this, I think. The explanation would be something like this: on older brokers, no pattern types existed except LITERAL (effectively...). So even though ANY is not directly supported on those brokers, we can get the same effect as ANY by setting LITERAL on those particular brokers.

Also, it's really ugly for "validate" to mutate the request. Can we have another function for this... 🙏

@cmccabe Older brokers supported LITERAL and *, right?

"*" is considered "LITERAL" for compatibility reasons

…ated protocol Also add support for flexible versions to both protocol types.

…r` list

Also use assertThrows instead of expected attribute in Test annotation.

With DeleteAclsRequest and respective tests

ijuma · 2020-01-31T20:45:06Z

This should be ready to review once the tests complete.

ijuma · 2020-02-02T00:46:07Z

1 failure in one job and 4 in the other. All unrelated to this PR.

ijuma · 2020-02-02T00:46:16Z

retest this please

ijuma · 2020-02-02T01:54:25Z

retest this please

ijuma · 2020-02-02T17:47:32Z

2 flaky failures in one job and 1 in another.

ijuma · 2020-02-02T17:47:37Z

retest this please

rajinisivaram

@ijuma Thanks for the PR, LGTM

ijuma · 2020-02-03T15:10:34Z

More unrelated flaky failures:

JDK 11:

kafka.admin.DescribeConsumerGroupTest.testDescribeGroupWithShortInitializationTimeout
kafka.admin.ResetConsumerGroupOffsetTest.testResetOffsetsExportImportPlan
org.apache.kafka.streams.kstream.internals.KStreamImplTest.shouldSupportTriggerMaterializedWithKTableFromKStream

JDK 8:

kafka.admin.DeleteConsumerGroupsTest.testDeleteCmdAllGroups
kafka.api.ConsumerBounceTest.testCloseDuringRebalance

ijuma · 2020-02-03T15:12:32Z

I cherry-picked this to 2.5 so that describe/create/delete ACLs are all bumped at the same time.

…to use generated protocol (#7725) Also add support for flexible versions to both protocol types. Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>, Colin Patrick McCabe <cmccabe@apache.org> Co-authored-by: Rajini Sivaram <rajinisivaram@googlemail.com> Co-authored-by: Jason Gustafson <jason@confluent.io>

Conflicts related to version bump in AK, replaced 2.6 with 6.0: * gradle.properties * __init__.py * version.py * apache-github/trunk: KAFKA-9490: Fix generics for Grouped (apache#8028) Bump trunk to 2.6.0-SNAPSHOT (apache#8026) KAFKA-9027, KAFKA-9028: Convert create/delete acls requests/response to use generated protocol (apache#7725) MINOR: Refactor CheckpointFile to improve testability (apache#7391) MINOR: updated documentation where RocksDBStore was being used as the sample class for byte[] versus Bytes examples (apache#5884)

…to use generated protocol (apache#7725) Also add support for flexible versions to both protocol types. Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>, Colin Patrick McCabe <cmccabe@apache.org> Co-authored-by: Rajini Sivaram <rajinisivaram@googlemail.com> Co-authored-by: Jason Gustafson <jason@confluent.io>

ijuma changed the title ~~KAFKA-9027: Convert create/delete acls requests/response to use generated protocol~~ KAFKA-9027, KAFKA-9028: Convert create/delete acls requests/response to use generated protocol Nov 21, 2019

ijuma mentioned this pull request Nov 24, 2019

KAFKA-9026: Use automatic RPC generation in DescribeAcls #7560

Merged

3 tasks

ijuma force-pushed the kafka-9027-create-acls-generated branch from 30101cc to 4a14ddd Compare November 24, 2019 17:39

hachikuji force-pushed the kafka-9027-create-acls-generated branch 2 times, most recently from 8490c73 to 0657ddb Compare December 5, 2019 23:59

cmccabe reviewed Dec 6, 2019

View reviewed changes

ijuma force-pushed the kafka-9027-create-acls-generated branch from 0657ddb to c6c81e9 Compare December 9, 2019 15:16

ijuma and others added 5 commits January 30, 2020 07:39

KAFKA-9027: Convert create/delete acls requests/response to use gener…

59c9705

…ated protocol Also add support for flexible versions to both protocol types.

Remove unnecessary classes in CreateAclsRequest and CreateAclsResponse

43d5c1d

WIP: Simplify

f228b05

Fix failing test cases

453651a

Remove DeleteAclsRequest.Builder overload that takes `AclBindingFilte…

b0f13b0

…r` list

ijuma force-pushed the kafka-9027-create-acls-generated branch from c6c81e9 to b0f13b0 Compare January 30, 2020 15:59

ijuma added 12 commits January 30, 2020 08:07

Remove unused methods

5ff2463

Remove unused methods

7e5abcf

Address review comments for DeleteAclsRequest

f8b789c

Also use assertThrows instead of expected attribute in Test annotation.

Tweak DescribeAclsRequest for consistency

23b6ecd

With DeleteAclsRequest and respective tests

Simplify DescribeAclsRequest

5f955ba

Minor improvemens

ede4c80

Remove unnecessary classes in DeleteAclsResponse

90f4470

Avoid unnecessary allocation in CreateAclsRequest

f288303

Minor efficiency improvement

e9a29c7

Fix KafkaAdminClient.createAcls

d3477a0

CreateAcls renames for consistency

f66f569

Fix KafkaAdminClient.deleteAcls

4f90436

ijuma requested a review from rajinisivaram January 31, 2020 20:43

Fix bug in DeleteAclsRequest

44bf6d1

ijuma force-pushed the kafka-9027-create-acls-generated branch from e70688f to 44bf6d1 Compare January 31, 2020 20:44

rajinisivaram approved these changes Feb 3, 2020

View reviewed changes

ijuma merged commit 738e14e into apache:trunk Feb 3, 2020

ijuma deleted the kafka-9027-create-acls-generated branch February 3, 2020 15:12

Conversation

ijuma commented Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

cmccabe Dec 6, 2019 • 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!

ijuma Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

cmccabe Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

ijuma Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

cmccabe Dec 6, 2019 • 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!

cmccabe Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

ijuma Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

cmccabe Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

ijuma commented Jan 31, 2020

Uh oh!

ijuma commented Feb 2, 2020

Uh oh!

ijuma commented Feb 2, 2020

Uh oh!

ijuma commented Feb 2, 2020

Uh oh!

ijuma commented Feb 2, 2020

Uh oh!

ijuma commented Feb 2, 2020

Uh oh!

rajinisivaram left a comment

Choose a reason for hiding this comment

Uh oh!

ijuma commented Feb 3, 2020

Uh oh!

ijuma commented Feb 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ijuma commented Nov 21, 2019 •

edited

Loading

cmccabe Dec 6, 2019 •

edited

Loading

cmccabe Dec 6, 2019 •

edited

Loading