desaikomal commented on code in PR #2553:
URL: https://github.com/apache/helix/pull/2553#discussion_r1260287707
##########
helix-core/src/main/java/org/apache/helix/controller/stages/TopStateHandoffReportStage.java:
##########
@@ -317,12 +317,18 @@ private void
reportTopStateHandoffFailIfNecessary(ResourceControllerDataProvider
String partitionName = partition.getPartitionName();
MissingTopStateRecord record =
missingTopStateMap.get(resourceName).get(partitionName);
long startTime = record.getStartTimeStamp();
- if (startTime > 0 && System.currentTimeMillis() - startTime >
durationThreshold && !record
- .isFailed()) {
+ long missingDuration = System.currentTimeMillis() - startTime;
+ if (startTime > 0 && missingDuration > durationThreshold &&
!record.isFailed()) {
record.setFailed();
missingTopStateMap.get(resourceName).put(partitionName, record);
+ // Since top state handoff has not completed yet we can't log helix top
state latency but can log since how long
+ // top state is missing.
+ LogUtil.logInfo(LOG, _eventId, String.format(
Review Comment:
If i understand correctly, this is what ESPRESSO wants it.
##########
helix-core/src/main/java/org/apache/helix/controller/stages/TopStateHandoffReportStage.java:
##########
@@ -469,16 +475,17 @@ private void
reportTopStateComesBack(ResourceControllerDataProvider cache, Map<S
}
}
+ long duration = handOffEndTime - handOffStartTime;
+ long helixLatency = duration - fromTopStateUserLatency -
toTopStateUserLatency;
+ // It is possible that during controller leader switch, we lost previous
master information
+ // and use current time to approximate missing top state start time. If we
see the actual
+ // user latency is larger than the duration we estimated, we use user
latency to start with
+ duration = Math.max(duration, helixLatency);
+ boolean isGraceful = record.isGracefulHandoff();
+ logMissingTopStateInfo(duration, helixLatency, isGraceful,
partition.getPartitionName());
Review Comment:
the if condition below says: ( handOffEndTime - handOffStartTime <=
threshold )
- which is not ESPRESSO ask, so why are we moving the message
unconditionally? if it is for improving debuggability, then it will flood as
it will log for everything which is missing top state and especially when
instance goes down, it will definately cause lot more messages.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
