Skip to content

Conversation

@Rancho-7
Copy link
Collaborator

@Rancho-7 Rancho-7 commented Apr 21, 2025

jira: https://issues.apache.org/jira/browse/KAFKA-19178

The vector is a synchronized collection, and in the case we don't need
to sync. Also, we can use Collections.enumeration to convert
collection to enumeration easily.

Reviewers: PoAn Yang payang@apache.org, Ken Huang
s7133700@gmail.com, Chia-Ping Tsai chia7712@gmail.com

@github-actions github-actions bot added connect triage PRs from the community small Small PRs labels Apr 21, 2025
Copy link
Member

@FrankYang0529 FrankYang0529 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Could you also refactor Vector as ArrayList in

Vector<AclBinding> aclBindings = FakeLocalMetadataStore.ALL_ACLS.getOrDefault(principal, new Vector<>());

Thanks.

@github-actions github-actions bot removed the triage PRs from the community label Apr 22, 2025
@Rancho-7
Copy link
Collaborator Author

Thanks for the PR. Could you also refactor Vector as ArrayList in

Vector<AclBinding> aclBindings = FakeLocalMetadataStore.ALL_ACLS.getOrDefault(principal, new Vector<>());

Thanks.

@FrankYang0529 Thanks for the suggestion! I just have one concern — in our current test setup,both the primary and backup clusters use 3 connector workers each. Since ArrayList isn't thread safe,do you think this change could introduce any concurrency issues in this context?

@chia7712
Copy link
Member

@Rancho-7 the returned type Enumeration is a read-only interface, so it should be fine to use ArrayList

@Rancho-7
Copy link
Collaborator Author

Since this change will only affect the test scenarios and to maintain consistency in the code, I believe replacing it with ArrayList shouldn't have a significant impact. Please let me know if you have any further thoughts or concerns. @chia7712 @FrankYang0529

@FrankYang0529 FrankYang0529 requested a review from chia7712 April 24, 2025 07:33
*/
public static void addACLs(String principal, AclBinding aclBinding) {
Vector<AclBinding> aclBindings = FakeLocalMetadataStore.ALL_ACLS.getOrDefault(principal, new Vector<>());
List<AclBinding> aclBindings = FakeLocalMetadataStore.ALL_ACLS.getOrDefault(principal, new ArrayList<>());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is interesting. It seems those methods should be thread-safe, but it isn't. Could you please revert those changes and then open a jira for it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chia7712 chia7712 merged commit 51ef290 into apache:trunk Apr 26, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants