KAFKA-16472: Fix integration tests in Java with parameter name

[FrankYang0529]

Following test cases don't really run kraft case. The reason is that the test info doesn't contain parameter name, so it always returns false in TestInfoUtils#isKRaft.

• TopicCommandIntegrationTest
• DeleteConsumerGroupsTest
• AuthorizerIntegrationTest
• DeleteOffsetsConsumerGroupCommandIntegrationTest

We can fix it by adding options.compilerArgs << '-parameters' after

kafka/build.gradle

Lines 264 to 273 in 376e9e2

	tasks.withType(JavaCompile) {
	options.encoding = 'UTF-8'
	options.compilerArgs << "-Xlint:all"
	// temporary exclusions until all the warnings are fixed
	if (!project.path.startsWith(":connect"))
	options.compilerArgs << "-Xlint:-rawtypes"
	options.compilerArgs << "-Xlint:-serial"
	options.compilerArgs << "-Xlint:-try"
	options.compilerArgs << "-Werror"

Also, we have to add String quorum to cases in DeleteOffsetsConsumerGroupCommandIntegrationTest.

Committer Checklist (excluded from commit message)

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

[showuon]

@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?

[FrankYang0529]

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.

[chia7712]

LGTM

[chia7712]

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?

[chia7712]

Could you add comments for this arg?

[FrankYang0529]

Updated it. Thank you.

[FrankYang0529]

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

[showuon]

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 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?

[chia7712]

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

[FrankYang0529]

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 .

[chia7712]

@ijuma Could you please take a look at this PR?

[chia7712]

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.

[showuon]

LGTM! Thanks @FrankYang0529 ! Really nice catch!