This is an automated email from the ASF dual-hosted git repository.

xiangfu0 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git


The following commit(s) were added to refs/heads/master by this push:
     new edc48dff392 Let a retried validation-manager IdealState update add the 
segments it created (#19739)
edc48dff392 is described below

commit edc48dff392553e812c516efb3ba890580e96965
Author: Suvodeep Pyne <[email protected]>
AuthorDate: Wed Oct 21 01:25:34 2026 -0700

    Let a retried validation-manager IdealState update add the segments it 
created (#19739)
    
    * Let a retried validation-manager IdealState update add the segments it 
created
    
    RealtimeSegmentValidationManager repairs partitions inside an IdealState
    updater: it creates the ZK metadata of the next CONSUMING segment, then adds
    the segment to the IdealState. When the IdealState write fails, e.g. on a
    version conflict with another writer, the updater runs again. The new
    segment's ZK metadata is still there, so the retry finds a latest segment 
that
    has ZK metadata but is not in the IdealState, and skips it because its ZK
    metadata was updated less than max.segment.completion.time.millis ago (the
    check that protects segments being committed). The repair lands only on a
    later run after that time, while the partition does not consume.
    
    Track the segments created by the updater across its attempts, and add a
    segment created by an earlier attempt to the IdealState right away. A 
segment
    is tracked as soon as its ZK metadata is created, before instances are
    assigned to it. The potential data loss is still reported when the segment
    replaces a CONSUMING segment that is no longer CONSUMING on the retry, but
    not for segments created for OFFLINE, ONLINE or new partitions, which have 
no
    previous CONSUMING segment.
    
    * Check the retry's created segments through ZK metadata
    
    testEnsureAllPartitionsConsumingRetryAddsSegmentsCreatedByEarlierAttempt
    asserted the contents of the internal map that tracks the segments created
    across attempts. Check the outcome instead: after the retry, partitions 0 
and
    1 have exactly the expected segment ZK metadata, so no extra segment was
    created, and the IdealState assertions cover the rest.
    
    * Prefer the IdealState segment among same-sequence repair candidates
    
    When two controllers repair a partition at the same time, e.g. during a 
lead controller change, both can create a new segment with the same sequence 
number. If the other controller adds its segment to the IdealState first, the 
retry of the IdealState update could pick its own new segment as the latest 
one, add it right away as created by an earlier attempt, and fail on the 
duplicate sequence number check, which aborts the whole update. The latest 
segment of a partition is now the one  [...]
    
    Also covers the public ensureAllPartitionsConsuming with a failed 
IdealState write, and the retry for a new partition.
---
 .../realtime/PinotLLCRealtimeSegmentManager.java   |  50 ++++--
 .../PinotLLCRealtimeSegmentManagerTest.java        | 196 ++++++++++++++++++++-
 2 files changed, 233 insertions(+), 13 deletions(-)

diff --git 
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java
 
b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java
index b4add6ac69e..d3211b55feb 100644
--- 
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java
+++ 
b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java
@@ -1548,8 +1548,9 @@ public class PinotLLCRealtimeSegmentManager implements 
PinotClusterConfigChangeL
   /// Returns the latest LLC realtime segment ZK metadata for each partition.
   ///
   /// When several segments of a partition have the highest sequence number, 
e.g. the new segment of a failed commit
-  /// start and the new segment of a concurrent repair, the one in the 
IdealState is the latest one. If none of them is
-  /// in the IdealState, whichever is listed first is the latest one.
+  /// start and the new segment of a concurrent repair, or the new segments of 
two controllers repairing the partition
+  /// at the same time, the one in the IdealState is the latest one. If none 
of them is in the IdealState, whichever is
+  /// listed first is the latest one.
   ///
   /// @param realtimeTableName Realtime table name
   /// @param instanceStatesMap Instance states map of the IdealState
@@ -1622,6 +1623,10 @@ public class PinotLLCRealtimeSegmentManager implements 
PinotClusterConfigChangeL
     Preconditions.checkState(!_isStopping, "Segment manager is stopping");
 
     String realtimeTableName = tableConfig.getTableName();
+    // The updater below runs again when the IdealState write fails (e.g. on a 
version conflict), while the segment ZK
+    // metadata created by the failed attempt remains. Track the created 
segments so that the next attempt still adds
+    // them to the IdealState.
+    Map<String, Boolean> newSegmentsCreated = new ConcurrentHashMap<>();
     try {
       HelixHelper.updateIdealState(_helixManager, realtimeTableName, 
idealState -> {
         assert idealState != null;
@@ -1643,7 +1648,7 @@ public class PinotLLCRealtimeSegmentManager implements 
PinotClusterConfigChangeL
               getNewStreamMetadataList(streamConfigs, 
currentPartitionGroupConsumptionStatusList, idealState);
           streamConfigs.stream().forEach(streamConfig -> 
streamConfig.setOffsetCriteria(originalOffsetCriteria));
           return ensureAllPartitionsConsuming(tableConfig, streamConfigs, 
idealState, streamMetadataList,
-              offsetCriteria);
+              offsetCriteria, newSegmentsCreated);
         } else {
           LOGGER.info("Skipping LLC segments validation for table: {}, 
isTableEnabled: {}, isTablePaused: {}",
               realtimeTableName, isTableEnabled, isTablePaused);
@@ -1914,6 +1919,16 @@ public class PinotLLCRealtimeSegmentManager implements 
PinotClusterConfigChangeL
   @VisibleForTesting
   IdealState ensureAllPartitionsConsuming(TableConfig tableConfig, 
List<StreamConfig> streamConfigs,
       IdealState idealState, List<StreamMetadata> streamMetadataList, 
OffsetCriteria offsetCriteria) {
+    return ensureAllPartitionsConsuming(tableConfig, streamConfigs, 
idealState, streamMetadataList, offsetCriteria,
+        new HashMap<>());
+  }
+
+  /// @param newSegmentsCreated Segments created by earlier attempts of the 
same IdealState update, mapped to whether
+  ///                           they replace a CONSUMING segment. Segments 
created by this attempt are added to it.
+  @VisibleForTesting
+  IdealState ensureAllPartitionsConsuming(TableConfig tableConfig, 
List<StreamConfig> streamConfigs,
+      IdealState idealState, List<StreamMetadata> streamMetadataList, 
OffsetCriteria offsetCriteria,
+      Map<String, Boolean> newSegmentsCreated) {
     String realtimeTableName = tableConfig.getTableName();
 
     InstancePartitions instancePartitions = 
getConsumingInstancePartitions(tableConfig);
@@ -2006,6 +2021,7 @@ public class PinotLLCRealtimeSegmentManager implements 
PinotClusterConfigChangeL
               createNewSegmentZKMetadata(tableConfig, 
streamConfigs.get(streamConfigIdx), newLLCSegmentName,
                   currentTimeMs,
                   committingSegmentDescriptor, latestSegmentZKMetadata, 
instancePartitions, numPartitions, numReplicas);
+              newSegmentsCreated.put(newSegmentName, true);
               updateInstanceStatesForNewConsumingSegment(instanceStatesMap, 
latestSegmentName, newSegmentName,
                   segmentAssignment, instancePartitionsMap);
             } else { // partition group reached end of life
@@ -2078,7 +2094,7 @@ public class PinotLLCRealtimeSegmentManager implements 
PinotClusterConfigChangeL
                     latestSegmentZKMetadata.getStartOffset()); // segments are 
OFFLINE; start from beginning
             createNewConsumingSegment(tableConfig, 
streamConfigs.get(streamConfigIdx), latestSegmentZKMetadata,
                 currentTimeMs, numPartitions, instancePartitions, 
instanceStatesMap, segmentAssignment,
-                instancePartitionsMap, startOffset);
+                instancePartitionsMap, startOffset, newSegmentsCreated);
           } else {
             LOGGER.info("Resuming consumption for partition: {} of table: {}", 
partitionId, realtimeTableName);
             StreamPartitionMsgOffset startOffset =
@@ -2086,7 +2102,7 @@ public class PinotLLCRealtimeSegmentManager implements 
PinotClusterConfigChangeL
                     tableConfig.getTableName(), offsetFactory, 
latestSegmentZKMetadata.getEndOffset());
             createNewConsumingSegment(tableConfig, 
streamConfigs.get(streamConfigIdx), latestSegmentZKMetadata,
                 currentTimeMs, numPartitions, instancePartitions, 
instanceStatesMap, segmentAssignment,
-                instancePartitionsMap, startOffset);
+                instancePartitionsMap, startOffset, newSegmentsCreated);
           }
         }
       } else {
@@ -2095,11 +2111,19 @@ public class PinotLLCRealtimeSegmentManager implements 
PinotClusterConfigChangeL
         // i.e. after updating old segment metadata (old segment metadata 
state = DONE/COMMITTING)
         // and creating new segment metadata (new segment metadata state = 
IN_PROGRESS),
         // but before updating ideal state (new segment ideal missing from 
ideal state)
-        if (!isExceededMaxSegmentCompletionTime(realtimeTableName, 
latestSegmentName, currentTimeMs)) {
-          continue;
+        // The segment can also be created by an earlier attempt of this 
update, which is not committing anything and
+        // can be added right away.
+        Boolean replacesConsumingSegment = 
newSegmentsCreated.get(latestSegmentName);
+        boolean createdByEarlierAttempt = replacesConsumingSegment != null;
+        if (createdByEarlierAttempt) {
+          LOGGER.info("Adding segment: {} created by an earlier attempt of the 
IdealState update", latestSegmentName);
+        } else {
+          if (!isExceededMaxSegmentCompletionTime(realtimeTableName, 
latestSegmentName, currentTimeMs)) {
+            continue;
+          }
+          LOGGER.info("Repairing segment: {} which has segment ZK metadata but 
does not exist in IdealState",
+              latestSegmentName);
         }
-        LOGGER.info("Repairing segment: {} which has segment ZK metadata but 
does not exist in IdealState",
-            latestSegmentName);
 
         if (latestSegmentZKMetadata.getStatus() == Status.IN_PROGRESS) {
           // Find the previous CONSUMING segment
@@ -2111,7 +2135,8 @@ public class PinotLLCRealtimeSegmentManager implements 
PinotClusterConfigChangeL
               break;
             }
           }
-          if (previousConsumingSegment == null) {
+          // A segment created for an OFFLINE, ONLINE or new partition has no 
previous CONSUMING segment
+          if (previousConsumingSegment == null && (!createdByEarlierAttempt || 
replacesConsumingSegment)) {
             LOGGER.error(
                 "Failed to find previous CONSUMING segment for partition: {} 
of table: {}, potential data loss",
                 partitionId, realtimeTableName);
@@ -2134,6 +2159,7 @@ public class PinotLLCRealtimeSegmentManager implements 
PinotClusterConfigChangeL
           String newSegmentName =
               setupNewPartitionGroup(tableConfig, 
streamMetadata.getStreamConfig(), partitionGroupMetadata,
                   currentTimeMs, instancePartitions, numPartitions, 
numReplicas);
+          newSegmentsCreated.put(newSegmentName, false);
           updateInstanceStatesForNewConsumingSegment(instanceStatesMap, null, 
newSegmentName, segmentAssignment,
               instancePartitionsMap);
         }
@@ -2147,7 +2173,8 @@ public class PinotLLCRealtimeSegmentManager implements 
PinotClusterConfigChangeL
       SegmentZKMetadata latestSegmentZKMetadata, long currentTimeMs,
       int numPartitions, InstancePartitions instancePartitions,
       Map<String, Map<String, String>> instanceStatesMap, SegmentAssignment 
segmentAssignment,
-      Map<InstancePartitionsType, InstancePartitions> instancePartitionsMap, 
StreamPartitionMsgOffset startOffset) {
+      Map<InstancePartitionsType, InstancePartitions> instancePartitionsMap, 
StreamPartitionMsgOffset startOffset,
+      Map<String, Boolean> newSegmentsCreated) {
     int numReplicas = getNumReplicas(tableConfig, instancePartitions);
     LLCSegmentName latestLLCSegmentName = new 
LLCSegmentName(latestSegmentZKMetadata.getSegmentName());
     LLCSegmentName newLLCSegmentName = 
getNextLLCSegmentName(latestLLCSegmentName, currentTimeMs);
@@ -2156,6 +2183,7 @@ public class PinotLLCRealtimeSegmentManager implements 
PinotClusterConfigChangeL
     createNewSegmentZKMetadata(tableConfig, streamConfig, newLLCSegmentName, 
currentTimeMs, committingSegmentDescriptor,
         latestSegmentZKMetadata, instancePartitions, numPartitions, 
numReplicas);
     String newSegmentName = newLLCSegmentName.getSegmentName();
+    newSegmentsCreated.put(newSegmentName, false);
     updateInstanceStatesForNewConsumingSegment(instanceStatesMap, null, 
newSegmentName, segmentAssignment,
         instancePartitionsMap);
   }
diff --git 
a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManagerTest.java
 
b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManagerTest.java
index 2f943077ccb..4eeef13372e 100644
--- 
a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManagerTest.java
+++ 
b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManagerTest.java
@@ -27,6 +27,7 @@ import java.net.URISyntaxException;
 import java.nio.charset.Charset;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -41,6 +42,7 @@ import java.util.TreeMap;
 import java.util.TreeSet;
 import java.util.UUID;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.stream.Collectors;
 import java.util.stream.IntStream;
 import javax.annotation.Nullable;
@@ -63,6 +65,7 @@ import 
org.apache.pinot.common.exception.HttpErrorStatusException;
 import org.apache.pinot.common.metadata.ZKMetadataProvider;
 import org.apache.pinot.common.metadata.segment.SegmentPartitionMetadata;
 import org.apache.pinot.common.metadata.segment.SegmentZKMetadata;
+import org.apache.pinot.common.metrics.ControllerMeter;
 import org.apache.pinot.common.metrics.ControllerMetrics;
 import org.apache.pinot.common.restlet.resources.BatchConfig;
 import org.apache.pinot.common.restlet.resources.PauseStatusDetails;
@@ -70,6 +73,7 @@ import 
org.apache.pinot.common.restlet.resources.TableLLCSegmentUploadResponse;
 import org.apache.pinot.common.utils.FileUploadDownloadClient;
 import org.apache.pinot.common.utils.LLCSegmentName;
 import org.apache.pinot.common.utils.URIUtils;
+import org.apache.pinot.common.utils.helix.HelixHelper;
 import org.apache.pinot.controller.ControllerConf;
 import org.apache.pinot.controller.helix.core.PinotHelixResourceManager;
 import 
org.apache.pinot.controller.helix.core.assignment.segment.SegmentAssignment;
@@ -117,6 +121,7 @@ import org.joda.time.Interval;
 import org.mockito.MockedStatic;
 import org.testng.Assert;
 import org.testng.annotations.AfterClass;
+import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
 import static 
org.apache.pinot.controller.ControllerConf.ControllerPeriodicTasksConf.ENABLE_TMP_SEGMENT_ASYNC_DELETION;
@@ -1029,6 +1034,181 @@ public class PinotLLCRealtimeSegmentManagerTest {
     assertEquals(consumingSegmentInstanceStateMap.get(offlineInstance), 
SegmentStateModel.CONSUMING);
   }
 
+  @Test
+  public void 
testEnsureAllPartitionsConsumingRetryAddsSegmentsCreatedByEarlierAttempt() {
+    ControllerMetrics controllerMetrics = mock(ControllerMetrics.class);
+    FakePinotLLCRealtimeSegmentManager segmentManager = new 
FakePinotLLCRealtimeSegmentManager(controllerMetrics);
+    setUpNewTable(segmentManager, 2, 5, 4);
+    Map<String, Map<String, String>> instanceStatesMap = 
segmentManager._idealState.getRecord().getMapFields();
+
+    // Partition 0: step 1 of the segment commit is done, but the segment is 
still CONSUMING in IdealState
+    String committingSegment = new LLCSegmentName(RAW_TABLE_NAME, 0, 0, 
CURRENT_TIME_MS).getSegmentName();
+    SegmentZKMetadata committingSegmentZKMetadata = 
segmentManager._segmentZKMetadataMap.get(committingSegment);
+    committingSegmentZKMetadata.setStatus(Status.DONE);
+    committingSegmentZKMetadata.setEndOffset(NEXT_OFFSET);
+    
segmentManager._segmentsExceedingMaxSegmentCompletionTime.add(committingSegment);
+    // Partition 1: all replicas of the consuming segment are OFFLINE
+    String offlineSegment = new LLCSegmentName(RAW_TABLE_NAME, 1, 0, 
CURRENT_TIME_MS).getSegmentName();
+    instanceStatesMap.get(offlineSegment).replaceAll((instance, state) -> 
SegmentStateModel.OFFLINE);
+    // Partition 4: a new partition of the stream
+    segmentManager._numPartitions = 5;
+
+    // The first attempt creates the new segments, but its IdealState write 
fails
+    Map<String, Boolean> newSegmentsCreated = new HashMap<>();
+    runFailedEnsureAllPartitionsConsumingAttempt(segmentManager, 
newSegmentsCreated);
+    String newSegment0 = new LLCSegmentName(RAW_TABLE_NAME, 0, 1, 
CURRENT_TIME_MS).getSegmentName();
+    String newSegment1 = new LLCSegmentName(RAW_TABLE_NAME, 1, 1, 
CURRENT_TIME_MS).getSegmentName();
+    String newSegment4 = new LLCSegmentName(RAW_TABLE_NAME, 4, 0, 
CURRENT_TIME_MS).getSegmentName();
+
+    // The retry starts from the unchanged IdealState. It must add the new 
segments right away instead of waiting for
+    // them to exceed the max segment completion time.
+    segmentManager.ensureAllPartitionsConsuming(segmentManager._tableConfig, 
segmentManager._streamConfigs,
+        segmentManager._idealState, getStreamMetadataList(segmentManager), 
null, newSegmentsCreated);
+    assertEquals(segmentManager._segmentZKMetadataMap.keySet().stream()
+            .filter(segment -> Set.of(0, 1, 4).contains(new 
LLCSegmentName(segment).getPartitionGroupId()))
+            .collect(Collectors.toSet()),
+        Set.of(committingSegment, newSegment0, offlineSegment, newSegment1, 
newSegment4));
+    assertEquals(new 
HashSet<>(instanceStatesMap.get(committingSegment).values()), 
Set.of(SegmentStateModel.ONLINE));
+    assertEquals(new HashSet<>(instanceStatesMap.get(newSegment0).values()), 
Set.of(SegmentStateModel.CONSUMING));
+    assertEquals(new 
HashSet<>(instanceStatesMap.get(offlineSegment).values()), 
Set.of(SegmentStateModel.OFFLINE));
+    assertEquals(new HashSet<>(instanceStatesMap.get(newSegment1).values()), 
Set.of(SegmentStateModel.CONSUMING));
+    assertEquals(new HashSet<>(instanceStatesMap.get(newSegment4).values()), 
Set.of(SegmentStateModel.CONSUMING));
+    verify(controllerMetrics, 
never()).addMeteredTableValue(REALTIME_TABLE_NAME, 
ControllerMeter.LLC_STREAM_DATA_LOSS,
+        1L);
+  }
+
+  @Test
+  @SuppressWarnings("unchecked")
+  public void 
testEnsureAllPartitionsConsumingAddsSegmentsCreatedByAttemptWithFailedIdealStateWrite()
 {
+    PinotHelixResourceManager resourceManager = 
FakePinotLLCRealtimeSegmentManager.createMockedResourceManager();
+    FakePinotLLCRealtimeSegmentManager segmentManager = new 
FakePinotLLCRealtimeSegmentManager(resourceManager);
+    setUpNewTable(segmentManager, 2, 5, 4);
+    String committingSegment = new LLCSegmentName(RAW_TABLE_NAME, 0, 0, 
CURRENT_TIME_MS).getSegmentName();
+    SegmentZKMetadata committingSegmentZKMetadata = 
segmentManager._segmentZKMetadataMap.get(committingSegment);
+    committingSegmentZKMetadata.setStatus(Status.DONE);
+    committingSegmentZKMetadata.setEndOffset(NEXT_OFFSET);
+    
segmentManager._segmentsExceedingMaxSegmentCompletionTime.add(committingSegment);
+
+    // The first IdealState write fails because another controller uploads a 
segment in the meantime
+    HelixDataAccessor dataAccessor = mock(HelixDataAccessor.class);
+    
when(resourceManager.getHelixZkManager().getHelixDataAccessor()).thenReturn(dataAccessor);
+    when(dataAccessor.keyBuilder()).thenReturn(new 
PropertyKey.Builder(CLUSTER_NAME));
+    when(dataAccessor.getProperty(any(PropertyKey.class))).thenAnswer(
+        invocation -> HelixHelper.cloneIdealState(segmentManager._idealState));
+    BaseDataAccessor<ZNRecord> baseDataAccessor = mock(BaseDataAccessor.class);
+    when(dataAccessor.getBaseDataAccessor()).thenReturn(baseDataAccessor);
+    String uploadedSegment = "uploadedSegment";
+    AtomicInteger numWrites = new AtomicInteger();
+    when(baseDataAccessor.set(anyString(), any(ZNRecord.class), anyInt(), 
anyInt())).thenAnswer(invocation -> {
+      if (numWrites.getAndIncrement() == 0) {
+        segmentManager._idealState.getRecord().getMapFields()
+            .put(uploadedSegment, Map.of(Helix.PREFIX_OF_SERVER_INSTANCE + 0, 
SegmentStateModel.ONLINE));
+        return false;
+      }
+      segmentManager._idealState = new 
IdealState(invocation.<ZNRecord>getArgument(1));
+      return true;
+    });
+
+    // The retry adds the new segment created by the first attempt right away, 
and keeps the uploaded segment
+    segmentManager.ensureAllPartitionsConsuming(segmentManager._tableConfig, 
segmentManager._streamConfigs, null);
+    assertEquals(numWrites.get(), 2);
+    String newSegment = new LLCSegmentName(RAW_TABLE_NAME, 0, 1, 
CURRENT_TIME_MS).getSegmentName();
+    assertEquals(segmentManager._segmentZKMetadataMap.keySet().stream()
+        .filter(segment -> new LLCSegmentName(segment).getPartitionGroupId() 
== 0)
+        .collect(Collectors.toSet()), Set.of(committingSegment, newSegment));
+    Map<String, Map<String, String>> instanceStatesMap = 
segmentManager._idealState.getRecord().getMapFields();
+    assertEquals(new 
HashSet<>(instanceStatesMap.get(committingSegment).values()), 
Set.of(SegmentStateModel.ONLINE));
+    assertEquals(new HashSet<>(instanceStatesMap.get(newSegment).values()), 
Set.of(SegmentStateModel.CONSUMING));
+    assertTrue(instanceStatesMap.containsKey(uploadedSegment));
+  }
+
+  @DataProvider
+  public Object[][] reverseLLCSegments() {
+    return new Object[][]{{false}, {true}};
+  }
+
+  @Test(dataProvider = "reverseLLCSegments")
+  public void 
testEnsureAllPartitionsConsumingRetryKeepsSegmentAddedByAnotherController(boolean
 reverseLLCSegments) {
+    ControllerMetrics controllerMetrics = mock(ControllerMetrics.class);
+    FakePinotLLCRealtimeSegmentManager segmentManager = new 
FakePinotLLCRealtimeSegmentManager(controllerMetrics);
+    setUpNewTable(segmentManager, 2, 5, 4);
+    // Lists the segments in both orders across the test runs, as the segments 
with the same sequence number below
+    // must be resolved the same way in either order
+    segmentManager._reverseLLCSegments = reverseLLCSegments;
+    Map<String, Map<String, String>> instanceStatesMap = 
segmentManager._idealState.getRecord().getMapFields();
+    String committingSegment = new LLCSegmentName(RAW_TABLE_NAME, 0, 0, 
CURRENT_TIME_MS).getSegmentName();
+    SegmentZKMetadata committingSegmentZKMetadata = 
segmentManager._segmentZKMetadataMap.get(committingSegment);
+    committingSegmentZKMetadata.setStatus(Status.DONE);
+    committingSegmentZKMetadata.setEndOffset(NEXT_OFFSET);
+    
segmentManager._segmentsExceedingMaxSegmentCompletionTime.add(committingSegment);
+
+    // The first attempt creates a new segment, but its IdealState write fails
+    Map<String, Boolean> newSegmentsCreated = new HashMap<>();
+    runFailedEnsureAllPartitionsConsumingAttempt(segmentManager, 
newSegmentsCreated);
+    String newSegment = new LLCSegmentName(RAW_TABLE_NAME, 0, 1, 
CURRENT_TIME_MS).getSegmentName();
+
+    // Meanwhile, another controller, e.g. the previous lead controller, 
creates its own new segment with the same
+    // sequence number and adds it to the IdealState first
+    String otherNewSegment =
+        new LLCSegmentName(RAW_TABLE_NAME, 0, 1, CURRENT_TIME_MS - 
TimeUnit.MINUTES.toMillis(1)).getSegmentName();
+    segmentManager.persistSegmentZKMetadata(REALTIME_TABLE_NAME, new 
SegmentZKMetadata(
+        new 
ZNRecord(segmentManager._segmentZKMetadataMap.get(newSegment).toZNRecord(), 
otherNewSegment)), -1);
+    Map<String, String> otherNewSegmentInstanceStateMap = new TreeMap<>();
+    for (String instance : instanceStatesMap.get(committingSegment).keySet()) {
+      otherNewSegmentInstanceStateMap.put(instance, 
SegmentStateModel.CONSUMING);
+    }
+    instanceStatesMap.get(committingSegment).replaceAll((instance, state) -> 
SegmentStateModel.ONLINE);
+    instanceStatesMap.put(otherNewSegment, otherNewSegmentInstanceStateMap);
+
+    // The retry keeps the segment of the other controller instead of adding 
its own one
+    Map<String, Map<String, String>> expectedInstanceStatesMap = 
cloneInstanceStatesMap(instanceStatesMap);
+    Set<String> expectedSegments = new 
HashSet<>(segmentManager._segmentZKMetadataMap.keySet());
+    segmentManager.ensureAllPartitionsConsuming(segmentManager._tableConfig, 
segmentManager._streamConfigs,
+        segmentManager._idealState, getStreamMetadataList(segmentManager), 
null, newSegmentsCreated);
+    assertEquals(instanceStatesMap, expectedInstanceStatesMap);
+    assertEquals(segmentManager._segmentZKMetadataMap.keySet(), 
expectedSegments);
+    verify(controllerMetrics, 
never()).addMeteredTableValue(REALTIME_TABLE_NAME, 
ControllerMeter.LLC_STREAM_DATA_LOSS,
+        1L);
+  }
+
+  @Test
+  public void 
testEnsureAllPartitionsConsumingRetryReportsDataLossWhenReplacedSegmentGoesOffline()
 {
+    ControllerMetrics controllerMetrics = mock(ControllerMetrics.class);
+    FakePinotLLCRealtimeSegmentManager segmentManager = new 
FakePinotLLCRealtimeSegmentManager(controllerMetrics);
+    setUpNewTable(segmentManager, 2, 5, 4);
+    Map<String, Map<String, String>> instanceStatesMap = 
segmentManager._idealState.getRecord().getMapFields();
+    String committingSegment = new LLCSegmentName(RAW_TABLE_NAME, 0, 0, 
CURRENT_TIME_MS).getSegmentName();
+    SegmentZKMetadata committingSegmentZKMetadata = 
segmentManager._segmentZKMetadataMap.get(committingSegment);
+    committingSegmentZKMetadata.setStatus(Status.DONE);
+    committingSegmentZKMetadata.setEndOffset(NEXT_OFFSET);
+    
segmentManager._segmentsExceedingMaxSegmentCompletionTime.add(committingSegment);
+
+    Map<String, Boolean> newSegmentsCreated = new HashMap<>();
+    runFailedEnsureAllPartitionsConsumingAttempt(segmentManager, 
newSegmentsCreated);
+
+    // All replicas of the segment being replaced go OFFLINE before the retry
+    instanceStatesMap.get(committingSegment).replaceAll((instance, state) -> 
SegmentStateModel.OFFLINE);
+    segmentManager.ensureAllPartitionsConsuming(segmentManager._tableConfig, 
segmentManager._streamConfigs,
+        segmentManager._idealState, getStreamMetadataList(segmentManager), 
null, newSegmentsCreated);
+    String newSegment = new LLCSegmentName(RAW_TABLE_NAME, 0, 1, 
CURRENT_TIME_MS).getSegmentName();
+    assertEquals(new HashSet<>(instanceStatesMap.get(newSegment).values()), 
Set.of(SegmentStateModel.CONSUMING));
+    verify(controllerMetrics).addMeteredTableValue(REALTIME_TABLE_NAME, 
ControllerMeter.LLC_STREAM_DATA_LOSS, 1L);
+  }
+
+  /// Runs ensureAllPartitionsConsuming on a copy of the IdealState, as an 
attempt whose IdealState write fails.
+  private void 
runFailedEnsureAllPartitionsConsumingAttempt(FakePinotLLCRealtimeSegmentManager 
segmentManager,
+      Map<String, Boolean> newSegmentsCreated) {
+    IdealState failedAttemptIdealState = new IdealState(REALTIME_TABLE_NAME);
+    failedAttemptIdealState.getRecord()
+        
.setMapFields(cloneInstanceStatesMap(segmentManager._idealState.getRecord().getMapFields()));
+    segmentManager.ensureAllPartitionsConsuming(segmentManager._tableConfig, 
segmentManager._streamConfigs,
+        failedAttemptIdealState, getStreamMetadataList(segmentManager), null, 
newSegmentsCreated);
+  }
+
+  private List<StreamMetadata> 
getStreamMetadataList(FakePinotLLCRealtimeSegmentManager segmentManager) {
+    return 
segmentManager.getNewStreamMetadataList(segmentManager._streamConfigs, 
List.of(), mock(IdealState.class));
+  }
+
   @Test
   public void testPartialOfflineReplicaRepairDisabled() {
     // Set up a new table with 3 replicas, 5 instances, 2 partitions
@@ -2843,6 +3023,10 @@ public class PinotLLCRealtimeSegmentManagerTest {
     int _numPartitions;
     List<StreamMetadata> _streamMetadataList = null;
     boolean _exceededMaxSegmentCompletionTime = false;
+    // Segments that exceed the max segment completion time even when 
_exceededMaxSegmentCompletionTime is false
+    Set<String> _segmentsExceedingMaxSegmentCompletionTime = new HashSet<>();
+    // Whether to list the LLC segments in reverse order
+    boolean _reverseLLCSegments = false;
     // Number of upcoming IdealState updates on segment completion that fail 
without being applied
     int _numFailedIdealStateUpdates = 0;
     // Number of upcoming IdealState updates on segment completion that are 
applied but reported as failed
@@ -2874,6 +3058,10 @@ public class PinotLLCRealtimeSegmentManagerTest {
       _mockResourceManager = pinotHelixResourceManager;
     }
 
+    FakePinotLLCRealtimeSegmentManager(ControllerMetrics controllerMetrics) {
+      super(createMockedResourceManager(), CONTROLLER_CONF, controllerMetrics);
+    }
+
     FakePinotLLCRealtimeSegmentManager(PinotHelixResourceManager 
pinotHelixResourceManager,
         boolean enablePartialOfflineReplicaRepair) {
       super(pinotHelixResourceManager, 
createControllerConf(enablePartialOfflineReplicaRepair),
@@ -2964,7 +3152,11 @@ public class PinotLLCRealtimeSegmentManagerTest {
 
     @Override
     List<String> getLLCSegments(String realtimeTableName) {
-      return new ArrayList<>(_segmentZKMetadataMap.keySet());
+      List<String> llcSegments = new 
ArrayList<>(_segmentZKMetadataMap.keySet());
+      if (_reverseLLCSegments) {
+        Collections.reverse(llcSegments);
+      }
+      return llcSegments;
     }
 
     @Override
@@ -3086,7 +3278,7 @@ public class PinotLLCRealtimeSegmentManagerTest {
     @Override
     protected boolean isExceededMaxSegmentCompletionTime(String 
realtimeTableName, String segmentName,
         long currentTimeMs) {
-      return _exceededMaxSegmentCompletionTime;
+      return _exceededMaxSegmentCompletionTime || 
_segmentsExceedingMaxSegmentCompletionTime.contains(segmentName);
     }
 
     @Override


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to