xiangfu0 commented on code in PR #19998:
URL: https://github.com/apache/pinot/pull/19998#discussion_r4354475370
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java:
##########
@@ -483,21 +580,94 @@ 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();
+ }
+
+ /// Indicates whether the immutable segment is reading its metadata from the
binary mapping.
+ public boolean isMapped() {
+ return _mappedStore != null;
+ }
+
+ @Nullable
+ @Override
+ public ColumnMetadata getColumnMetadataFor(String column) {
+ if (_mappedStore == null) {
+ return _columnMetadataMap != null ? _columnMetadataMap.get(column) :
null;
+ }
+ ColumnMetadata virtual = _virtualColumnMetadata.get(column);
+ if (virtual != null) {
+ return virtual;
+ }
+ if (_removedColumns.contains(column)) {
+ return null;
+ }
+ int ordinal = _mappedStore.ordinal(column);
+ if (ordinal < 0) {
+ return null;
+ }
+ MappedColumnMetadata metadata = _mappedColumnMetadata.get(ordinal);
+ if (metadata == null) {
+ metadata = new MappedColumnMetadata(_mappedStore, ordinal);
+ if (!_mappedColumnMetadata.compareAndSet(ordinal, null, metadata)) {
+ metadata = _mappedColumnMetadata.get(ordinal);
+ }
+ }
+ return metadata;
+ }
+
+ /// Adds a virtual column generated during serving load without changing the
on-disk physical-column directory.
+ public void addVirtualColumnMetadata(String column, ColumnMetadata metadata)
{
+ if (_mappedStore == null) {
+ _columnMetadataMap.put(column, metadata);
+ } else {
+ _virtualColumnMetadata.put(column, metadata);
+ }
+ }
+
+ /// Releases the metadata mapping. The owning segment directory closes it
after all serving readers are retired.
Review Comment:
Small doc nit: close() being synchronized doesn't actually exclude serving
readers (they read the buffer without any lock). The real safety here is the
refcount gate — destroy() only runs at refcount 0, after readers retire. Might
be worth saying that so a future maintainer doesn't assume close() is safe
mid-query. The synchronized + _mappedStoreClosed idempotency guard against
double-close is still correct and useful.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java:
##########
@@ -201,6 +208,10 @@ public static ImmutableSegment load(SegmentDirectory
segmentDirectory, IndexLoad
public static ImmutableSegment load(SegmentDirectory segmentDirectory,
IndexLoadingConfig indexLoadingConfig,
@Nullable Schema schema)
throws Exception {
+ // Preprocessing uses mutable metadata. Only the final immutable serving
path uses the mapped representation.
+ if (segmentDirectory instanceof SegmentLocalFSDirectory localDirectory) {
+ localDirectory.loadMappedMetadata();
Review Comment:
This writes segment_metadata.bin into the live serving dir. The
peer-download and upload paths in TablesResource (createCompressedTarFile at
~L622/~L1112) re-tar this whole directory recursively with no filter, so the
sidecar leaks into deep store and peer transfers.
Not a correctness issue (CRC is from creation.meta; the receiver
re-validates/rebuilds), but it breaks the "purely local" invariant. Worth
excluding FILE_NAME at those tar sites.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/SegmentLocalFSDirectory.java:
##########
@@ -141,11 +141,35 @@ public SegmentMetadataImpl getSegmentMetadata() {
return _segmentMetadata;
}
+ /// Called only during preprocessing, before the segment is queryable;
replacing and closing metadata is not
+ /// synchronized with serving readers.
@Override
public void reloadMetadata()
throws Exception {
- _segmentMetadata = new SegmentMetadataImpl(_indexDir);
- _columnIndexDirectory.setSegmentMetadata(_segmentMetadata);
+ SegmentMetadataImpl metadata = new SegmentMetadataImpl(_indexDir);
+ _columnIndexDirectory.setSegmentMetadata(metadata);
+ SegmentMetadataImpl previous = _segmentMetadata;
+ _segmentMetadata = metadata;
+ previous.close();
+ }
+
+ /// Replaces mutable metadata with the mapped representation after
preprocessing and before the segment is queryable.
+ public void loadMappedMetadata()
Review Comment:
Minor: this feature is effectively on-by-default with no way to turn it off
(serving load always calls this). The fallback-on-failure design makes that
defensible, but an instance-level kill-switch would be a handy lever — both to
disable writing the sidecar on read-only mounts and as mitigation if the
tar-leak above ships before it's fixed.
--
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]
