KAFKA-8333; Cache checkpointed high watermarks for reuse on LeaderAndIsr request#6696
KAFKA-8333; Cache checkpointed high watermarks for reuse on LeaderAndIsr request#6696hachikuji wants to merge 1 commit into
Conversation
2004b6c to
885d7ae
Compare
mumrah
left a comment
mumrah
left a comment
There was a problem hiding this comment.
Looks good all around. Abstracting ZK, removing ReplicaManager and isOffline from Partition, this will definitely help reduce mocking in tests. I'm always a fan of polymorphism over endless conditional checks scattered everywhere (e.g., if (!isOffline)). Only a few comments in-line.
| private val tags = Map("topic" -> topic, "partition" -> partitionId.toString) | ||
|
|
||
| // Do not create metrics if this partition is ReplicaManager.OfflinePartition | ||
| if (!isOffline) { |
There was a problem hiding this comment.
So we're able to remove the isOffline flag here due to the new OfflinePartition in ReplicaManager (which does not include a Partition instance)?
There was a problem hiding this comment.
Yes, this is something I've been meaning to do for quite a while. The sentinel we had before is just a bad practice.
| } | ||
| } | ||
|
|
||
| // Visible for testing |
There was a problem hiding this comment.
Do we have an annotation like Guava's @VisibleForTesting? I always preferred that to comments
| def makeLeader(controllerId: Int, | ||
| partitionStateInfo: LeaderAndIsrRequest.PartitionState, | ||
| correlationId: Int, | ||
| highWatermarkCheckpoint: LazyOffsetCheckpoints): Boolean = { |
There was a problem hiding this comment.
Should we add a simple trait for LazyOffsetCheckpoints? I see that it gets used in lots of method calls, but would probably simplify testing it we had an interface to provide fetch(String, TopicPartition): Option[Long]
| } | ||
| private def shrinkIsr(newIsr: Set[Replica]): Unit = { | ||
| val newLeaderAndIsr = new LeaderAndIsr(localBrokerId, leaderEpoch, newIsr.map(_.brokerId).toList, zkVersion) | ||
| val zkVersionOpt = stateStore.shrinkIsr(controllerEpoch, newLeaderAndIsr) |
There was a problem hiding this comment.
Drop the "zk" name here? (since stateStore is now abstracted from ZK)
|
@mumrah Thanks for the review. I think I am going to try and separate the refactor into another patch to make review a bit easier. |
|
Going to close this and open a new PR since #6705 was merged. |
2 participants
Currently we load the high watermark checkpoint file separately for every partition that is loaded on a broker. This patch adds logic to load the checkpoint file only once when a LeaderAndIsr request is received. The results are reused for all partitions included in the request.
Additionally, this patch removes the dependence on
ReplicaManagerinsidePartition, which should make testing easier. I have also simplified the future replica loading logic since it was unnecessarily loading the checkpoint file.Committer Checklist (excluded from commit message)