heesung-sn commented on code in PR #21854:
URL: https://github.com/apache/pulsar/pull/21854#discussion_r1443185232
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/manager/UnloadManager.java:
##########
@@ -38,13 +41,75 @@ public class UnloadManager implements StateChangeListener {
private final UnloadCounter counter;
private final Map<String, CompletableFuture<Void>> inFlightUnloadRequest;
+ private final String lookupServiceAddress;
- public UnloadManager(UnloadCounter counter) {
+ private enum LatencyMetric {
+ UNLOAD(buildHistogram(
+ "brk_lb_unload_latency", "Total time duration of unload operations
on source brokers"), true, false),
+ ASSIGN(buildHistogram(
+ "brk_lb_assign_latency", "Time spent in the load balancing ASSIGN
state on source brokers"), true, false),
Review Comment:
should these be (false, true) and "Time spent in the load balancing ASSIGN
state on destination brokers"?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##########
@@ -789,15 +789,19 @@ private void handleOwnEvent(String serviceUnit,
ServiceUnitStateData data) {
}
private void handleAssignEvent(String serviceUnit, ServiceUnitStateData
data) {
+ stateChangeListeners.notifyOnArrival(serviceUnit, data);
if (isTargetBroker(data.dstBroker())) {
ServiceUnitStateData next = new ServiceUnitStateData(
Owned, data.dstBroker(), data.sourceBroker(),
getNextVersionId(data));
stateChangeListeners.notifyOnCompletion(pubAsync(serviceUnit,
next), serviceUnit, data)
.whenComplete((__, e) -> log(e, serviceUnit, data, next));
+ } else if (isTargetBroker(data.sourceBroker())) {
Review Comment:
can we remove this as the source broker is not an actor for this state?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##########
@@ -813,6 +817,8 @@ private void handleReleaseEvent(String serviceUnit,
ServiceUnitStateData data) {
stateChangeListeners.notifyOnCompletion(unloadFuture
.thenCompose(__ -> pubAsync(serviceUnit, next)),
serviceUnit, data)
.whenComplete((__, e) -> log(e, serviceUnit, data, next));
+ } else if (isTargetBroker(data.dstBroker())) {
Review Comment:
can we remove this as the dst broker is not an actor for this state?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/manager/UnloadManager.java:
##########
@@ -38,13 +41,75 @@ public class UnloadManager implements StateChangeListener {
private final UnloadCounter counter;
private final Map<String, CompletableFuture<Void>> inFlightUnloadRequest;
+ private final String lookupServiceAddress;
- public UnloadManager(UnloadCounter counter) {
+ private enum LatencyMetric {
+ UNLOAD(buildHistogram(
+ "brk_lb_unload_latency", "Total time duration of unload operations
on source brokers"), true, false),
+ ASSIGN(buildHistogram(
+ "brk_lb_assign_latency", "Time spent in the load balancing ASSIGN
state on source brokers"), true, false),
+ RELEASE(buildHistogram(
+ "brk_lb_release_latency", "Time spent in the load balancing
RELEASE state on source brokers"), true, false),
+ DISCONNECT(buildHistogram(
+ "brk_lb_owned_latency", "Time spent in the load balancing
disconnected state on destination brokers"),
Review Comment:
- Should these be (true, false) and "Time spent in the load balancing
disconnected state on source brokers"?
- can we rename this metric to `brk_lb_disconnect_latency` to be consistent?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/StateChangeListeners.java:
##########
@@ -54,6 +54,17 @@ public <T> CompletableFuture<T>
notifyOnCompletion(CompletableFuture<T> future,
return future.whenComplete((r, ex) -> notify(serviceUnit, data, ex));
}
+ public void notifyOnArrival(String serviceUnit, ServiceUnitStateData data)
{
Review Comment:
Sorry. I realized that we can call this `notifyOnArrival` in the
`notifyOnCompletion` at line 53. Then, we don't need to call `notifyOnArrival`
in the ServiceUnitStateChannelImpl explicitly. (this means we can make
notifyOnArrival a private func)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
