AndrewJSchofield commented on code in PR #18862:
URL: https://github.com/apache/kafka/pull/18862#discussion_r1952331697
##########
core/src/test/java/kafka/server/share/SharePartitionManagerTest.java:
##########
@@ -1035,7 +1067,7 @@ public void testSharePartitionKey() {
assertNotEquals(sharePartitionKey1, sharePartitionKey3);
assertNotEquals(sharePartitionKey1, sharePartitionKey4);
assertNotEquals(sharePartitionKey1, sharePartitionKey5);
- assertNotEquals(sharePartitionKey1, null);
+ assertNotEquals(null, sharePartitionKey1);
Review Comment:
`assertNotNull`
##########
core/src/test/java/kafka/server/share/SharePartitionManagerTest.java:
##########
@@ -407,7 +427,7 @@ public void testShareSessionExpiration() {
ShareRequestMetadata reqMetadata1 = new
ShareRequestMetadata(Uuid.randomUuid(), ShareRequestMetadata.INITIAL_EPOCH);
ShareFetchContext session1context =
sharePartitionManager.newContext(groupId, session1req, EMPTY_PART_LIST,
reqMetadata1, false);
- assertEquals(session1context.getClass(), ShareSessionContext.class);
+ assertEquals(ShareSessionContext.class, session1context.getClass());
Review Comment:
I wonder whether these assertions about class should actually be
`assertInstanceOf`.
##########
core/src/test/java/kafka/server/share/SharePartitionManagerTest.java:
##########
@@ -323,28 +340,28 @@ public void testNewContext() {
assertEquals(respData2, resp2.responseData(topicNames));
ShareSessionKey shareSessionKey2 = new ShareSessionKey(groupId,
- reqMetadata2.memberId());
+ reqMetadata2.memberId());
// Test trying to create a new session with an invalid epoch
assertThrows(InvalidShareSessionEpochException.class, () ->
sharePartitionManager.newContext(groupId, reqData2, EMPTY_PART_LIST,
- new ShareRequestMetadata(shareSessionKey2.memberId(), 5),
true));
+ new ShareRequestMetadata(shareSessionKey2.memberId(), 5), true));
// Test trying to create a new session with a non-existent session key
Uuid memberId4 = Uuid.randomUuid();
assertThrows(ShareSessionNotFoundException.class, () ->
sharePartitionManager.newContext(groupId, reqData2, EMPTY_PART_LIST,
- new ShareRequestMetadata(memberId4, 1), true));
+ new ShareRequestMetadata(memberId4, 1), true));
// Continue the first share session we created.
ShareFetchContext context5 = sharePartitionManager.newContext(groupId,
Collections.emptyMap(), EMPTY_PART_LIST,
- new ShareRequestMetadata(shareSessionKey2.memberId(), 1),
true);
+ new ShareRequestMetadata(shareSessionKey2.memberId(), 1), true);
assertEquals(ShareSessionContext.class, context5.getClass());
assertTrue(((ShareSessionContext) context5).isSubsequent());
ShareSessionContext shareSessionContext5 = (ShareSessionContext)
context5;
synchronized (shareSessionContext5.session()) {
shareSessionContext5.session().partitionMap().forEach(cachedSharePartition -> {
TopicIdPartition topicIdPartition = new
TopicIdPartition(cachedSharePartition.topicId(), new
Review Comment:
This indentation is really odd with the trailing `new`.
##########
core/src/test/java/kafka/server/share/SharePartitionManagerTest.java:
##########
@@ -180,16 +181,23 @@ public void setUp() {
@AfterEach
public void tearDown() throws Exception {
- mockTimer.close();
+ if (sharePartitionManager != null) {
+ sharePartitionManager.close();
+ } else {
+ mockTimer.close();
Review Comment:
It seems strange to me that the closing of the mockTimer would be
conditional.
--
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]
