KAFKA-18592: Cleanup ReplicaManager #18621
Conversation
github-actions
bot
added
triage
| isFromConsumer: Boolean, | ||
| fetchOnlyFromLeader: Boolean): Seq[Long] = { | ||
| val partition = getPartitionOrException(topicPartition) | ||
| partition.legacyFetchOffsetsForTimestamp(timestamp, maxNumOffsets, isFromConsumer, fetchOnlyFromLeader) |
There was a problem hiding this comment.
Could we remove it from partition also?
| def hasDelayedElectionOperations: Boolean = delayedElectLeaderPurgatory.numDelayed != 0 | ||
|
|
||
| def tryCompleteElection(key: DelayedOperationKey): Unit = { | ||
| val completed = delayedElectLeaderPurgatory.checkAndComplete(key) |
There was a problem hiding this comment.
Could we remove the purgatory too?
frankvicky
force-pushed
the
KAFKA-18592
branch
from
34db24b to
5b59263
Compare
| @@ -302,9 +302,6 @@ class ReplicaManager(val config: KafkaConfig, | |||
| new DelayedOperationPurgatory[DelayedDeleteRecords]( | |||
| "DeleteRecords", config.brokerId, | |||
| config.deleteRecordsPurgatoryPurgeIntervalRequests)) | |||
| val delayedElectLeaderPurgatory = delayedElectLeaderPurgatoryParam.getOrElse( | |||
There was a problem hiding this comment.
please remove delayedElectLeaderPurgatoryParam also
There was a problem hiding this comment.
Also, this also remove the following metrics.
- kafka.server:type=DelayedOperationPurgatory,delayedOperation=ElectLeader,name=PurgatorySize
- kafka.server:type=DelayedOperationPurgatory,delayedOperation=ElectLeader,name=NumDelayedOperations
please add them into zk2kraft.html
There was a problem hiding this comment.
Interesting, we still allow elect leader to be called. Why are these metrics no longer relevant?
There was a problem hiding this comment.
The zk broker waits for the metadata update before returning a response to the ElectLeader request, whereas the Kraft broker does not. Consequently, the Kraft broker does not need delayedOperation metrics for ElectLeader requests. Instead, kraft broker has ForwardingManager metrics to monitor the forwarded requests.
frankvicky
force-pushed
the
KAFKA-18592
branch
2 times, most recently
from
2d70136 to
f142c30
Compare
frankvicky
force-pushed
the
KAFKA-18592
branch
from
f142c30 to
a2c5502
Compare
|
Please resolve the conflicts. |
frankvicky
force-pushed
the
KAFKA-18592
branch
from
a2c5502 to
1f80a44
Compare
| isFromConsumer: Boolean, | ||
| fetchOnlyFromLeader: Boolean): Seq[Long] = inReadLock(leaderIsrUpdateLock) { | ||
| val localLog = localLogWithEpochOrThrow(Optional.empty(), fetchOnlyFromLeader) | ||
| val allOffsets = localLog.legacyFetchOffsetsBefore(timestamp, maxNumOffsets) |
There was a problem hiding this comment.
Perhaps it can be removed from LocalLog too (in a follow-up).
There was a problem hiding this comment.
You are right.
If this PR gets merged, this method would only be invoked in tests.
I will file a JIRA for this one.
There was a problem hiding this comment.
docs/zk2kraft.html
Outdated
| @@ -176,5 +176,14 @@ <h5 class="anchor-heading">Removal metrics</h5> | |||
| Kafka remove all zookeeper dependencies, so the metrics is removed. | |||
| </p> | |||
| </li> | |||
| <li> | |||
| <p> | |||
| Remove the metrics which is monitoring the leader election. | |||
There was a problem hiding this comment.
This seems misleading - I think these metrics are related to the purgatory for the ElectLeader operation - we still have metrics for monitoring leader election.
There was a problem hiding this comment.
Thanks for pointing out.
I have just updated the description. 🙇🏼
There was a problem hiding this comment.
Do we need to explain the usage for each removed metric? Since there are numerous removed metrics, I propose adding a listed item to include all of them. This was also the suggestion I made in #18654.
@ijuma @frankvicky WDYT?
There was a problem hiding this comment.
Make sense to me.
There was a problem hiding this comment.
I think it may be helpful to indicate which metrics replace the ones being removed or if they're not applicable anymore. That would be useful, I believe.
frankvicky
force-pushed
the
KAFKA-18592
branch
from
1f80a44 to
af3dff4
Compare
JIRA: KAFKA-18592 There are few methods in ReplicaManager unused now, we should remove them. # Conflicts: # docs/zk2kraft.html
| return this; | ||
| } | ||
|
|
||
| public ReplicaManagerBuilder setDelayedDeleteRecordsPurgatory(DelayedOperationPurgatory<DelayedDeleteRecords> delayedDeleteRecordsPurgatory) { |
There was a problem hiding this comment.
Is there a reason why you can't remove the field(s) associated with this setter (and others) as well? As far as I can see it is only used as an argument for creating a ReplicaManager and you could just leave the instantiation of the purgatory in the constructor of ReplicaManager. I am happy for this to be done in a follow-up as long as it has already been accounted for and it isn't just a miss.
There was a problem hiding this comment.
You are right. These fields are something we miss and also need to be clean.
I will file a jira for this one.
There was a problem hiding this comment.
frankvicky
force-pushed
the
KAFKA-18592
branch
from
af3dff4 to
76a7eee
Compare
Reviewers: Ismael Juma <[email protected]>, Christo Lolov <[email protected]>, Chia-Ping Tsai <[email protected]>
Reviewers: Ismael Juma <[email protected]>, Christo Lolov <[email protected]>, Chia-Ping Tsai <[email protected]>
JIRA: KAFKA-18592
There are a few methods in
ReplicaManagerunused now, we should remove them.Committer Checklist (excluded from commit message)