Copilot commented on code in PR #17488:
URL: https://github.com/apache/pinot/pull/17488#discussion_r2692376842
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -303,6 +310,43 @@ protected boolean isTTLEnabled() {
return _metadataTTL > 0 || _deletedKeysTTL > 0;
}
+ /**
+ * Checks if all segments are loaded for the table. Uses a cached result to
avoid frequent checks.
+ * Once all segments are loaded, the check is skipped in subsequent calls.
+ * This method is used to prevent deleted primary keys from being removed
from memory until all segments
+ * are loaded, which prevents stale records from reappearing after server
restart due to out-of-order
+ * segment loading.
+ *
+ * @return true if all segments are loaded, false otherwise
+ */
+ protected boolean areAllSegmentsLoaded() {
+ if (_allSegmentsLoaded) {
+ return true;
+ }
+ synchronized (this) {
+ if (_allSegmentsLoaded) {
+ return true;
+ }
+ long currentTimeMs = System.currentTimeMillis();
+ if (currentTimeMs - _lastSegmentsLoadedCheckTimeMs <=
SEGMENTS_LOADED_CHECK_INTERVAL_MS) {
+ return false;
+ }
+ _lastSegmentsLoadedCheckTimeMs = currentTimeMs;
+ TableDataManager tableDataManager = _context.getTableDataManager();
+ if (tableDataManager != null) {
+ HelixManager helixManager = tableDataManager.getHelixManager();
+ if (helixManager != null) {
+ _allSegmentsLoaded =
TableStateUtils.isAllSegmentsLoaded(helixManager, _tableNameWithType);
+ if (_allSegmentsLoaded) {
+ _logger.info("All segments are now loaded for table: {},
partition: {}", _tableNameWithType, _partitionId);
+ }
+ return _allSegmentsLoaded;
+ }
+ }
+ return false;
+ }
+ }
Review Comment:
The double-checked locking pattern is incomplete. The `_allSegmentsLoaded`
field is declared as volatile (line 142), but `_lastSegmentsLoadedCheckTimeMs`
is not volatile. This could lead to visibility issues where one thread updates
`_lastSegmentsLoadedCheckTimeMs` inside the synchronized block but another
thread reads a stale value before entering the block. Either make
`_lastSegmentsLoadedCheckTimeMs` volatile or always access it within the
synchronized block.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerTest.java:
##########
@@ -2313,4 +2331,89 @@ public void testNoRevertForImmutableSegmentReplacement()
upsertMetadataManager.stop();
upsertMetadataManager.close();
}
+
+ @Test
+ public void testRemoveExpiredDeletedKeysSkippedWhenSegmentsStillLoading()
+ throws IOException, ReflectiveOperationException {
+ // Mock TableDataManager and HelixManager to simulate segments still
loading
+ TableDataManager tableDataManager = mock(TableDataManager.class);
+ when(tableDataManager.getTableDataDir()).thenReturn(INDEX_DIR);
+ HelixManager helixManager = mock(HelixManager.class);
+ when(tableDataManager.getHelixManager()).thenReturn(helixManager);
+
+
_contextBuilder.setDeleteRecordColumn(DELETE_RECORD_COLUMN).setDeletedKeysTTL(20)
+ .setTableDataManager(tableDataManager);
+
+ ConcurrentMapPartitionUpsertMetadataManager upsertMetadataManager =
+ new ConcurrentMapPartitionUpsertMetadataManager(REALTIME_TABLE_NAME, 0,
+ _contextBuilder.setHashFunction(HashFunction.NONE).build());
+ Map<Object, RecordLocation> recordLocationMap =
upsertMetadataManager._primaryKeyToRecordLocationMap;
+
+ // Add the first segment with one record
+ int numRecords = 1;
+ int[] primaryKeys = new int[]{0};
+ int[] timestamps = new int[]{100};
+ ThreadSafeMutableRoaringBitmap validDocIds1 = new
ThreadSafeMutableRoaringBitmap();
+ ThreadSafeMutableRoaringBitmap queryableDocIds1 = new
ThreadSafeMutableRoaringBitmap();
+ ImmutableSegmentImpl segment1 =
+ mockImmutableSegment(1, validDocIds1, queryableDocIds1,
getPrimaryKeyList(numRecords, primaryKeys));
+ upsertMetadataManager.addSegment(segment1, validDocIds1, queryableDocIds1,
+ getRecordInfoListForTTL(numRecords, primaryKeys, timestamps,
null).iterator());
+
+ // Add a mutable segment with delete record (outside TTL window)
+ ThreadSafeMutableRoaringBitmap validDocIds2 = new
ThreadSafeMutableRoaringBitmap();
+ ThreadSafeMutableRoaringBitmap queryableDocIds2 = new
ThreadSafeMutableRoaringBitmap();
+ MutableSegment segment2 = mockMutableSegment(1, validDocIds2,
queryableDocIds2);
+ upsertMetadataManager.addRecord(segment2, new
RecordInfo(makePrimaryKey(0), 0, new Integer(120), true));
+
+ // Add another record with a higher timestamp to update
largestSeenComparisonValue
+ // This makes timestamp 120 fall outside the TTL window (150 - 20 = 130 >
120)
+ upsertMetadataManager.addRecord(segment2, new
RecordInfo(makePrimaryKey(1), 1, new Integer(150), false));
Review Comment:
Avoid using `new Integer(150)` constructor which is deprecated. Use
`Integer.valueOf(150)` or simply `150` (autoboxing) instead. The Integer
constructor was deprecated in Java 9 and should be replaced with valueOf or
autoboxing for better performance and memory usage.
```suggestion
upsertMetadataManager.addRecord(segment2, new
RecordInfo(makePrimaryKey(0), 0, 120, true));
// Add another record with a higher timestamp to update
largestSeenComparisonValue
// This makes timestamp 120 fall outside the TTL window (150 - 20 = 130
> 120)
upsertMetadataManager.addRecord(segment2, new
RecordInfo(makePrimaryKey(1), 1, 150, false));
```
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerTest.java:
##########
@@ -2313,4 +2331,89 @@ public void testNoRevertForImmutableSegmentReplacement()
upsertMetadataManager.stop();
upsertMetadataManager.close();
}
+
+ @Test
+ public void testRemoveExpiredDeletedKeysSkippedWhenSegmentsStillLoading()
+ throws IOException, ReflectiveOperationException {
+ // Mock TableDataManager and HelixManager to simulate segments still
loading
+ TableDataManager tableDataManager = mock(TableDataManager.class);
+ when(tableDataManager.getTableDataDir()).thenReturn(INDEX_DIR);
+ HelixManager helixManager = mock(HelixManager.class);
+ when(tableDataManager.getHelixManager()).thenReturn(helixManager);
+
+
_contextBuilder.setDeleteRecordColumn(DELETE_RECORD_COLUMN).setDeletedKeysTTL(20)
+ .setTableDataManager(tableDataManager);
+
+ ConcurrentMapPartitionUpsertMetadataManager upsertMetadataManager =
+ new ConcurrentMapPartitionUpsertMetadataManager(REALTIME_TABLE_NAME, 0,
+ _contextBuilder.setHashFunction(HashFunction.NONE).build());
+ Map<Object, RecordLocation> recordLocationMap =
upsertMetadataManager._primaryKeyToRecordLocationMap;
+
+ // Add the first segment with one record
+ int numRecords = 1;
+ int[] primaryKeys = new int[]{0};
+ int[] timestamps = new int[]{100};
+ ThreadSafeMutableRoaringBitmap validDocIds1 = new
ThreadSafeMutableRoaringBitmap();
+ ThreadSafeMutableRoaringBitmap queryableDocIds1 = new
ThreadSafeMutableRoaringBitmap();
+ ImmutableSegmentImpl segment1 =
+ mockImmutableSegment(1, validDocIds1, queryableDocIds1,
getPrimaryKeyList(numRecords, primaryKeys));
+ upsertMetadataManager.addSegment(segment1, validDocIds1, queryableDocIds1,
+ getRecordInfoListForTTL(numRecords, primaryKeys, timestamps,
null).iterator());
+
+ // Add a mutable segment with delete record (outside TTL window)
+ ThreadSafeMutableRoaringBitmap validDocIds2 = new
ThreadSafeMutableRoaringBitmap();
+ ThreadSafeMutableRoaringBitmap queryableDocIds2 = new
ThreadSafeMutableRoaringBitmap();
+ MutableSegment segment2 = mockMutableSegment(1, validDocIds2,
queryableDocIds2);
+ upsertMetadataManager.addRecord(segment2, new
RecordInfo(makePrimaryKey(0), 0, new Integer(120), true));
+
+ // Add another record with a higher timestamp to update
largestSeenComparisonValue
+ // This makes timestamp 120 fall outside the TTL window (150 - 20 = 130 >
120)
+ upsertMetadataManager.addRecord(segment2, new
RecordInfo(makePrimaryKey(1), 1, new Integer(150), false));
Review Comment:
Avoid using `new Integer(120)` constructor which is deprecated. Use
`Integer.valueOf(120)` or simply `120` (autoboxing) instead. The Integer
constructor was deprecated in Java 9 and should be replaced with valueOf or
autoboxing for better performance and memory usage.
```suggestion
upsertMetadataManager.addRecord(segment2, new
RecordInfo(makePrimaryKey(0), 0, 120, true));
// Add another record with a higher timestamp to update
largestSeenComparisonValue
// This makes timestamp 120 fall outside the TTL window (150 - 20 = 130
> 120)
upsertMetadataManager.addRecord(segment2, new
RecordInfo(makePrimaryKey(1), 1, 150, false));
```
--
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]
