[GitHub] [pinot] mcvsubbu commented on a diff in pull request #9994: [feature] Add a Tracker class to support aggregate-worst case consumption delay…

[GitBox <gi...@apache.org>]

mcvsubbu commented on code in PR #9994:
URL: https://github.com/apache/pinot/pull/9994#discussion_r1053912680


##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SegmentOnlineOfflineStateModelFactory.java:
##########
@@ -91,6 +92,16 @@ public void onBecomeOnlineFromConsuming(Message message, NotificationContext con
 
       TableDataManager tableDataManager = _instanceDataManager.getTableDataManager(realtimeTableName);
       Preconditions.checkNotNull(tableDataManager);
+      if (tableDataManager instanceof RealtimeTableDataManager) {

Review Comment:
   Please add the desired method in the `BaseTableDataManager` class with a default implementation that does nothing, and override it on the `RealtimeTableDataManager`.



##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SegmentOnlineOfflineStateModelFactory.java:
##########
@@ -138,6 +149,19 @@ public void onBecomeOfflineFromConsuming(Message message, NotificationContext co
     @Transition(from = "CONSUMING", to = "DROPPED")
     public void onBecomeDroppedFromConsuming(Message message, NotificationContext context) {
       _logger.info("SegmentOnlineOfflineStateModel.onBecomeDroppedFromConsuming() : " + message);
+      String realtimeTableName = message.getResourceName();
+      String segmentNameStr = message.getPartitionName();
+      TableDataManager tableDataManager = _instanceDataManager.getTableDataManager(realtimeTableName);
+      Preconditions.checkNotNull(tableDataManager);
+      if (tableDataManager instanceof RealtimeTableDataManager) {

Review Comment:
   Same here, let us just add a method in the base class



##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/tablestate/TableStateUtils.java:
##########
@@ -38,6 +38,40 @@ public class TableStateUtils {
   private TableStateUtils() {
   }
 
+  /**
+   * Returns all online segments for a given table.
+   *
+   * @param helixManager instance of Helix manager
+   * @param tableNameWithType table for which we are obtaining ONLINE segments
+   *
+   * @return List of ONLINE segment names.
+   */
+  public static List<String> getTableOnlineSegments(HelixManager helixManager, String tableNameWithType) {

Review Comment:
   Suggest rename to `getOnlineSegmentsForThisInstance()` 



##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java:
##########
@@ -611,6 +616,8 @@ private boolean processStreamEvents(MessageBatch messagesAndOffsets, long idlePi
       if (_segmentLogger.isDebugEnabled()) {
         _segmentLogger.debug("empty batch received - sleeping for {}ms", idlePipeSleepTimeMillis);
       }
+      // Record Pinot ingestion delay as zero since we are up-to-date and no new events

Review Comment:
   @Jackie-Jiang to propose appropriate interface. I suggest an interface in the stream metadata like (just as an example) `emptyReturnIndicatesNoEventsAvailable()`. I think what Jackie had in mind was to change the poll API to indicate whether there are more events. That could be a return in the metadata, for example. I am open to either approach.



##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/tablestate/TableStateUtils.java:
##########
@@ -47,6 +81,10 @@ private TableStateUtils() {
    * @return true if all segments for the given table are succesfully loaded. False otherwise
    */
   public static boolean isAllSegmentsLoaded(HelixManager helixManager, String tableNameWithType) {
+    HelixDataAccessor dataAccessor = helixManager.getHelixDataAccessor();
+    PropertyKey.Builder keyBuilder = dataAccessor.keyBuilder();
+    String instanceName = helixManager.getInstanceName();
+    /*

Review Comment:
   I suppose you are going to remove the commented code, right? It is here just for review. Please add a TODO or some clear indication that these lines will be removed before you merge



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org