[GitHub] [pinot] KKcorps commented on a diff in pull request #10198: Fix the potential deadlock for partial-upsert segment loading check

["KKcorps (via GitHub)" <gi...@apache.org>]

KKcorps commented on code in PR #10198:
URL: https://github.com/apache/pinot/pull/10198#discussion_r1089665870


##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/tablestate/TableStateUtils.java:
##########
@@ -83,58 +84,57 @@ public static List<String> getSegmentsInGivenStateForThisInstance(HelixManager h
    * @return true if all segments for the given table are succesfully loaded. False otherwise
    */
   public static boolean isAllSegmentsLoaded(HelixManager helixManager, String tableNameWithType) {
+    List<String> onlineSegments =
+        getSegmentsInGivenStateForThisInstance(helixManager, tableNameWithType, SegmentStateModel.ONLINE);
+    if (onlineSegments.isEmpty()) {
+      LOGGER.info("No ONLINE segment found for table: {}", tableNameWithType);
+      return true;
+    }
+
+    // Check if ideal state and current state matches for all segments assigned to the current instance
     HelixDataAccessor dataAccessor = helixManager.getHelixDataAccessor();
     PropertyKey.Builder keyBuilder = dataAccessor.keyBuilder();
     String instanceName = helixManager.getInstanceName();
-
-    List<String> onlineSegments = getSegmentsInGivenStateForThisInstance(helixManager, tableNameWithType,
-        CommonConstants.Helix.StateModel.SegmentStateModel.ONLINE);
-    if (onlineSegments.size() > 0) {
-      LiveInstance liveInstance = dataAccessor.getProperty(keyBuilder.liveInstance(instanceName));
-      if (liveInstance == null) {
-        LOGGER.warn("Failed to find live instance for instance: {}", instanceName);
-        return false;
-      }
-      String sessionId = liveInstance.getEphemeralOwner();
-      CurrentState currentState =
-          dataAccessor.getProperty(keyBuilder.currentState(instanceName, sessionId, tableNameWithType));
-      if (currentState == null) {
-        LOGGER.warn("Failed to find current state for instance: {}, sessionId: {}, table: {}", instanceName, sessionId,
-            tableNameWithType);
-        return false;
-      }
-      // Check if ideal state and current state matches for all segments assigned to the current instance
-      Map<String, String> currentStateMap = currentState.getPartitionStateMap();
-
-      for (String segmentName : onlineSegments) {
-        String actualState = currentStateMap.get(segmentName);
-        if (!CommonConstants.Helix.StateModel.SegmentStateModel.ONLINE.equals(actualState)) {
-          if (CommonConstants.Helix.StateModel.SegmentStateModel.ERROR.equals(actualState)) {
-            LOGGER.error("Find ERROR segment: {}, table: {}, expected: {}", segmentName, tableNameWithType,
-                CommonConstants.Helix.StateModel.SegmentStateModel.ONLINE);
-          } else {
-            LOGGER.info("Find unloaded segment: {}, table: {}, expected: {}, actual: {}", segmentName,
-                tableNameWithType, CommonConstants.Helix.StateModel.SegmentStateModel.ONLINE, actualState);
-          }
+    LiveInstance liveInstance = dataAccessor.getProperty(keyBuilder.liveInstance(instanceName));
+    if (liveInstance == null) {
+      LOGGER.warn("Failed to find live instance for instance: {}", instanceName);
+      return false;
+    }
+    String sessionId = liveInstance.getEphemeralOwner();
+    CurrentState currentState =
+        dataAccessor.getProperty(keyBuilder.currentState(instanceName, sessionId, tableNameWithType));
+    if (currentState == null) {
+      LOGGER.warn("Failed to find current state for instance: {}, sessionId: {}, table: {}", instanceName, sessionId,
+          tableNameWithType);
+      return false;
+    }
+    List<String> unloadedSegments = new ArrayList<>();
+    Map<String, String> currentStateMap = currentState.getPartitionStateMap();
+    for (String segmentName : onlineSegments) {
+      String actualState = currentStateMap.get(segmentName);
+      if (!SegmentStateModel.ONLINE.equals(actualState)) {
+        if (SegmentStateModel.ERROR.equals(actualState)) {
+          LOGGER.error("Found ERROR segment: {}, table: {}, expected: {}", segmentName, tableNameWithType,

Review Comment:
   ```suggestion
             LOGGER.error("Found segment: {} in ERROR state, table: {}, expected: {}", segmentName, tableNameWithType,
   ```



-- 
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