Re: [PR] KAFKA-16472: Fix integration tests in Java with parameter name [kafka]
showuon commented on PR #15663: URL: https://github.com/apache/kafka/pull/15663#issuecomment-2040993296 LGTM! Thanks @FrankYang0529 ! Really nice catch! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16472: Fix integration tests in Java with parameter name [kafka]
chia7712 merged PR #15663: URL: https://github.com/apache/kafka/pull/15663 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16472: Fix integration tests in Java with parameter name [kafka]
chia7712 commented on PR #15663: URL: https://github.com/apache/kafka/pull/15663#issuecomment-2040889410 I'm going to merge this PR in order to make other tool tests can run with KRaft. We can address all late reviews in other PRs. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16472: Fix integration tests in Java with parameter name [kafka]
chia7712 commented on PR #15663: URL: https://github.com/apache/kafka/pull/15663#issuecomment-2040402625 @ijuma Could you please take a look at this PR? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16472: Fix integration tests in Java with parameter name [kafka]
FrankYang0529 commented on PR #15663: URL: https://github.com/apache/kafka/pull/15663#issuecomment-2039753742 > > I think the class file size increasing is indeed a direct drawback after adding -parameter option because we'll include all the parameters into .class files. I'd like to know if there's any other way to fix this? Could we use ARGUMENTS instead of ARGUMENTS_WITH_NAMES? > > Or we can add the new arg to compileTestJava only to avoid impacting production binary Updated it as `compileTestJava.options.compilerArgs.add "-parameters"`. Thanks for the suggestion @chia7712 @showuon . -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16472: Fix integration tests in Java with parameter name [kafka]
chia7712 commented on PR #15663: URL: https://github.com/apache/kafka/pull/15663#issuecomment-2039097741 > I think the class file size increasing is indeed a direct drawback after adding -parameter option because we'll include all the parameters into .class files. I'd like to know if there's any other way to fix this? Could we use ARGUMENTS instead of ARGUMENTS_WITH_NAMES? Or we can add the new arg to compileTestJava only to avoid impacting production binary -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16472: Fix integration tests in Java with parameter name [kafka]
showuon commented on PR #15663: URL: https://github.com/apache/kafka/pull/15663#issuecomment-2039081866 Sorry @FrankYang0529 , I saw this: https://stackoverflow.com/questions/44067477/drawbacks-of-javac-parameters-flag > Briefly, the stated reasons to make parameter names optional are concerns about class file size, compatibility surface, and exposure of sensitive information. I think the class file size increasing is indeed a direct drawback after adding `-parameter` option. I'd like to know if there's any other way to fix this? Could we use `ARGUMENTS` instead of `ARGUMENTS_WITH_NAMES`? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16472: Fix integration tests in Java with parameter name [kafka]
FrankYang0529 commented on code in PR #15663:
URL: https://github.com/apache/kafka/pull/15663#discussion_r1553003195
##
build.gradle:
##
@@ -270,6 +270,7 @@ subprojects {
options.compilerArgs << "-Xlint:-serial"
options.compilerArgs << "-Xlint:-try"
options.compilerArgs << "-Werror"
+options.compilerArgs << "-parameters"
Review Comment:
Updated it. Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16472: Fix integration tests in Java with parameter name [kafka]
FrankYang0529 commented on PR #15663: URL: https://github.com/apache/kafka/pull/15663#issuecomment-2039041551 > > However, this way still doesn't check whether "parameter name" is correct. Probably, we can give another check is that if display name contains zk or kraft, but not quorum, then throwing error in TestInfoUtils#isKRaft. > > @FrankYang0529 Could you file jira to trace this? Yes, added it. https://issues.apache.org/jira/browse/KAFKA-16476 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16472: Fix integration tests in Java with parameter name [kafka]
chia7712 commented on code in PR #15663:
URL: https://github.com/apache/kafka/pull/15663#discussion_r1552981401
##
build.gradle:
##
@@ -270,6 +270,7 @@ subprojects {
options.compilerArgs << "-Xlint:-serial"
options.compilerArgs << "-Xlint:-try"
options.compilerArgs << "-Werror"
+options.compilerArgs << "-parameters"
Review Comment:
Could you add comments for this arg?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16472: Fix integration tests in Java with parameter name [kafka]
chia7712 commented on PR #15663: URL: https://github.com/apache/kafka/pull/15663#issuecomment-2039025377 > However, this way still doesn't check whether "parameter name" is correct. Probably, we can give another check is that if display name contains zk or kraft, but not quorum, then throwing error in TestInfoUtils#isKRaft. @FrankYang0529 Could you file jira to trace this? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16472: Fix integration tests in Java with parameter name [kafka]
FrankYang0529 commented on PR #15663: URL: https://github.com/apache/kafka/pull/15663#issuecomment-2039019880 > 2. How could we avoid this things happen in the future? Like adding some checking before the tests startup or something? Do you have any idea? I don't have a good idea to do the check in gradle or before the tests startup. The possible way maybe check whether `TestInfo#getDisplayName` contains `.` but not `=`. For example, before the fix, `DeleteOffsetsConsumerGroupCommandIntegrationTest#testDeleteOffsetsNonExistingGroup` shows `(String).` and `TopicCommandIntegrationTest#testCreate` shows `(String).zk` or `(String).kraft`. After the fix, both test cases show `(String).quorum=zk` or `(String).quorum=kraft`. So we can know that if display name has `.`, but not `=`, it means the case misses parameter name. However, this way still doesn't check whether "parameter name" is correct. Probably, we can give another check is that if display name contains `zk` or `kraft`, but not `quorum`, then throwing error in `TestInfoUtils#isKRaft`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16472: Fix integration tests in Java with parameter name [kafka]
showuon commented on PR #15663: URL: https://github.com/apache/kafka/pull/15663#issuecomment-2038807461 @FrankYang0529 , thanks for the fix! Nice catch! Questions: 1. How could we confirm the `String quorum` missing only in `DeleteOffsetsConsumerGroupCommandIntegrationTest`, not other places? 2. How could we avoid this things happen in the future? Like adding some checking before the tests startup or something? Do you have any idea? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
