Skip to content

KAFKA-10769 Remove JoinGroupRequest#containsValidPattern as it is dup… #9851

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 12 commits into from
Apr 7, 2021
Merged

KAFKA-10769 Remove JoinGroupRequest#containsValidPattern as it is dup… #9851

merged 12 commits into from
Apr 7, 2021

Conversation

highluck
Copy link
Contributor

@highluck highluck commented Jan 9, 2021

https://issues.apache.org/jira/browse/KAFKA-10769
.cc @chia7712

Committer Checklist (excluded from commit message)

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

@highluck
Copy link
Contributor Author

highluck commented Jan 9, 2021

@chia7712
Please review! thanks!

@chia7712
Copy link
Member

@highluck Thanks for your patch.

The check of static group id (https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/requests/JoinGroupRequest.java#L68) is almost same to check in Topic (https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/internals/Topic.java#L36).

Should we remove the duplicate also? WDYT?

@chia7712
Copy link
Member

@abbccdda Could we reuse the code (https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/internals/Topic.java#L36) to do the validation for static member id? Please correct me if there is any concern. Thanks!

@highluck
Copy link
Contributor Author

highluck commented Jan 10, 2021 •
edited
Loading

@chia7712, @abbccdda
The code was modified
How about the following?

@highluck
Copy link
Contributor Author

@chia7712 @abbccdda
Would it be okay to ask for confirmation once?

chia7712
chia7712 reviewed Feb 24, 2021
View reviewed changes
clients/src/main/java/org/apache/kafka/common/requests/JoinGroupRequest.java
/**
* Ported from class Topic in {@link org.apache.kafka.common.internals} to restrict the charset for
* static member id.
*/
public static void validateGroupInstanceId(String id) {
if (id.equals(""))
throw new InvalidConfigurationException("Group instance id must be non-empty string");
Copy link
Member

Choose a reason for hiding this comment

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

The exception is changed from InvalidConfigurationException to InvalidTopicException. It results in different Errors so I feel it would be better to keep throwing InvalidConfigurationException for this check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modified code thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping @chia7712
thanks

Copy link
Member

Choose a reason for hiding this comment

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

the following check have similar issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chia7712
Would it be okay to explain a little more about what follow check means?

Copy link
Member

Choose a reason for hiding this comment

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

sorry for unclear comment :(

My point was error type thrown by following checks was changed also.

Copy link
Contributor Author

@highluck highluck Mar 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@chia7712
thanks
I modified code!
I think everything will be fine if I modify it like this. How about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping @chia7712
thanks!

@highluck highluck requested a review from chia7712 March 4, 2021 15:42
@highluck
Copy link
Contributor Author

ping @chia7712
Please give me a review

chia7712
chia7712 reviewed Mar 26, 2021
View reviewed changes
Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@highluck thanks for updating code. please take a look at following comments. thanks!

clients/src/test/java/org/apache/kafka/common/requests/JoinGroupRequestTest.java Outdated
@@ -64,7 +65,7 @@ public void shouldRecognizeInvalidCharactersInGroupInstanceIds() {

for (char c : invalidChars) {
String instanceId = "Is " + c + "illegal";
assertFalse(JoinGroupRequest.containsValidPattern(instanceId));
assertFalse(Topic.containsValidPattern(instanceId));
Copy link
Member

Choose a reason for hiding this comment

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

This test case is duplicate to https://github.com/apache/kafka/blob/trunk/clients/src/test/java/org/apache/kafka/common/internals/TopicTest.java#L65. I feel it is fine to remove it.

clients/src/main/java/org/apache/kafka/common/internals/Topic.java Outdated
@@ -74,7 +81,7 @@ public static boolean hasCollision(String topicA, String topicB) {
/**
* Valid characters for Kafka topics are the ASCII alphanumerics, '.', '_', and '-'
*/
static boolean containsValidPattern(String topic) {
public static boolean containsValidPattern(String topic) {
Copy link
Member

Choose a reason for hiding this comment

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

This change is unnecessary if we remove the duplicate test case.

@highluck highluck requested a review from chia7712 March 26, 2021 16:09
@highluck
Copy link
Contributor Author

ping @chia7712
Please give me a review
thanks!

@chia7712
Copy link
Member

@highluck Could you merge trunk to trigger QA again? It seems our jenkins was on vacation before :)

@highluck
Copy link
Contributor Author

highluck commented Apr 1, 2021

@chia7712 There seems to be some hung builds. Shall we merge again?

@chia7712
Copy link
Member

chia7712 commented Apr 1, 2021

yep, please merge trunk. Thanks!

@highluck
Copy link
Contributor Author

highluck commented Apr 5, 2021

@chia7712 There seems to be some test fail.
Shall we merge again?!

chia7712
chia7712 approved these changes Apr 6, 2021
View reviewed changes
Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@highluck thanks for updating code. one last comment is left. Please fix it and then I will merge this patch.

clients/src/test/java/org/apache/kafka/common/requests/JoinGroupRequestTest.java Outdated
}
}


Copy link
Member

Choose a reason for hiding this comment

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

please remove those indents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chia7712
thanks i updated code

@chia7712 chia7712 merged commit be592df into apache:trunk Apr 7, 2021
ijuma added a commit to ijuma/kafka that referenced this pull request Apr 7, 2021
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into remove-checksum
136f86f 
* apache-github/trunk:
  KAFKA-10769 Remove JoinGroupRequest#containsValidPattern as it is dup… (apache#9851)
  KAFKA-12384: stabilize ListOffsetsRequestTest#testResponseIncludesLeaderEpoch (apache#10389)
  KAFKA-5146: remove Connect dependency from Streams module (apache#10131)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chia7712 chia7712 chia7712 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@highluck @chia7712