reddycharan commented on a change in pull request #1883: Enhance
EnsemblePlacementPolicy and DNSResolverDecorator to log relevant metrics.
URL: https://github.com/apache/bookkeeper/pull/1883#discussion_r246826556
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookieWatcherImpl.java
##########
@@ -213,19 +223,33 @@ public void initialBlockingBookieRead() throws
BKException {
int ackQuorumSize, Map<String, byte[]> customMetadata)
throws BKNotEnoughBookiesException {
long startTime = MathUtils.nowInNano();
+ Pair<List<BookieSocketAddress>, Boolean> newEnsembleResponse;
List<BookieSocketAddress> socketAddresses;
+ boolean isEnsembleAdheringToPlacementPolicy = false;
try {
- socketAddresses = placementPolicy.newEnsemble(ensembleSize,
+ newEnsembleResponse = placementPolicy.newEnsemble(ensembleSize,
writeQuorumSize, ackQuorumSize, customMetadata, new
HashSet<BookieSocketAddress>(
quarantinedBookies.asMap().keySet()));
+ socketAddresses = newEnsembleResponse.getLeft();
+ isEnsembleAdheringToPlacementPolicy =
newEnsembleResponse.getRight();
+ if (!isEnsembleAdheringToPlacementPolicy) {
+ newEnsembleNotAdheringToPlacementPolicy.inc();
+ log.warn("New ensemble: {} is not adhering to Placement
Policy", socketAddresses);
+ }
Review comment:
I would see methods of BookieWatcher / BookieWatcherImpl - newEnsemble,
replaceBookie as the interface methods for BookKeeper client operations and it
is appropriate to deal with metrics and log lines here rather than in
EnsemblePlacementPolicy classes for few reasons
- first BookieWatcher / BookieWatcherImpl methods have more context, so
having log line with appropriate info. along with metric statement would be
helpful in diagnosing/debugging any noticed issue.
- it would make things simpler and easier to have metric and log line in
these methods instead of having similar metric and log line in all of the
EnsemblePlacementPolicy implementations and in several overloaded methods in
EnsemblePlacementPolicy implementation classes.
- moreover I don't see advantage in moving these metrics and log lines down
the stack, especially considering the complexity of each
EnsemblePlacementPolicy implementations
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
