KAFKA-18332: fix ClassDataAbstractionCoupling problem in KafkaRaftClientTest(1/2) #18926
Conversation
remove unnecessary public
remove unnecessary `toString`
checked all changed list in this class are immutable
extra the method in the assertThrows
rename record to expectedRecord
use a loop instead of repeated code
extract a unit test named KafkaRaftClientClusterAuthTest to reduce the number of imported class
|
Hi @ijuma, @chia7712, @gongxuanzhang, could you please take a look? |
|
A label of 'needs-attention' was automatically added to this PR in order to raise the |
There was a problem hiding this comment.
@leaf-soba thanks for this improvement. one small comment is left
| @@ -4641,15 +4533,15 @@ public void testHandleLeaderChangeFiresAfterResignRegistration(boolean withKip85 | |||
|
|
|||
| @ParameterizedTest | |||
| @ValueSource(booleans = { true, false }) | |||
| public void testObserverFetchWithNoLocalId(boolean withKip853Rpc) throws Exception { | |||
There was a problem hiding this comment.
could you please revert those unrelated changes?
There was a problem hiding this comment.
I've reverted the unrelated changes. Let me know if there's anything else that needs adjustment. Thanks!
revert remove unnecessary public
| @@ -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 | |||
There was a problem hiding this comment.
Pardon me, I don't catch the point of this change. Is it a refactor about code style?
There was a problem hiding this 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:
prepareAppendArrays.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.
There was a problem hiding this 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
There was a problem hiding this 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.
revert extra the method in the assertThrows
|
@leaf-soba could you please fix the build error? |
change the import order
@chia7712, I have fixed the build error, but there is still one failing unit test: |
There was a problem hiding this comment.
@leaf-soba thanks for this patch. please take a look at a couple of minor comments.
| @@ -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(); | |||
There was a problem hiding this comment.
Out of curiosity, what are the specific benefits associated with renaming it?
There was a problem hiding this 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.
| @@ -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(); | |||
| @@ -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); | |||
| 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( |
There was a problem hiding this 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"
);
There was a problem hiding this comment.
@chia7712, Thanks for the suggestion! I've learned that using var and Set improves performance and ensures immutability.
use var instead of list
|
@leaf-soba do you plan to file a follow-up to fix |
|
@chia7712 yes I want to fix whole issue. |
fix merge error
…ntTest(1/2) (apache#18926) - extract a unit test named `KafkaRaftClientClusterAuthTest` to reduce the number of imported class Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
KafkaRaftClientClusterAuthTestto reduce the number of imported classReviewers: Chia-Ping Tsai chia7712@gmail.com