lianetm commented on code in PR #18532:
URL: https://github.com/apache/kafka/pull/18532#discussion_r1923992550
##########
core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala:
##########
@@ -297,12 +302,15 @@ class ConsumerBounceTest extends AbstractConsumerTest
with Logging {
* there is no coordinator, but close should timeout and return. If close is
invoked with a very
* large timeout, close should timeout after request timeout.
*/
- private def checkCloseWithClusterFailure(numRecords: Int, group1: String,
group2: String): Unit = {
+ private def checkCloseWithClusterFailure(numRecords: Int, group1: String,
group2: String,
+ groupProtocol: String): Unit = {
val consumer1 = createConsumerAndReceive(group1, manualAssign = false,
numRecords)
val requestTimeout = 6000
- this.consumerConfig.setProperty(ConsumerConfig.SESSION_TIMEOUT_MS_CONFIG,
"5000")
-
this.consumerConfig.setProperty(ConsumerConfig.HEARTBEAT_INTERVAL_MS_CONFIG,
"1000")
+ if (groupProtocol.equals(GroupProtocol.CLASSIC.name)) {
+
this.consumerConfig.setProperty(ConsumerConfig.SESSION_TIMEOUT_MS_CONFIG,
"5000")
+
this.consumerConfig.setProperty(ConsumerConfig.HEARTBEAT_INTERVAL_MS_CONFIG,
"1000")
+ }
Review Comment:
Here we conditionally set these 2 only really, and then the HB only in some
other places. Happy to change how you think it's better, but personally I don't
quite see a clearer shape for this given that we set different things based on
the condition (sometimes HB, sometimes session, some both). Of course params
would do, or separate funcs, but then seems more obfuscated than explicitly
setting properties in the test?
1. `maybeSetHB`(groupProtocol, hbInvertal) +
`maybeSetSession`(groupProtocol, sessionTimeout)
2. `maybeSetHBAndSession`(groupProtocol, hb, session) -> it would need to
accept null, we don't always set them both
Changing this in TestUtils and used for all test would bring in changes in a
lot of files, that I think would be better to keep out of this PR, so if you
prefer a refactor to one of the options above could it be separate jira for
changing TestUtils and all the usages?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
