You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "leaf-soba (via GitHub)" <gi...@apache.org> on 2025/02/17 02:14:12 UTC
[PR] KAFKA-18332: fix ClassDataAbstractionCoupling problem in KafkaRaftClientTest [kafka]
leaf-soba opened a new pull request, #18926:
URL: https://github.com/apache/kafka/pull/18926
- extract a unit test named `KafkaRaftClientClusterAuthTest` to reduce the number of imported class
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18332: fix ClassDataAbstractionCoupling problem in KafkaRaftClientTest(1/2) [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #18926:
URL: https://github.com/apache/kafka/pull/18926#issuecomment-2677350104
A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the `triage` label
should be removed to prevent this automation from happening again.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18332: fix ClassDataAbstractionCoupling problem in KafkaRaftClientTest(1/2) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18926:
URL: https://github.com/apache/kafka/pull/18926#discussion_r1976738391
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java:
##########
@@ -359,7 +356,10 @@ public void testInitializeAsResignedLeaderFromStateStore(boolean withKip853Rpc)
assertEquals(0L, context.log.endOffset().offset());
context.assertElectedLeader(epoch, localId);
context.client.poll();
- assertThrows(NotLeaderException.class, () -> context.client.prepareAppend(epoch, Arrays.asList("a", "b")));
+ // Only one method invocation is expected when testing runtime exceptions
Review Comment:
Pardon me, I don't catch the point of this change. Is it a refactor about code style?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18332: fix ClassDataAbstractionCoupling problem in KafkaRaftClientTest(1/2) [kafka]
Posted by "leaf-soba (via GitHub)" <gi...@apache.org>.
leaf-soba commented on code in PR #18926:
URL: https://github.com/apache/kafka/pull/18926#discussion_r1998286042
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java:
##########
@@ -920,10 +917,10 @@ public void testInitializeAsUnattachedAndBecomeLeader(boolean withKip853Rpc) thr
RecordBatch batch = records.batches().iterator().next();
assertTrue(batch.isControlBatch());
- Record record = batch.iterator().next();
- assertEquals(electionTimestamp, record.timestamp());
+ Record expectedRecord = batch.iterator().next();
Review Comment:
@chia7712, I believe restricted identifiers should be avoided as variable names. While the code compiles, using such names can make it less readable and more confusing. For example:
```
var var = "var";
record Data(int id, String name) {} //record is used to define record classes
```
Renaming it to `expectedRecord` improves clarity and makes the intent of the variable more explicit.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18332: fix ClassDataAbstractionCoupling problem in KafkaRaftClientTest(1/2) [kafka]
Posted by "leaf-soba (via GitHub)" <gi...@apache.org>.
leaf-soba commented on code in PR #18926:
URL: https://github.com/apache/kafka/pull/18926#discussion_r1976937260
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java:
##########
@@ -359,7 +356,10 @@ public void testInitializeAsResignedLeaderFromStateStore(boolean withKip853Rpc)
assertEquals(0L, context.log.endOffset().offset());
context.assertElectedLeader(epoch, localId);
context.client.poll();
- assertThrows(NotLeaderException.class, () -> context.client.prepareAppend(epoch, Arrays.asList("a", "b")));
+ // Only one method invocation is expected when testing runtime exceptions
Review Comment:
Thank you for your comment.
the original code:
```
assertThrows(NotLeaderException.class, () -> context.client.prepareAppend(epoch, Arrays.asList("a", "b")));
```
In this case, there are two method calls inside `assertThrows`:
1. `prepareAppend`
2. `Arrays.asList`
I believe it’s generally a good practice to keep the tested code as clear as possible by avoiding multiple method calls within `assertThrows`. While `Arrays.asList` would never throw a `NotLeaderException`, separating it slightly improves the readability of the test.
I understand if this change is unnecessary. Please let me know if you’d prefer me to revert it.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18332: fix ClassDataAbstractionCoupling problem in KafkaRaftClientTest(1/2) [kafka]
Posted by "leaf-soba (via GitHub)" <gi...@apache.org>.
leaf-soba commented on PR #18926:
URL: https://github.com/apache/kafka/pull/18926#issuecomment-2699635983
> @leaf-soba could you please fix the build error?
@chia7712, I have fixed the build error, but there is still one failing unit test: testDescribeQuorumRequestToControllers. I ran it locally, and it passed without any issues, so I'm not sure how to resolve this. Could you provide any hints?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18332: fix ClassDataAbstractionCoupling problem in KafkaRaftClientTest(1/2) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #18926:
URL: https://github.com/apache/kafka/pull/18926#issuecomment-2766880184
@leaf-soba do you plan to file a follow-up to fix `ClassFanOutComplexity`?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18332: fix ClassDataAbstractionCoupling problem in KafkaRaftClientTest(1/2) [kafka]
Posted by "leaf-soba (via GitHub)" <gi...@apache.org>.
leaf-soba commented on PR #18926:
URL: https://github.com/apache/kafka/pull/18926#issuecomment-2771040071
@chia7712 yes I want to fix whole issue.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18332: fix ClassDataAbstractionCoupling problem in KafkaRaftClientTest(1/2) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18926:
URL: https://github.com/apache/kafka/pull/18926#discussion_r1976941671
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java:
##########
@@ -359,7 +356,10 @@ public void testInitializeAsResignedLeaderFromStateStore(boolean withKip853Rpc)
assertEquals(0L, context.log.endOffset().offset());
context.assertElectedLeader(epoch, localId);
context.client.poll();
- assertThrows(NotLeaderException.class, () -> context.client.prepareAppend(epoch, Arrays.asList("a", "b")));
+ // Only one method invocation is expected when testing runtime exceptions
Review Comment:
Thank you for sharing all of this. We prefer to keep this PR focused on fixing only what's necessary. Other improvements can be addressed in a follow-up if needed
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18332: fix ClassDataAbstractionCoupling problem in KafkaRaftClientTest(1/2) [kafka]
Posted by "leaf-soba (via GitHub)" <gi...@apache.org>.
leaf-soba commented on code in PR #18926:
URL: https://github.com/apache/kafka/pull/18926#discussion_r1998298448
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java:
##########
@@ -4030,24 +4027,17 @@ public void testMetrics(boolean withKip853Rpc) throws Exception {
.build();
context.pollUntil(() -> context.log.endOffset().offset() == 1L);
- assertNotNull(getMetric(context.metrics, "current-state"));
- assertNotNull(getMetric(context.metrics, "current-leader"));
- assertNotNull(getMetric(context.metrics, "current-vote"));
- assertNotNull(getMetric(context.metrics, "current-epoch"));
- assertNotNull(getMetric(context.metrics, "high-watermark"));
- assertNotNull(getMetric(context.metrics, "log-end-offset"));
- assertNotNull(getMetric(context.metrics, "log-end-epoch"));
- assertNotNull(getMetric(context.metrics, "number-unknown-voter-connections"));
- assertNotNull(getMetric(context.metrics, "poll-idle-ratio-avg"));
- assertNotNull(getMetric(context.metrics, "commit-latency-avg"));
- assertNotNull(getMetric(context.metrics, "commit-latency-max"));
- assertNotNull(getMetric(context.metrics, "election-latency-avg"));
- assertNotNull(getMetric(context.metrics, "election-latency-max"));
- assertNotNull(getMetric(context.metrics, "fetch-records-rate"));
- assertNotNull(getMetric(context.metrics, "append-records-rate"));
- assertNotNull(getMetric(context.metrics, "number-of-voters"));
- assertNotNull(getMetric(context.metrics, "number-of-observers"));
- assertNotNull(getMetric(context.metrics, "uncommitted-voter-change"));
+ List<String> metricNames = Arrays.asList(
Review Comment:
@chia7712, Thanks for the suggestion! I've learned that using `var` and `Set` improves performance and ensures immutability.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18332: fix ClassDataAbstractionCoupling problem in KafkaRaftClientTest(1/2) [kafka]
Posted by "leaf-soba (via GitHub)" <gi...@apache.org>.
leaf-soba commented on PR #18926:
URL: https://github.com/apache/kafka/pull/18926#issuecomment-2670367999
Hi @ijuma, @chia7712, @gongxuanzhang, could you please take a look?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18332: fix ClassDataAbstractionCoupling problem in KafkaRaftClientTest(1/2) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #18926:
URL: https://github.com/apache/kafka/pull/18926
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18332: fix ClassDataAbstractionCoupling problem in KafkaRaftClientTest(1/2) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #18926:
URL: https://github.com/apache/kafka/pull/18926#issuecomment-2694325084
@leaf-soba could you please fix the build error?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18332: fix ClassDataAbstractionCoupling problem in KafkaRaftClientTest(1/2) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18926:
URL: https://github.com/apache/kafka/pull/18926#discussion_r1976306426
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java:
##########
@@ -4641,15 +4533,15 @@ public void testHandleLeaderChangeFiresAfterResignRegistration(boolean withKip85
@ParameterizedTest
@ValueSource(booleans = { true, false })
- public void testObserverFetchWithNoLocalId(boolean withKip853Rpc) throws Exception {
Review Comment:
could you please revert those unrelated changes?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18332: fix ClassDataAbstractionCoupling problem in KafkaRaftClientTest(1/2) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18926:
URL: https://github.com/apache/kafka/pull/18926#discussion_r1996245505
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java:
##########
@@ -920,10 +917,10 @@ public void testInitializeAsUnattachedAndBecomeLeader(boolean withKip853Rpc) thr
RecordBatch batch = records.batches().iterator().next();
assertTrue(batch.isControlBatch());
- Record record = batch.iterator().next();
- assertEquals(electionTimestamp, record.timestamp());
+ Record expectedRecord = batch.iterator().next();
Review Comment:
Out of curiosity, what are the specific benefits associated with renaming it?
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java:
##########
@@ -969,10 +966,10 @@ public void testInitializeAsCandidateAndBecomeLeaderQuorumOfThree(boolean withKi
RecordBatch batch = records.batches().iterator().next();
assertTrue(batch.isControlBatch());
- Record record = batch.iterator().next();
- assertEquals(electionTimestamp, record.timestamp());
+ Record expectedRecord = batch.iterator().next();
Review Comment:
ditto
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java:
##########
@@ -4030,24 +4027,17 @@ public void testMetrics(boolean withKip853Rpc) throws Exception {
.build();
context.pollUntil(() -> context.log.endOffset().offset() == 1L);
- assertNotNull(getMetric(context.metrics, "current-state"));
- assertNotNull(getMetric(context.metrics, "current-leader"));
- assertNotNull(getMetric(context.metrics, "current-vote"));
- assertNotNull(getMetric(context.metrics, "current-epoch"));
- assertNotNull(getMetric(context.metrics, "high-watermark"));
- assertNotNull(getMetric(context.metrics, "log-end-offset"));
- assertNotNull(getMetric(context.metrics, "log-end-epoch"));
- assertNotNull(getMetric(context.metrics, "number-unknown-voter-connections"));
- assertNotNull(getMetric(context.metrics, "poll-idle-ratio-avg"));
- assertNotNull(getMetric(context.metrics, "commit-latency-avg"));
- assertNotNull(getMetric(context.metrics, "commit-latency-max"));
- assertNotNull(getMetric(context.metrics, "election-latency-avg"));
- assertNotNull(getMetric(context.metrics, "election-latency-max"));
- assertNotNull(getMetric(context.metrics, "fetch-records-rate"));
- assertNotNull(getMetric(context.metrics, "append-records-rate"));
- assertNotNull(getMetric(context.metrics, "number-of-voters"));
- assertNotNull(getMetric(context.metrics, "number-of-observers"));
- assertNotNull(getMetric(context.metrics, "uncommitted-voter-change"));
+ List<String> metricNames = Arrays.asList(
Review Comment:
```
var metricNames = Set.of(
"current-state", "current-leader", "current-vote", "current-epoch", "high-watermark",
"log-end-offset", "log-end-epoch", "number-unknown-voter-connections", "poll-idle-ratio-avg",
"commit-latency-avg", "commit-latency-max", "election-latency-avg", "election-latency-max",
"fetch-records-rate", "append-records-rate", "number-of-voters", "number-of-observers",
"uncommitted-voter-change"
);
```
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java:
##########
@@ -3968,10 +3965,10 @@ public void testLeaderAppendSingleMemberQuorum(boolean withKip853Rpc) throws Exc
List<Record> readRecords = Utils.toList(leaderChangeBatch.iterator());
assertEquals(1, readRecords.size());
- Record record = readRecords.get(0);
- assertEquals(now, record.timestamp());
+ Record expectedRecord = readRecords.get(0);
Review Comment:
ditto
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18332: fix ClassDataAbstractionCoupling problem in KafkaRaftClientTest(1/2) [kafka]
Posted by "leaf-soba (via GitHub)" <gi...@apache.org>.
leaf-soba commented on code in PR #18926:
URL: https://github.com/apache/kafka/pull/18926#discussion_r1976365066
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java:
##########
@@ -4641,15 +4533,15 @@ public void testHandleLeaderChangeFiresAfterResignRegistration(boolean withKip85
@ParameterizedTest
@ValueSource(booleans = { true, false })
- public void testObserverFetchWithNoLocalId(boolean withKip853Rpc) throws Exception {
Review Comment:
I've reverted the unrelated changes. Let me know if there's anything else that needs adjustment. Thanks!
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18332: fix ClassDataAbstractionCoupling problem in KafkaRaftClientTest(1/2) [kafka]
Posted by "leaf-soba (via GitHub)" <gi...@apache.org>.
leaf-soba commented on code in PR #18926:
URL: https://github.com/apache/kafka/pull/18926#discussion_r1976979280
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java:
##########
@@ -359,7 +356,10 @@ public void testInitializeAsResignedLeaderFromStateStore(boolean withKip853Rpc)
assertEquals(0L, context.log.endOffset().offset());
context.assertElectedLeader(epoch, localId);
context.client.poll();
- assertThrows(NotLeaderException.class, () -> context.client.prepareAppend(epoch, Arrays.asList("a", "b")));
+ // Only one method invocation is expected when testing runtime exceptions
Review Comment:
@chia7712 Thank you for your feedback! I have reverted the unnecessary change related to throwing two exceptions in one assertThrows.
There are still some other minor improvements—would you prefer that I revert all of them in this PR, or should I keep them and address them in a follow-up PR instead? I'm happy to proceed in whichever way aligns best with the project's approach.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org