[improve][broker] PIP-192 Added operation counters in ServiceUnitStateChannelImpl #19410
Conversation
Demogorgon314
requested review from
eolivelli,
shibd,
Demogorgon314,
BewareMyPower,
codelipenghui,
gaoran10 and
Technoboy-
| } | ||
|
|
||
| private void handle(String serviceUnit, ServiceUnitStateData data) { | ||
| long totalHandledRequests = getHandlerTotalCounter(data).incrementAndGet(); |
There was a problem hiding this comment.
Do we need to record all events for all brokers? Or record event metrics only for the target broker.
There was a problem hiding this comment.
Yes, we want to record all events for all brokers.
The goal is to monitor the consistency of the tableviews across all brokers.
We need to monitor if the rate of the increase of HandlerTotalCounters( increase / x sec) is the same across all brokers.
There was a problem hiding this comment.
Ok, thanks for your explanation.
...ava/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java
Outdated
Show resolved
Hide resolved
| } catch (Throwable e) { | ||
| log.error("Failed to publish assign event. serviceUnit:{}, broker:{}, assignPublishFailureCount:{}", | ||
| serviceUnit, broker, eventCounters.get(eventType).getFailure().incrementAndGet(), e); | ||
| throw e; |
There was a problem hiding this comment.
It seems that we need to return to an exceptional future. There are some similar methods.
There was a problem hiding this comment.
I removed this exception handling by returning an exceptional future from validateChannelState(). This outer try-catch is no longer required.
| long handlerTotalCount = getHandlerTotalCounter(data).get(); | ||
| long handlerFailureCount = getHandlerFailureCounter(data).get(); | ||
| log.info("{} handled {} event for serviceUnit:{}, cur:{}, next:{}, " | ||
| + "totalHandledRequests{}, totalFailedRequests:{}", |
There was a problem hiding this comment.
Seems the log level is debug
There was a problem hiding this comment.
This is meant for info. If this is a transfer command, we want to log it at the info level.
In other PRs, I will clean the debug logs based on isLoadBalancerDebugModeEnabled.
private boolean debug() {
return pulsar.getConfiguration().isLoadBalancerDebugModeEnabled() || log.isDebugEnabled();
}
heesung-sn
force-pushed
the
pip-192-bsc-op-metrics
branch
from
659074e to
c053fc9
Compare
Codecov Report
@@ Coverage Diff @@
## master #19410 +/- ##
============================================
+ Coverage 62.58% 63.61% +1.02%
- Complexity 3471 26088 +22617
============================================
Files 1829 1832 +3
Lines 133919 134124 +205
Branches 14733 14756 +23
============================================
+ Hits 83812 85318 +1506
+ Misses 42384 40976 -1408
- Partials 7723 7830 +107
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Master Issue: #16691
Motivation
We will start raising PRs to implement PIP-192, #16691
Modifications
This PR added operation counters in ServiceUnitStateChannelImpl and its unit test.
Verifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeWe will have separate PRs to update the Doc later.
Matching PR in forked repository
PR in forked repository: heesung-sn#27