showuon commented on a change in pull request #9202: URL: https://github.com/apache/kafka/pull/9202#discussion_r486161428
########## File path: core/src/test/scala/unit/kafka/coordinator/group/GroupMetadataManagerTest.scala ########## @@ -931,6 +932,42 @@ class GroupMetadataManagerTest { assertTrue(group.has(memberId)) } + @Test + def testShouldThrowExceptionForUnsupportedGroupMetadataVersion(): Unit = { + val generation = 1 + val protocol = "range" + val memberId = "memberId" + val unSupportedVersion = Short.MinValue + + // put the un-supported version as the version value + val groupMetadataRecordValue = buildStableGroupRecordWithMember(generation, protocolType, protocol, memberId) + .value().putShort(unSupportedVersion) + // reset the position to the starting position 0 so that it can read the data in correct order + groupMetadataRecordValue.position(0) + + val e = assertThrows(classOf[KafkaException], + () => GroupMetadataManager.readGroupMessageValue(groupId, groupMetadataRecordValue, time)) + assertEquals(s"Unknown group metadata version ${unSupportedVersion}", e.getMessage) + } + + @Test + def testCurrentStateTSForAllGroupMetadataVersion(): Unit = { + val generation = 1 + val protocol = "range" + val memberId = "memberId" + + for (apiVersion <- ApiVersion.allVersions) { + val groupMetadataRecord = buildStableGroupRecordWithMember(generation, protocolType, protocol, memberId, apiVersion = apiVersion) + + val deserializedGroupMetadata = GroupMetadataManager.readGroupMessageValue(groupId, groupMetadataRecord.value(), time) + // GROUP_METADATA_VALUE_SCHEMA_V2 or higher should correctly set the currentStateTimestamp + if (apiVersion >= KAFKA_2_1_IV0) + assertEquals(time.milliseconds(), deserializedGroupMetadata.currentStateTimestamp.get) Review comment: Correct! Also, after your question, I tried and found the message when test failed will be: ``` kafka.coordinator.group.GroupMetadataManagerTest > testCurrentStateTimestampForAllGroupMetadataVersions FAILED java.util.NoSuchElementException: None.get ``` It's not clear and not helpful for debugging. So I added failed message and use `getOrElse(-1)` to get an unexpected timestamp. Now, the failed message will be: ``` kafka.coordinator.group.GroupMetadataManagerTest > testCurrentStateTimestampForAllGroupMetadataVersions FAILED java.lang.AssertionError: the apiVersion 2.3-IV0 doesn't set the currentStateTimestamp correctly. expected:<1599726297785> but was:<-1> ``` It'll be more clear to show which version cause this error and what error it is. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org