Skip to content

Commit e089d06

Browse files
authored
Sum file sizes when multiple files in a segment share the same extension (opensearch-project#21000)
Previously, we would only track the size of the last file with a given extension. --------- Signed-off-by: Thy Tran <58045538+ThyTran1402@users.noreply.github.com>
1 parent 81ec182 commit e089d06

2 files changed

Lines changed: 125 additions & 2 deletions

File tree

server/src/main/java/org/opensearch/index/engine/Engine.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1059,10 +1059,10 @@ private Map<String, Long> getSegmentFileSizes(SegmentReader segmentReader) {
10591059
final Directory finalDirectory = directory;
10601060
logger.warn(() -> new ParameterizedMessage("Error when trying to query fileLength [{}] [{}]", finalDirectory, file), e);
10611061
}
1062-
if (length == 0L) {
1062+
if (length == 0L || extension == null) {
10631063
continue;
10641064
}
1065-
map.put(extension, length);
1065+
map.merge(extension, length, Long::sum);
10661066
}
10671067

10681068
if (useCompoundFile) {

server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@
5050
import org.apache.lucene.index.FilterDirectoryReader;
5151
import org.apache.lucene.index.FilterLeafReader;
5252
import org.apache.lucene.index.IndexCommit;
53+
import org.apache.lucene.index.IndexFileNames;
5354
import org.apache.lucene.index.IndexReader;
5455
import org.apache.lucene.index.IndexWriter;
5556
import org.apache.lucene.index.IndexWriterConfig;
@@ -64,6 +65,7 @@
6465
import org.apache.lucene.index.NumericDocValues;
6566
import org.apache.lucene.index.PointValues;
6667
import org.apache.lucene.index.SegmentInfos;
68+
import org.apache.lucene.index.SegmentReader;
6769
import org.apache.lucene.index.SoftDeletesRetentionMergePolicy;
6870
import org.apache.lucene.index.StoredFields;
6971
import org.apache.lucene.index.Term;
@@ -138,6 +140,7 @@
138140
import org.opensearch.index.mapper.DocumentMapper;
139141
import org.opensearch.index.mapper.DocumentMapperForType;
140142
import org.opensearch.index.mapper.IdFieldMapper;
143+
import org.opensearch.index.mapper.MappedFieldType;
141144
import org.opensearch.index.mapper.MapperService;
142145
import org.opensearch.index.mapper.ParseContext;
143146
import org.opensearch.index.mapper.ParseContext.Document;
@@ -243,6 +246,7 @@
243246
import static org.hamcrest.Matchers.not;
244247
import static org.hamcrest.Matchers.notNullValue;
245248
import static org.hamcrest.Matchers.nullValue;
249+
import static org.mockito.ArgumentMatchers.any;
246250
import static org.mockito.Mockito.atLeastOnce;
247251
import static org.mockito.Mockito.doAnswer;
248252
import static org.mockito.Mockito.mock;
@@ -9352,4 +9356,123 @@ private ParsedDocument createDocumentWithNestedField(String id, String contactNa
93529356
return testParsedDocument(id, null, testDocumentWithTextField(), source, null);
93539357
}
93549358

9359+
/**
9360+
* Verifies that {@code getSegmentFileSizes} correctly accumulates sizes for all files in a
9361+
* segment, including multiple files that share the same extension.
9362+
*
9363+
* <p>When fuzzy-set-for-doc-ID is enabled, {@link
9364+
* org.opensearch.index.codec.PerFieldMappingPostingFormatCodec} assigns
9365+
* {@code FuzzyFilterPostingsFormat} to the {@code _id} field and the standard Lucene format to
9366+
* all other text fields. Because these are two distinct {@code PostingsFormat} implementations,
9367+
* Lucene's {@code PerFieldPostingsFormat} writes a separate file group for each, producing
9368+
* multiple files with the same extension in one segment (e.g. two {@code .tim} files, two
9369+
* {@code .doc} files, etc.).
9370+
*
9371+
* <p>The bug was that {@code getSegmentFileSizes} used {@code Map.put(extension, length)},
9372+
* which silently overwrote earlier entries, causing the reported {@code file_sizes} total to be
9373+
* less than the actual on-disk size. The fix replaces {@code put} with
9374+
* {@code map.merge(extension, length, Long::sum)} so every file's bytes are counted.
9375+
*/
9376+
public void testSegmentFileSizesAccumulatesAllFilesIncludingDuplicateExtensions() throws Exception {
9377+
// Disable compound file so each Lucene file is a separate entry in the directory,
9378+
// making it straightforward to compare the expected total (sum of file lengths from the
9379+
// directory) against the actual total reported by segmentsStats.
9380+
IndexSettings indexSettings = IndexSettingsModule.newIndexSettings(
9381+
"test_file_sizes",
9382+
Settings.builder().put(defaultSettings.getSettings()).put(EngineConfig.INDEX_USE_COMPOUND_FILE.getKey(), false).build()
9383+
);
9384+
// Enable fuzzy set for doc ID so that _id uses FuzzyFilterPostingsFormat while other
9385+
// text fields use the standard Lucene format. Two distinct PostingsFormat instances
9386+
// in one segment cause PerFieldPostingsFormat to write two file groups that share
9387+
// extensions, which is exactly the condition that exposed the map.put() bug.
9388+
indexSettings.setEnableFuzzySetForDocId(true);
9389+
9390+
// Mock MapperService so that PerFieldMappingPostingFormatCodec sees a non-null field
9391+
// type for _id (required for the FuzzyFilter branch to be reached).
9392+
MappedFieldType idFieldType = mock(MappedFieldType.class);
9393+
when(idFieldType.unwrap()).thenReturn(idFieldType);
9394+
MapperService mapperService = mock(MapperService.class);
9395+
when(mapperService.fieldType(any())).thenReturn(null);
9396+
when(mapperService.fieldType(IdFieldMapper.NAME)).thenReturn(idFieldType);
9397+
when(mapperService.getIndexSettings()).thenReturn(indexSettings);
9398+
when(mapperService.isCompositeIndexPresent()).thenReturn(false);
9399+
9400+
CodecService codecService = new CodecService(mapperService, indexSettings, logger, List.of());
9401+
9402+
try (Store store = createStore()) {
9403+
Path translogPath = createTempDir();
9404+
// Build a base config then rebuild it with our custom CodecService.
9405+
EngineConfig base = config(indexSettings, store, translogPath, NoMergePolicy.INSTANCE, null, null, null);
9406+
EngineConfig engineConfig = new EngineConfig.Builder().shardId(base.getShardId())
9407+
.threadPool(base.getThreadPool())
9408+
.indexSettings(indexSettings)
9409+
.warmer(base.getWarmer())
9410+
.store(store)
9411+
.mergePolicy(NoMergePolicy.INSTANCE)
9412+
.analyzer(base.getAnalyzer())
9413+
.similarity(base.getSimilarity())
9414+
.codecService(codecService)
9415+
.eventListener(base.getEventListener())
9416+
.queryCache(base.getQueryCache())
9417+
.queryCachingPolicy(base.getQueryCachingPolicy())
9418+
.translogConfig(base.getTranslogConfig())
9419+
.flushMergesAfter(base.getFlushMergesAfter())
9420+
.externalRefreshListener(base.getExternalRefreshListener())
9421+
.internalRefreshListener(base.getInternalRefreshListener())
9422+
.indexSort(base.getIndexSort())
9423+
.circuitBreakerService(base.getCircuitBreakerService())
9424+
.globalCheckpointSupplier(base.getGlobalCheckpointSupplier())
9425+
.retentionLeasesSupplier(base.retentionLeasesSupplier())
9426+
.primaryTermSupplier(base.getPrimaryTermSupplier())
9427+
.tombstoneDocSupplier(base.getTombstoneDocSupplier())
9428+
.build();
9429+
9430+
try (InternalEngine engine = createEngine(engineConfig)) {
9431+
// Index one document. The _id field goes through FuzzyFilterPostingsFormat;
9432+
// the "value" text field goes through the standard format. Both end up in the
9433+
// same segment, guaranteeing multiple files per extension.
9434+
ParsedDocument doc = testParsedDocument("1", null, testDocumentWithTextField(), SOURCE, null);
9435+
engine.index(indexForDoc(doc));
9436+
engine.flush(true, true);
9437+
engine.refresh("test");
9438+
9439+
// Compute the expected total: sum of the actual on-disk lengths of every file
9440+
// that belongs to the flushed segment.
9441+
long expectedTotal = 0;
9442+
try (Engine.Searcher searcher = engine.acquireSearcher("test")) {
9443+
for (LeafReaderContext ctx : searcher.getIndexReader().getContext().leaves()) {
9444+
SegmentReader segmentReader = Lucene.segmentReader(ctx.reader());
9445+
for (String file : segmentReader.getSegmentInfo().files()) {
9446+
if (IndexFileNames.getExtension(file) == null) {
9447+
continue;
9448+
}
9449+
long len = store.directory().fileLength(file);
9450+
if (len == 0L) {
9451+
continue;
9452+
}
9453+
expectedTotal += len;
9454+
}
9455+
}
9456+
}
9457+
assertThat("expected at least one segment file after flush", expectedTotal, greaterThan(0L));
9458+
9459+
// Compute the actual total reported by segmentsStats with file sizes enabled.
9460+
SegmentsStats stats = engine.segmentsStats(true, false);
9461+
Map<String, Long> fileSizes = stats.getFileSizes();
9462+
assertFalse("file_sizes must not be empty when include_segment_file_sizes=true", fileSizes.isEmpty());
9463+
long actualTotal = fileSizes.values().stream().mapToLong(Long::longValue).sum();
9464+
9465+
// With the old map.put() bug, actualTotal < expectedTotal whenever the codec
9466+
// writes more than one file per extension (as the FuzzyFilter + standard
9467+
// postings formats do). With the fix (map.merge(Long::sum)) all bytes are counted.
9468+
assertEquals(
9469+
"file_sizes total must equal the actual sum of all segment file sizes; "
9470+
+ "a mismatch means some files were silently dropped due to duplicate extensions",
9471+
expectedTotal,
9472+
actualTotal
9473+
);
9474+
}
9475+
}
9476+
}
9477+
93559478
}

0 commit comments

Comments
 (0)