xiangfu0 commented on code in PR #20114:
URL: https://github.com/apache/pinot/pull/20114#discussion_r4435366662
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/store/SegmentDirectory.java:
##########
@@ -101,6 +101,13 @@ public abstract class SegmentDirectory implements
Closeable {
public abstract void reloadMetadata()
throws Exception;
+ /// Switches to mapped segment metadata, for a directory that supports it,
after preprocessing and before the segment
+ /// is queryable. The serving load calls this only when mapped segment
metadata is enabled. The default keeps the
+ /// current metadata.
+ public void loadMappedMetadata()
Review Comment:
Minor (SPI contract): `SegmentLocalFSDirectory` catches mapping failures and
keeps eager metadata. A plugin that throws here fails the load instead:
`load(File, …)` rethrows, and `tryLoadExistingSegment` drops the local copy and
re-downloads. Could the doc ask implementations to fall back rather than throw,
allow repeat calls, and own closing both the replaced and the mapped metadata?
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/SegmentMetadataImplTest.java:
##########
@@ -495,6 +498,50 @@ public void testMappedMetadataRoundTripAndRebuild()
assertEquals(PinotDataBuffer.getMmapBufferCount(), mappings);
}
+ @Test
+ public void testMappedMetadataSubclass()
+ throws Exception {
+ new SegmentV1V2ToV3FormatConverter().convert(_segmentDirectory);
+ SegmentMetadataImpl eager = new SegmentMetadataImpl(_segmentDirectory);
+ long mappings = PinotDataBuffer.getMmapBufferCount();
+ try (MetadataOnlySegmentMetadata mapped = new
MetadataOnlySegmentMetadata(_segmentDirectory)) {
+ assertTrue(mapped.isMapped());
+ assertNull(mapped.getIndexDir());
+ assertEquals(mapped.getName(), eager.getName());
+ assertEquals(mapped.getTotalDocs(), eager.getTotalDocs());
+ assertEquals(mapped.getPhysicalColumnNames(),
eager.getSchema().getPhysicalColumnNames());
+ for (String column : mapped.getPhysicalColumnNames()) {
+ ColumnMetadata actual = mapped.getColumnMetadataFor(column);
+ assertTrue(actual instanceof MappedColumnMetadata, column);
+ assertEquals(((MappedColumnMetadata) actual).getIndexSizeMap(),
+ ((ColumnMetadataImpl)
eager.getColumnMetadataFor(column)).getIndexSizeMap(), column);
+ }
+ SegmentMetadataImpl detached = mapped.detachedCopy();
+ assertFalse(detached.isMapped());
+ assertFalse(detached instanceof MetadataOnlySegmentMetadata);
+ assertNull(detached.getIndexDir());
+ assertEquals(detached.getTotalDocs(), eager.getTotalDocs());
+ }
+ assertEquals(PinotDataBuffer.getMmapBufferCount(), mappings);
+ }
+
+ @Test
+ public void testServingLoadSwitchesAnySegmentDirectoryToMappedMetadata()
+ throws Exception {
+ InstanceDataManagerConfig instanceConfig =
mock(InstanceDataManagerConfig.class);
+ PinotConfiguration config = new PinotConfiguration();
+ when(instanceConfig.getConfig()).thenReturn(config);
+ for (boolean enabled : new boolean[]{true, false}) {
+ config.setProperty(IndexLoadingConfig.MAPPED_SEGMENT_METADATA_ENABLED,
enabled);
+ // Not a SegmentLocalFSDirectory: the switch is part of the
SegmentDirectory contract.
+ SegmentDirectory segmentDirectory = mock(SegmentDirectory.class);
+ when(segmentDirectory.getSegmentMetadata()).thenReturn(
+ new SegmentMetadataImpl("testTable", "emptySegment", new Schema(),
System.currentTimeMillis()));
+ ImmutableSegmentLoader.load(segmentDirectory, new
IndexLoadingConfig(instanceConfig, null, null)).destroy();
+ verify(segmentDirectory, times(enabled ? 1 : 0)).loadMappedMetadata();
Review Comment:
Nit: this test doesn't check order. With the hook moved after
`getSegmentMetadata()`, it still passed locally. An `InOrder` check
(`loadMappedMetadata()` before `getSegmentMetadata()`) would pin the "before
the segment is queryable" contract at the SPI level.
--
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]
