MINOR: Refactor kafka.cluster.Replica#12081
Conversation
| @@ -0,0 +1,132 @@ | |||
| /* | |||
There was a problem hiding this comment.
I have renamed this file. Those tests have nothing to do with the Replica. I have to check if they could be relocated to another place, perhaps in UnifiedLogTest. Do you mind if I do this as a follow up? I need this PR for the next one.
There was a problem hiding this comment.
I'm ok with that, but it might not be as much work as you imagine: UnifiedLogTest has a pretty minimalistic setup.
There was a problem hiding this comment.
Yeah, you're right. I just did it and it was simpler than what I though.
hachikuji
left a comment
There was a problem hiding this comment.
Thanks, nice cleanup. Left a few minor comments.
| @@ -0,0 +1,132 @@ | |||
| /* | |||
There was a problem hiding this comment.
I'm ok with that, but it might not be as much work as you imagine: UnifiedLogTest has a pretty minimalistic setup.
| * @return true if the HW was incremented, and false otherwise. | ||
| */ | ||
| private def maybeIncrementLeaderHW(leaderLog: UnifiedLog, curTime: Long = time.milliseconds): Boolean = { | ||
| private def maybeIncrementLeaderHW(leaderLog: UnifiedLog, currentTimeMs: Long = time.milliseconds): Boolean = { |
There was a problem hiding this comment.
Do we need the default argument for currentTimeMs? Looks like we have already computed the time in makeLeader.
There was a problem hiding this comment.
We have a few other calls where currentTimeMs is not computed beforehand. I will update makeLeader to pass it directly.
| ): Unit = { | ||
| replicaState.updateAndGet { currentReplicaState => | ||
| // When the leader is elected or re-elected, the follower's last caught up time | ||
| // is set to the current time if the follower is in the ISR, else to 0. The later |
|
@hachikuji Thanks for your review. I have addressed your comments. |
This patch refactors
kafka.cluster.Replica, it usages and tests. This is part of the work in KAFKA-13790.Committer Checklist (excluded from commit message)