xiangfu0 commented on code in PR #19998:
URL: https://github.com/apache/pinot/pull/19998#discussion_r4416632491
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java:
##########
@@ -556,6 +557,14 @@ public List<SegmentMetadata> getAllSegmentsMetadata(String
tableNameWithType) {
}
}
+ private static SegmentMetadata detachMappedMetadata(SegmentDataManager
segmentDataManager) {
+ SegmentMetadata metadata =
segmentDataManager.getSegment().getSegmentMetadata();
+ if (metadata instanceof SegmentMetadataImpl segmentMetadata &&
segmentMetadata.isMapped()) {
+ return segmentMetadata.detachedCopy();
Review Comment:
Safe to drop the wrapper here: this runs while the segment reference is
still acquired (before releaseSegment), so the mapped store can't close
mid-copy and the only theoretical failures are unchecked decode errors that
validation at open() already rules out. The lost segment-name context in the
old wrap is negligible — the column-level ISEs already name the column.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java:
##########
@@ -483,21 +621,135 @@ 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:
Correct cleanup — nothing in this body can throw a checked exception: the
copy constructor is plain field copies, and the mapped column getters wrap
their IO failures in IllegalStateException. The declared throws was a leftover,
and removing it is what forces the caller's dead catch out too.
--
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]
