Jackie-Jiang commented on code in PR #19998:
URL: https://github.com/apache/pinot/pull/19998#discussion_r4395919062
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java:
##########
@@ -483,21 +593,133 @@ 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()
+ throws IOException, ConfigurationException {
+ if (_mappedStore == null) {
+ return this;
+ }
+ SegmentMetadataImpl copy = new SegmentMetadataImpl(_indexDir);
Review Comment:
[P2] Copy the acquired metadata instead of reopening its mutable directory
The segment reference held by
HelixInstanceDataManager.getSegmentMetadata()/getAllSegmentsMetadata() pins the
old segment and mapping, but it does not pin the filesystem pathname.
BaseTableDataManager.acquireSegment() only increments the reference count.
During reload, createBackup(indexDir) at lines 1335/1369 renames that directory
while the original segment remains queryable through its existing file
descriptors; the original pathname is then repopulated/preprocessed before the
serving segment is replaced.
Consequently, new SegmentMetadataImpl(_indexDir) here can fail while that
path is absent/incomplete, or read replacement metadata and combine it with the
old segment's removed/virtual columns and ZK timestamps. The new detach test
leaves the source directory unchanged and does not cover this interleaving.
Please materialize the detached snapshot from the currently acquired mapped
metadata and in-memory segment fields/overlays. Add a deterministic test that
renames or replaces the original directory while the mapped instance remains
open, and verifies that detachedCopy() still returns the original metadata and
survives closing its owner.
I found no active callers of these InstanceDataManager APIs in the checked
OSS/StarTree trees, so this is an API correctness issue, not a demonstrated
query outage.
--
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]
