xiangfu0 commented on code in PR #19998:
URL: https://github.com/apache/pinot/pull/19998#discussion_r4396810063
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java:
##########
@@ -483,21 +621,136 @@ public String getEndOffset() {
@Override
public TreeMap<String, ColumnMetadata> getColumnMetadataMap() {
- return _columnMetadataMap;
+ if (_mappedStore == null) {
+ return _columnMetadataMap;
+ }
+ // Compatibility API for metadata/admin callers. The serving path uses
column names and direct lookup instead.
+ TreeMap<String, ColumnMetadata> result = new TreeMap<>();
+ for (String column : getPhysicalColumnNames()) {
+ result.put(column, getColumnMetadataFor(column));
+ }
+ result.putAll(_virtualColumnMetadata);
+ return result;
+ }
+
+ /// Returns the physical column names without materializing a metadata map
or Schema on mapped segments.
+ public Set<String> getPhysicalColumnNames() {
+ return _mappedStore != null ? _physicalColumnNames :
_schema.getPhysicalColumnNames();
+ }
+
+ @Override
+ public NavigableSet<String> getAllColumns() {
+ if (_mappedStore == null) {
+ return getSchema().getColumnNames();
+ }
+ NavigableSet<String> columns = _mappedAllColumns;
+ if (columns == null) {
+ NavigableSet<String> allColumns = new
TreeSet<>(getPhysicalColumnNames());
+ allColumns.addAll(_virtualColumnMetadata.keySet());
+ columns = Collections.unmodifiableNavigableSet(allColumns);
+ _mappedAllColumns = columns;
+ }
+ return columns;
+ }
+
+ /// Indicates whether the immutable segment is reading its metadata from the
binary mapping.
+ public boolean isMapped() {
+ return _mappedStore != null;
+ }
+
+ /// Returns metadata that remains readable after the mapped segment is
released. Eager metadata already owns its
+ /// column data; a mapped segment needs an eager copy with its serving-time
column overlays and ZK timestamps.
+ public SegmentMetadataImpl detachedCopy()
Review Comment:
This is the right fix for handing mapped metadata to callers that outlive
the segment's reference window — the detached copy holds no store/buffer
reference (isMapped()==false) and copies every field the eager path exposes,
including index sizes, ZK timestamps and virtual columns.
HelixInstanceDataManager builds it before releaseSegment() while the mapping is
still open, so it's safe. Good.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -107,13 +109,20 @@ public abstract class BasePartitionUpsertMetadataManager
implements PartitionUps
// Basically, it's possible that some segments in the table partition are
not tracked here, as their upsert metadata
// is not managed by the manager currently.
protected final Set<IndexSegment> _trackedSegments =
ConcurrentHashMap.newKeySet();
- // Track all the immutable segments where changes took place since last
snapshot was taken.
- // Note: the set can be updated by multiple Helix task threads and the
consuming thread concurrently. To not miss
- // segments updated while a snapshot is being taken, the snapshot flow
removes the exact segments it has persisted
- // instead of clearing the set, and it retains only the tracked segments at
the end of each snapshot round to avoid
- // keeping stale segment object references around.
+ // Track all the immutable segments where changes took place since last
snapshot was taken, along with their maximum
+ // comparison values. The value must remain readable if a segment is
destroyed while the snapshot pass is capping
+ // the TTL watermark. Helix task threads and the consuming thread can update
the map concurrently, so the snapshot
+ // flow removes only the exact segments it persisted and retains tracked
segments at the end of each round.
// Skip mutableSegments as only immutable segments are for taking snapshots.
- protected final Set<ImmutableSegmentImpl> _updatedSegmentsSinceLastSnapshot
= ConcurrentHashMap.newKeySet();
+ private final ConcurrentHashMap<ImmutableSegmentImpl, Double>
_updatedSegmentMaxComparisonValues =
+ new ConcurrentHashMap<>();
+ // Keep the protected Set field for extensions compiled against the previous
API. Both views share one backing map.
+ protected final Set<ImmutableSegmentImpl> _updatedSegmentsSinceLastSnapshot =
Review Comment:
Good call restoring this to the original Set<ImmutableSegmentImpl> type —
the prior head had changed it to a ConcurrentMap, which would have broken
subclasses compiled against the released API. Backing it with
keySet(TTL_WATERMARK_NOT_SET) keeps the marker semantics correct (an external
.add() inserts NOT_SET, which getWatermarkToPersist reads under the lock).
--
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]
