xiangfu0 commented on code in PR #19998:
URL: https://github.com/apache/pinot/pull/19998#discussion_r4354610112
##########
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:
Fixed in 643f9fb368: peer downloads and deep-store uploads now use the same
filtered tar path. It omits the root segment_metadata.bin and in-progress
.segment_metadata.*.tmp files while retaining the segment directory. The server
download regression and tar utility tests pass.
##########
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:
Updated in 643f9fb368: close() documentation now distinguishes the
reader-lifetime requirement from synchronized double-close protection. The
owning serving directory closes after readers retire, and standalone callers
must keep the mapping open while using it.
##########
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:
Added in 643f9fb368:
pinot.server.instance.segment.metadata.mmap.enabled=false disables mapped
metadata on a server instance. The default remains enabled. Both empty and
normal local serving loads honor the setting; a real-segment test confirms
disabled mode stays eager and creates no sidecar.
--
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]
