Jackie-Jiang commented on code in PR #19662:
URL: https://github.com/apache/pinot/pull/19662#discussion_r4113637313
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/instance/InstanceDataManagerConfig.java:
##########
@@ -59,6 +59,10 @@ public interface InstanceDataManagerConfig {
int getMaxSegmentPreloadThreads();
+ /// Max amount of mmap'ed segment data to proactively fault into memory on
segment load, in bytes.
+ /// Zero disables prefetching. Only applies when [#getReadMode()] is
[ReadMode#mmap].
+ long getMaxMmapPrefetchBytes();
Review Comment:
[P1] Please give this new SPI method a default implementation returning
`DEFAULT_MMAP_PREFETCH_MAX_SIZE_BYTES`. `IndexLoadingConfig.ImmutableState` now
calls it for every non-null `InstanceDataManagerConfig`; a previously compiled
external implementation lacks the method and will throw `AbstractMethodError`
when its segments load.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/SegmentLocalFSDirectory.java:
##########
@@ -299,6 +300,17 @@ public void close()
}
}
+ private static long toPrefetchPages(long maxMmapPrefetchBytes) {
+ Preconditions.checkArgument(maxMmapPrefetchBytes >= 0, "Max mmap prefetch
bytes must be non-negative, got: %s",
+ maxMmapPrefetchBytes);
+ return maxMmapPrefetchBytes / PAGE_SIZE_BYTES;
+ }
+
+ private static long getMaxMmapPrefetchBytes(@Nullable
SegmentDirectoryLoaderContext segmentDirectoryLoaderContext) {
Review Comment:
[P1] `TierBasedSegmentDirectoryLoader.load()` constructs `new
SegmentLocalFSDirectory(destDir, readMode)`, which supplies no context and
takes this 100 GiB fallback. Even when `ImmutableSegmentLoader` sets the
configured limit, tier-based segments ignore it. Please pass the loader context
through to the directory constructor.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/store/SegmentLocalFSDirectoryTest.java:
##########
@@ -121,6 +122,68 @@ public void testWriteAndReadBackData()
}
}
+ private static SegmentDirectoryLoaderContext prefetchLoaderContext(long
maxMmapPrefetchBytes) {
+ return new SegmentDirectoryLoaderContext.Builder()
+ .setMaxMmapPrefetchBytes(maxMmapPrefetchBytes)
+ .build();
+ }
+
+ /// Prefetching is bounded by a JVM-wide page counter that tests cannot
reset, so rather than asserting on how many
+ /// pages got faulted in, these cases pin down the config contract: the byte
limit is accepted, zero disables
+ /// prefetching, and reads stay correct either way.
+ @Test
+ public void testPrefetchLimitDisabledStillReadsData()
+ throws Exception {
+ File prefetchDir = new File(SegmentLocalFSDirectoryTest.class.getName() +
"-prefetch_disabled");
+ FileUtils.deleteQuietly(prefetchDir);
+ try {
+ FileUtils.copyDirectory(_segmentDirectory.getPath().toFile(),
prefetchDir);
+ // 0 bytes disables prefetching entirely
+ try (SegmentDirectory segmentDirectory = new
SegmentLocalFSDirectory(prefetchDir, _metadata,
+ ReadMode.mmap, prefetchLoaderContext(0))) {
+ try (SegmentDirectory.Writer writer = segmentDirectory.createWriter())
{
+ PinotDataBuffer buffer = writer.newIndexFor("noPrefetchColumn",
StandardIndexes.forward(), 1024);
+ loadData(buffer);
+ writer.save();
+ }
+ try (SegmentDirectory.Reader reader = segmentDirectory.createReader())
{
+ verifyData(reader.getIndexFor("noPrefetchColumn",
StandardIndexes.forward()));
+ }
+ }
+ } finally {
+ FileUtils.deleteQuietly(prefetchDir);
+ }
+ }
+
+ @Test
+ public void testSmallPrefetchLimitStillReadsData()
+ throws Exception {
+ File prefetchDir = new File(SegmentLocalFSDirectoryTest.class.getName() +
"-prefetch_small");
+ FileUtils.deleteQuietly(prefetchDir);
+ try {
+ FileUtils.copyDirectory(_segmentDirectory.getPath().toFile(),
prefetchDir);
+ // 8KB, i.e. a 2 page budget: exercises the slowdown branch that only
faults in header pages
+ try (SegmentDirectory segmentDirectory = new
SegmentLocalFSDirectory(prefetchDir, _metadata,
+ ReadMode.mmap, prefetchLoaderContext(8 * 1024))) {
+ try (SegmentDirectory.Writer writer = segmentDirectory.createWriter())
{
+ PinotDataBuffer buffer = writer.newIndexFor("smallPrefetchColumn",
StandardIndexes.forward(), 1024);
+ loadData(buffer);
+ writer.save();
+ }
+ try (SegmentDirectory.Reader reader = segmentDirectory.createReader())
{
Review Comment:
[P2] The zero and 8 KiB tests assert only that data stays readable, which
also passes if prefetch is always enabled or always disabled. The JVM-wide
`PREFETCHED_PAGES` counter can already exceed the two-page threshold, so this
test may never exercise the slowdown path. Please assert observable prefetch
reads with controlled state, and cover parsing and propagation from the server
configuration key.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/loader/SegmentDirectoryLoaderContext.java:
##########
@@ -104,6 +114,7 @@ public static class Builder {
private String _segmentTier;
private Map<String, Map<String, String>> _instanceTierConfigs;
private Map<String, String> _segmentCustomConfigs;
+ private long _maxMmapPrefetchBytes =
CommonConstants.Server.DEFAULT_MMAP_PREFETCH_MAX_SIZE_BYTES;
Review Comment:
[P1] `BaseTableDataManager.initSegmentDirectory()` builds a context without
calling `setMaxMmapPrefetchBytes`. Startup loads of existing local segments and
reloads without a download therefore use this 100 GiB default; setting
`pinot.server.instance.mmap.prefetch.max.size=0` does not disable their
prefetch. Please forward `indexLoadingConfig.getMaxMmapPrefetchBytes()` in that
builder.
--
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]
