-
Notifications
You must be signed in to change notification settings - Fork 65
[LI-HOTFIX] Improving performance of the isReplicaOnline method #206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -225,14 +225,6 @@ class ControllerContext { | |
| }.toSet | ||
| } | ||
|
|
||
| def isReplicaOnline(brokerId: Int, topicPartition: TopicPartition, includeShuttingDownBrokers: Boolean = false): Boolean = { | ||
| val brokerOnline = { | ||
| if (includeShuttingDownBrokers) liveOrShuttingDownBrokerIds.contains(brokerId) | ||
| else liveBrokerIds.contains(brokerId) | ||
| } | ||
| brokerOnline && !replicasOnOfflineDirs.getOrElse(brokerId, Set.empty).contains(topicPartition) | ||
| } | ||
|
|
||
| def replicasOnBrokers(brokerIds: Set[Int]): Set[PartitionAndReplica] = { | ||
| brokerIds.flatMap { brokerId => | ||
| partitionAssignments.flatMap { | ||
|
|
@@ -257,8 +249,9 @@ class ControllerContext { | |
| } | ||
|
|
||
| def allLiveReplicas(): Set[PartitionAndReplica] = { | ||
| val snapshot = ControllerContextSnapshot(this) | ||
| replicasOnBrokers(liveBrokerIds).filter { partitionAndReplica => | ||
| isReplicaOnline(partitionAndReplica.replica, partitionAndReplica.topicPartition) | ||
| snapshot.isReplicaOnline(partitionAndReplica.replica, partitionAndReplica.topicPartition) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -270,12 +263,13 @@ class ControllerContext { | |
| def onlineAndOfflineReplicas: (Set[PartitionAndReplica], Set[PartitionAndReplica]) = { | ||
| val onlineReplicas = mutable.Set.empty[PartitionAndReplica] | ||
| val offlineReplicas = mutable.Set.empty[PartitionAndReplica] | ||
| val snapshot = ControllerContextSnapshot(this) | ||
| for ((topic, partitionAssignments) <- partitionAssignments; | ||
| (partitionId, assignment) <- partitionAssignments) { | ||
| val partition = new TopicPartition(topic, partitionId) | ||
| for (replica <- assignment.replicas) { | ||
| val partitionAndReplica = PartitionAndReplica(partition, replica) | ||
| if (isReplicaOnline(replica, partition)) | ||
| if (snapshot.isReplicaOnline(replica, partition)) | ||
| onlineReplicas.add(partitionAndReplica) | ||
| else | ||
| offlineReplicas.add(partitionAndReplica) | ||
|
|
@@ -431,3 +425,22 @@ class ControllerContext { | |
| targetState.validPreviousStates.contains(partitionStates(partition)) | ||
|
|
||
| } | ||
|
|
||
| /** | ||
| * The ControllerContextSnapshot is an immutable snapshot of the ControllorContext. | ||
|
gitlw marked this conversation as resolved.
Outdated
|
||
| * The motivation for this class is that we don't need to calculate certain fields | ||
|
lmr3796 marked this conversation as resolved.
Outdated
|
||
| * repeatedly, including liveBrokerIds and liveOrShuttingDownBrokerIds. | ||
| */ | ||
| case class ControllerContextSnapshot(controllerContext: ControllerContext) { | ||
| val liveBrokerIds = controllerContext.liveBrokerIds | ||
| val liveOrShuttingDownBrokerIds = controllerContext.liveOrShuttingDownBrokerIds | ||
| val replicasOnOfflineDirs = controllerContext.replicasOnOfflineDirs | ||
|
Comment on lines
+435
to
+437
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So all these fields are computed? It's not clear because Scala is hiding method call vs field dereference.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The first 2 are method calls, and the last one |
||
|
|
||
| def isReplicaOnline(brokerId: Int, topicPartition: TopicPartition, includeShuttingDownBrokers: Boolean = false): Boolean = { | ||
| val brokerOnline = { | ||
| if (includeShuttingDownBrokers) liveOrShuttingDownBrokerIds.contains(brokerId) | ||
| else liveBrokerIds.contains(brokerId) | ||
| } | ||
| brokerOnline && !replicasOnOfflineDirs.getOrElse(brokerId, Set.empty).contains(topicPartition) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This caches the computed fields inside ControllerContextSnapshot so that they don't need to be repeatedly computed.