navina commented on code in PR #9244:
URL: https://github.com/apache/pinot/pull/9244#discussion_r950603573
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java:
##########
@@ -1440,6 +1440,17 @@ private void fetchLatestStreamOffset() {
}
}
+ public StreamPartitionMsgOffset fetchLatestStreamOffset(Long maxWaitTimeMs) {
Review Comment:
Looks like there is already a public method called
`getLatestStreamOffsetAtStartupTime` in `LLRealtimeSegmentDataManager`. This is
also used by the offset checker. I think it is the same information that you
are looking for.
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java:
##########
@@ -236,12 +236,24 @@ private void registerServiceStatusHandler() {
boolean isOffsetBasedConsumptionStatusCheckerEnabled =
_serverConf.getProperty(Server.CONFIG_OF_ENABLE_REALTIME_OFFSET_BASED_CONSUMPTION_STATUS_CHECKER,
Server.DEFAULT_ENABLE_REALTIME_OFFSET_BASED_CONSUMPTION_STATUS_CHECKER);
+ boolean isFreshnessStatusCheckerEnabled =
+
_serverConf.getProperty(Server.CONFIG_OF_ENABLE_REALTIME_FRESHNESS_BASED_CONSUMPTION_STATUS_CHECKER,
+
Server.DEFAULT_ENABLE_REALTIME_FRESHNESS_BASED_CONSUMPTION_STATUS_CHECKER);
+ int realtimeMinFreshnessMs =
_serverConf.getProperty(Server.CONFIG_OF_STARTUP_REALTIME_MIN_FRESHNESS_MS,
+ Server.DEFAULT_STARTUP_REALTIME_MIN_FRESHNESS_MS);
// collect all resources which have this instance in the ideal state
List<String> resourcesToMonitor = new ArrayList<>();
Set<String> consumingSegments = new HashSet<>();
boolean checkRealtime = realtimeConsumptionCatchupWaitMs > 0;
+ if (isFreshnessStatusCheckerEnabled && realtimeMinFreshnessMs <= 0) {
+ LOGGER.warn("Realtime min freshness {} must be > 0. Will not setup
freshness based checker.", realtimeMinFreshnessMs);
+ }
+ if (isFreshnessStatusCheckerEnabled &&
isOffsetBasedConsumptionStatusCheckerEnabled) {
+ LOGGER.warn("Offset and Freshness checkers both enabled. Will only setup
freshness based checker.");
+ }
+ boolean checkRealtimeFreshness = realtimeMinFreshnessMs > 0;
Review Comment:
This condition checks are confusing.
The first condition checks if a valid realtimeFreshness is set or not. The
second condition checks if both checks are enabled and warns saying it will
choose freshness based checker over offset based checker.
so, if my config has offsetBasedConsumptionStatusChecker = true,
freshnessBasedConsumptionStatusCheckerEnabled = true and realtimeMinFreshness =
-50, the logs will be very confusing.
It seems simpler to do this: if both are setup, choose freshness based check
and use default freshness value if the minFreshness is not configured
correctly. Also, the check for the correct config value can be moved further
down Line 298 `if (isFreshnessStatusCheckerEnabled) { `
nit: the boolean `checkRealtimeFreshness` is unused
--
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]
