Share single FormatChecksumStrategy instance per shard between engine and store

[ask-kamal-nayan]

Description

ParquetDataFormatPlugin.getFormatDescriptors() creates a new PrecomputedChecksumStrategy() on every call. Since the directory and engine call it independently, they end up with different instances. Also DataFormatPlugin.lastDescriptors where getting updated as new Shards are created as these are node level singleton class. Checksums registered by the engine during writes are invisible to the directory during uploads, forcing an O(n) full-file CRC32 fallback instead of O(1) lookup.

Fix:

Create strategies once per shard in IndexService.createShard() via DataFormatRegistry.createChecksumStrategies() and pass the same map to both DataFormatAwareStoreDirectory and IndexingEngineConfig.

Changes

• Add DataFormatRegistry.createChecksumStrategies() — single creation point
• Add checksumStrategies to IndexingEngineConfig record
• Remove FormatChecksumStrategy param from DataFormatPlugin.indexingEngine()
• DataFormatAwareStoreDirectory accepts pre-built strategies instead of DataFormatRegistry

Testing

• All existing unit tests updated and passing
• Server, sandbox, and test framework compile clean

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit a8dc95f)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

📝 TODO sections server/src/internalClusterTest/java/org/opensearch/index/shard/IndexShardIT.java [738]: Pass proper DataFormatRegistry instead of null test/framework/src/main/java/org/opensearch/index/shard/IndexShardTestCase.java [811]: Pass proper DataFormatRegistry instead of null

🔀 Multiple PR themes Sub-PR theme: Refactor DataFormatPlugin API to use Supplier-based descriptors and remove checksumStrategy parameter Relevant files: server/src/main/java/org/opensearch/index/engine/dataformat/DataFormatRegistry.java server/src/main/java/org/opensearch/index/engine/dataformat/DataFormatPlugin.java server/src/main/java/org/opensearch/index/engine/dataformat/IndexingEngineConfig.java server/src/main/java/org/opensearch/index/engine/dataformat/DataFormatDescriptor.java sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/ParquetDataFormatPlugin.java sandbox/plugins/composite-engine/src/main/java/org/opensearch/composite/CompositeDataFormatPlugin.java sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LucenePlugin.java test/framework/src/main/java/org/opensearch/index/engine/dataformat/stub/MockDataFormatPlugin.java Sub-PR theme: Wire shared checksumStrategies map through shard creation, store directory, and engine config Relevant files: server/src/main/java/org/opensearch/index/store/DataFormatAwareStoreDirectory.java server/src/main/java/org/opensearch/index/store/DataFormatAwareStoreDirectoryFactory.java server/src/main/java/org/opensearch/index/store/DefaultDataFormatAwareStoreDirectoryFactory.java server/src/main/java/org/opensearch/index/IndexService.java server/src/main/java/org/opensearch/index/shard/IndexShard.java server/src/main/java/org/opensearch/index/engine/EngineConfig.java server/src/main/java/org/opensearch/index/engine/EngineConfigFactory.java server/src/main/java/org/opensearch/index/engine/DataFormatAwareEngine.java

⚡ Recommended focus areas for review Strategy Overwrite The constructor initializes checksumStrategies from the passed-in map and then unconditionally calls this.checksumStrategies.put(DEFAULT_FORMAT, new LuceneChecksumHandler()). If the caller passes a map that already contains the DEFAULT_FORMAT key, it will be silently overwritten. Consider checking for key existence before inserting the default. public DataFormatAwareStoreDirectory(Directory delegate, ShardPath shardPath, Map<String, FormatChecksumStrategy> checksumStrategies) { super(new SubdirectoryAwareDirectory(delegate, shardPath)); this.shardPath = shardPath; this.checksumStrategies = new HashMap<>(checksumStrategies); this.checksumStrategies.put(DEFAULT_FORMAT, new LuceneChecksumHandler()); logger.debug( "Created DataFormatAwareStoreDirectory for shard {} with checksum strategies for formats: {}", shardPath.getShardId(), this.checksumStrategies.keySet() ); Incomplete Isolation Test testDifferentIndicesGetIsolatedStrategies asserts assertNotSame(stratA, stratB) but never actually verifies that the checksum registered in stratA is NOT visible in stratB. The test comment says "we can verify the cache is empty" but does not do so, leaving the cross-index contamination scenario only partially validated. public void testDifferentIndicesGetIsolatedStrategies() { MockDataFormat format = new MockDataFormat(FORMAT_NAME, 100L, Set.of()); DataFormatRegistry registry = createRegistry(format); IndexSettings indexSettingsA = createIndexSettings("index_a"); IndexSettings indexSettingsB = createIndexSettings("index_b"); Map<String, FormatChecksumStrategy> strategiesA = registry.createChecksumStrategies(indexSettingsA); Map<String, FormatChecksumStrategy> strategiesB = registry.createChecksumStrategies(indexSettingsB); // Different indices get different strategy instances assertNotSame(strategiesA.get(FORMAT_NAME), strategiesB.get(FORMAT_NAME)); // Register checksum in index A's strategy strategiesA.get(FORMAT_NAME).registerChecksum("_0.parquet", 12345L, 1L); // Index B's strategy should NOT see it PrecomputedChecksumStrategy stratB = (PrecomputedChecksumStrategy) strategiesB.get(FORMAT_NAME); // computeChecksum would fall back to file scan if not cached — but we can verify // the cache is empty by checking that a different checksum isn't magically present PrecomputedChecksumStrategy stratA = (PrecomputedChecksumStrategy) strategiesA.get(FORMAT_NAME); assertNotSame(stratA, stratB); } Null Map Risk checksumStrategies is initialized to Collections.emptyMap() and only populated when isPluggableDataFormatEnabled() && dataFormatRegistry != null. The resulting map is later passed into IndexingEngineConfig and stored in EngineConfig. If downstream code mutates or wraps this map without null-checking, the empty immutable map could cause issues. More importantly, createChecksumStrategies returns an unmodifiable map, but Collections.emptyMap() is also unmodifiable — ensure consistent handling throughout. Map<String, FormatChecksumStrategy> checksumStrategies = Collections.emptyMap(); if (this.indexSettings.isPluggableDataFormatEnabled() && dataFormatRegistry != null) { checksumStrategies = dataFormatRegistry.createChecksumStrategies(this.indexSettings); } Null Format Risk In getFormatDescriptors, dataFormatRegistry.format(primaryFormatName) and dataFormatRegistry.format(secondaryName) may return null if the format name is not registered, and getFormatDescriptors(indexSettings, null) would then be called. There is no null-check on the returned DataFormat before passing it to getFormatDescriptors. if (primaryFormatName != null) { descriptors.putAll(dataFormatRegistry.getFormatDescriptors(indexSettings, dataFormatRegistry.format(primaryFormatName))); } for (String secondaryName : secondaryFormatNames) { if (secondaryName != null) { descriptors.putAll(dataFormatRegistry.getFormatDescriptors(indexSettings, dataFormatRegistry.format(secondaryName))); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to a8dc95f

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent test failure from missing file during checksum lookup The test calls directoryStrategy.computeChecksum(fsDir, "_0_1.parquet") expecting it to return the pre-registered value 3847291056L. However, if PrecomputedChecksumStrategy.computeChecksum falls back to actually reading the file when the cache misses (e.g., due to a key mismatch between registration and lookup), the test will fail with a NoSuchFileException since _0_1.parquet does not exist in the temp directory. The test should verify the cache-hit path explicitly, for example by asserting the cached value is returned without file I/O, or by creating the file before calling computeChecksum. server/src/test/java/org/opensearch/index/engine/dataformat/FormatChecksumStrategySharingTests.java [157-158] +// Verify the checksum registered by the engine is readable from the directory's strategy (O(1) lookup) +// Create a dummy file so fallback file-scan doesn't throw if cache lookup key differs +Path parquetFile = shardDataPath.resolve(ShardPath.INDEX_FOLDER_NAME).resolve("_0_1.parquet"); +Files.write(parquetFile, new byte[0]); long actualChecksum = directoryStrategy.computeChecksum(fsDir, "_0_1.parquet"); assertEquals("Checksum registered by engine must be visible via directory strategy", expectedChecksum, actualChecksum); Suggestion importance[1-10]: 6 __ Why: This is a valid concern: if PrecomputedChecksumStrategy.computeChecksum falls back to file I/O on a cache miss, the test would fail with a NoSuchFileException. Creating the file as a safety net ensures the test doesn't fail for the wrong reason and makes the test more robust.	Low
General	Avoid overwriting existing Lucene checksum strategy The constructor copies the passed-in checksumStrategies into a new HashMap and then adds the default Lucene strategy. However, if the caller passes a map that already contains a "lucene" key, the put will silently overwrite the caller-provided strategy with a new LuceneChecksumHandler. This could cause subtle bugs if a custom Lucene strategy was intentionally provided. Use putIfAbsent to only set the default when no strategy is already registered for that format. server/src/main/java/org/opensearch/index/store/DataFormatAwareStoreDirectory.java [91-92] this.checksumStrategies = new HashMap<>(checksumStrategies); -this.checksumStrategies.put(DEFAULT_FORMAT, new LuceneChecksumHandler()); +this.checksumStrategies.putIfAbsent(DEFAULT_FORMAT, new LuceneChecksumHandler()); Suggestion importance[1-10]: 5 __ Why: Using putIfAbsent instead of put for the default "lucene" strategy prevents silently overwriting a caller-provided Lucene strategy. This is a valid defensive improvement, though in practice the checksumStrategies passed in are created from DataFormatRegistry.createChecksumStrategies which wouldn't include a "lucene" key.	Low
	Return unmodifiable view of checksum strategies map The getChecksumStrategies() method returns the internal map directly without defensive copying or wrapping as unmodifiable. Since checksumStrategies is stored as a field and callers could mutate it, this should return an unmodifiable view to prevent accidental modification of the shared strategies map. server/src/main/java/org/opensearch/index/engine/EngineConfig.java [663-665] public Map<String, FormatChecksumStrategy> getChecksumStrategies() { - return this.checksumStrategies; + return Collections.unmodifiableMap(this.checksumStrategies); } Suggestion importance[1-10]: 4 __ Why: The checksumStrategies field is initialized as Collections.emptyMap() by default and set from the builder, so it's already unmodifiable in the common case. However, if a mutable map is passed via the builder, returning it directly could allow mutation. The suggestion is valid but low-impact since the builder pattern typically controls what's passed in.	Low
	Co-locate strategy creation with directory creation branch The checksumStrategies map is created before the directory is constructed, but it is also passed to IndexShard which later creates the engine. If createChecksumStrategies throws an unexpected exception, the partially-initialized shard could leave resources in an inconsistent state. More critically, the strategies are created unconditionally for all shard types (primary, replica, etc.) even when the pluggable format path is not taken. While the guard condition is present, consider ensuring the strategies are only created when the DataFormatAwareStoreDirectory path is actually taken (i.e., inside the else if branch) to keep the logic co-located and avoid creating strategies that are never used. server/src/main/java/org/opensearch/index/IndexService.java [777-780] Map<String, FormatChecksumStrategy> checksumStrategies = Collections.emptyMap(); -if (this.indexSettings.isPluggableDataFormatEnabled() && dataFormatRegistry != null) { - checksumStrategies = dataFormatRegistry.createChecksumStrategies(this.indexSettings); +// ... +} else { + // Will be enabled in case of formatAware indices. + if (dataFormatRegistry != null) { + checksumStrategies = dataFormatRegistry.createChecksumStrategies(this.indexSettings); + } + directory = createDataFormatAwareStoreDirectory(shardId, path, checksumStrategies); } Suggestion importance[1-10]: 4 __ Why: The suggestion to move strategy creation inside the else branch is a reasonable refactoring to avoid creating strategies that are never used. However, the checksumStrategies map is also passed to IndexShard for engine creation, so it needs to be available outside the directory creation branch — making this refactoring non-trivial and potentially incorrect as shown in the improved_code.	Low

---

Previous suggestions

Suggestions up to commit 72b7b35

Category	Suggestion	Impact
Possible issue	Ensure test file exists before checksum computation The test calls computeChecksum on a PrecomputedChecksumStrategy with a file name "_0_1.parquet" that does not actually exist in fsDir. If PrecomputedChecksumStrategy falls back to reading the file when the checksum is not found in cache, this will throw an IOException rather than returning the expected value, making the test unreliable. Ensure the file exists or verify the strategy returns the cached value without file I/O. server/src/test/java/org/opensearch/index/engine/dataformat/FormatChecksumStrategySharingTests.java [156-157] +// Verify the checksum registered by the engine is readable from the directory's strategy (O(1) lookup) +// Create a dummy file so fallback I/O doesn't fail if cache lookup fails +Path parquetFile = shardDataPath.resolve(ShardPath.INDEX_FOLDER_NAME).resolve("_0_1.parquet"); +Files.write(parquetFile, new byte[0]); long actualChecksum = directoryStrategy.computeChecksum(fsDir, "_0_1.parquet"); assertEquals("Checksum registered by engine must be visible via directory strategy", expectedChecksum, actualChecksum); Suggestion importance[1-10]: 5 __ Why: The test calls computeChecksum on a file "_0_1.parquet" that doesn't physically exist in fsDir. If PrecomputedChecksumStrategy falls back to file I/O when the checksum isn't cached, this would throw an IOException rather than returning the expected value, making the test potentially unreliable.	Low
General	Prevent external mutation of internal strategies map The getChecksumStrategies() method returns the internal map directly. If the map was not initialized as unmodifiable (e.g., passed in from a caller), callers could mutate the engine's internal state. Wrap the return value with Collections.unmodifiableMap to prevent accidental mutation. server/src/main/java/org/opensearch/index/engine/EngineConfig.java [663-665] public Map<String, FormatChecksumStrategy> getChecksumStrategies() { - return this.checksumStrategies; + return Collections.unmodifiableMap(this.checksumStrategies); } Suggestion importance[1-10]: 4 __ Why: The checksumStrategies field is initialized to Collections.emptyMap() by default and set via builder, so it could be mutable if a caller passes a mutable map. Wrapping with Collections.unmodifiableMap is a defensive practice, but the impact is low since the map is typically created internally.	Low
	Prevent external mutation of shard's strategies map The getChecksumStrategies() method exposes the internal map reference directly. Since checksumStrategies is assigned from an external parameter, callers could mutate the shard's internal state. Return an unmodifiable view to protect the field. server/src/main/java/org/opensearch/index/shard/IndexShard.java [642-644] public Map<String, FormatChecksumStrategy> getChecksumStrategies() { - return checksumStrategies; + return Collections.unmodifiableMap(checksumStrategies); Suggestion importance[1-10]: 4 __ Why: The checksumStrategies field is assigned from an external parameter and could be mutable. Returning an unmodifiable view is a valid defensive measure, though the practical risk is low given the controlled usage patterns in this codebase.	Low
	Guard checksum strategy creation against unchecked exceptions createChecksumStrategies is called once per shard creation inside the closeInternal anonymous class, which is correct. However, if createChecksumStrategies throws an unchecked exception (e.g., due to a misconfigured plugin), the shard creation will fail without releasing the already-acquired resources (e.g., directory). Consider placing this call before resource acquisition or ensuring it is covered by the existing try-finally/cleanup block. server/src/main/java/org/opensearch/index/IndexService.java [777-780] Map<String, FormatChecksumStrategy> checksumStrategies = Collections.emptyMap(); -if (this.indexSettings.isPluggableDataFormatEnabled() && dataFormatRegistry != null) { - checksumStrategies = dataFormatRegistry.createChecksumStrategies(this.indexSettings); +try { + if (this.indexSettings.isPluggableDataFormatEnabled() && dataFormatRegistry != null) { + checksumStrategies = dataFormatRegistry.createChecksumStrategies(this.indexSettings); + } +} catch (Exception e) { + throw new IllegalStateException("Failed to create checksum strategies for shard " + routing.shardId(), e); } Suggestion importance[1-10]: 3 __ Why: While the concern about resource leaks is valid in theory, the existing shard creation code already has cleanup mechanisms, and createChecksumStrategies is unlikely to throw unchecked exceptions in normal operation. The suggested wrapping adds minimal practical value.	Low

Suggestions up to commit 72b7b35

Category	Suggestion	Impact
Possible issue	Composite format child strategies may be missing from map The createChecksumStrategies method calls getFormatDescriptors(indexSettings), which for composite formats may only return descriptors for the top-level format (not child formats like "parquet"). This means child format strategies won't be included in the returned map, causing engineConfig.checksumStrategies().get(ParquetDataFormat.PARQUET_DATA_FORMAT_NAME) in ParquetDataFormatPlugin to return null. Consider also iterating over all registered formats or delegating to a composite-aware method. server/src/main/java/org/opensearch/index/engine/dataformat/DataFormatRegistry.java [185-195] public Map<String, FormatChecksumStrategy> createChecksumStrategies(IndexSettings indexSettings) { - Map<String, DataFormatDescriptor> descriptors = getFormatDescriptors(indexSettings); Map<String, FormatChecksumStrategy> strategies = new HashMap<>(); - for (Map.Entry<String, DataFormatDescriptor> entry : descriptors.entrySet()) { - FormatChecksumStrategy strategy = entry.getValue().getChecksumStrategy(); - if (strategy != null) { - strategies.put(entry.getKey(), strategy); + // Collect descriptors from all registered formats to cover composite scenarios + for (DataFormat format : dataFormatPluginRegistry.keySet()) { + Map<String, DataFormatDescriptor> descriptors = getFormatDescriptors(indexSettings, format); + for (Map.Entry<String, DataFormatDescriptor> entry : descriptors.entrySet()) { + FormatChecksumStrategy strategy = entry.getValue().getChecksumStrategy(); + if (strategy != null) { + strategies.putIfAbsent(entry.getKey(), strategy); + } } } return Collections.unmodifiableMap(strategies); } Suggestion importance[1-10]: 6 __ Why: This is a valid concern: getFormatDescriptors(indexSettings) uses the pluggable_dataformat setting to find the top-level format, which for composite formats may not include child format descriptors like "parquet". The ParquetDataFormatPlugin relies on engineConfig.checksumStrategies().get(ParquetDataFormat.PARQUET_DATA_FORMAT_NAME) which could return null if not included. The suggested fix of iterating all registered formats is a reasonable approach.	Low
	Test may fail due to missing file during checksum computation The test calls computeChecksum on a PrecomputedChecksumStrategy with a file name "_0_1.parquet" that does not actually exist in fsDir. If PrecomputedChecksumStrategy falls back to reading the file when the checksum is not found in the cache, this will throw an IOException rather than returning the expected value, making the test unreliable. The test should verify the cached value is returned without a file read, e.g., by asserting the result equals expectedChecksum only after confirming the strategy uses the cache. server/src/test/java/org/opensearch/index/engine/dataformat/FormatChecksumStrategySharingTests.java [156-157] -long actualChecksum = directoryStrategy.computeChecksum(fsDir, "_0_1.parquet"); +// Verify the checksum registered by the engine is readable from the directory's strategy (O(1) lookup) +// Cast to PrecomputedChecksumStrategy to directly verify cache hit without file I/O +PrecomputedChecksumStrategy precomputed = (PrecomputedChecksumStrategy) directoryStrategy; +long actualChecksum = precomputed.computeChecksum(fsDir, "_0_1.parquet"); assertEquals("Checksum registered by engine must be visible via directory strategy", expectedChecksum, actualChecksum); Suggestion importance[1-10]: 5 __ Why: The improved_code is essentially the same as the existing_code (just adds a cast comment), and the actual concern about whether PrecomputedChecksumStrategy.computeChecksum falls back to file I/O when the checksum is cached is valid but depends on the implementation. The suggestion doesn't meaningfully change the code behavior.	Low
General	Prevent external mutation of internal strategies map The getChecksumStrategies() method returns the internal map directly. If the map was not created as unmodifiable (e.g., when passed in via the builder), callers could mutate the engine's internal state. Wrap the return value with Collections.unmodifiableMap to prevent unintended modifications. server/src/main/java/org/opensearch/index/engine/EngineConfig.java [663-665] public Map<String, FormatChecksumStrategy> getChecksumStrategies() { - return this.checksumStrategies; + return Collections.unmodifiableMap(this.checksumStrategies); } Suggestion importance[1-10]: 4 __ Why: The builder's checksumStrategies field is initialized to Collections.emptyMap() and the map passed in is typically already unmodifiable (created via Collections.unmodifiableMap in DataFormatRegistry), so the risk is low. Still a valid defensive programming suggestion.	Low
	Prevent external mutation of shard's strategies map The getChecksumStrategies() method exposes the internal map reference directly. Since checksumStrategies is assigned from an external parameter (which may or may not be unmodifiable), callers could mutate the shard's internal state. Return an unmodifiable view to be safe. server/src/main/java/org/opensearch/index/shard/IndexShard.java [642-644] public Map<String, FormatChecksumStrategy> getChecksumStrategies() { - return checksumStrategies; + return Collections.unmodifiableMap(checksumStrategies); +} Suggestion importance[1-10]: 4 __ Why: The checksumStrategies field is assigned from an external parameter that may already be unmodifiable, but wrapping with Collections.unmodifiableMap is a valid defensive measure to prevent accidental mutation.	Low

Suggestions up to commit 72b7b35

Category	Suggestion	Impact
Possible issue	Ensure directory and engine share identical strategy instances The checksumStrategies map is created once here and then passed to both the directory and the engine (via IndexShard). However, createChecksumStrategies returns an unmodifiable map, while DataFormatAwareStoreDirectory copies it into a mutable HashMap. This means the directory and engine no longer share the same strategy instances — defeating the purpose of the PR. The DataFormatAwareStoreDirectory constructor should store the passed-in strategies directly (or the shared mutable instances should be extracted before wrapping in unmodifiable). server/src/main/java/org/opensearch/index/IndexService.java [777-780] Map<String, FormatChecksumStrategy> checksumStrategies = Collections.emptyMap(); if (this.indexSettings.isPluggableDataFormatEnabled() && dataFormatRegistry != null) { checksumStrategies = dataFormatRegistry.createChecksumStrategies(this.indexSettings); } +// Note: DataFormatAwareStoreDirectory must use the same strategy instances, +// not copies, so that checksums registered by the engine are visible to the directory. Suggestion importance[1-10]: 7 __ Why: This is a real correctness concern: DataFormatAwareStoreDirectory copies the strategies map into a new HashMap, breaking instance sharing between the directory and engine. However, the improved_code only adds a comment and doesn't actually fix the issue, so the suggestion identifies the problem but doesn't provide a proper solution.	Medium
General	Avoid overwriting existing Lucene checksum strategy The constructor copies the incoming checksumStrategies into a new HashMap and then adds the default Lucene strategy. However, if the incoming map already contains a "lucene" key with a custom strategy, it will be silently overwritten by the default LuceneChecksumHandler. The default should only be added if no entry for DEFAULT_FORMAT already exists. server/src/main/java/org/opensearch/index/store/DataFormatAwareStoreDirectory.java [91-92] this.checksumStrategies = new HashMap<>(checksumStrategies); -this.checksumStrategies.put(DEFAULT_FORMAT, new LuceneChecksumHandler()); +this.checksumStrategies.putIfAbsent(DEFAULT_FORMAT, new LuceneChecksumHandler()); Suggestion importance[1-10]: 5 __ Why: Using put unconditionally overwrites any "lucene" strategy that may have been passed in via checksumStrategies, while putIfAbsent would preserve a custom strategy. This is a valid correctness concern, though in practice the default Lucene strategy is unlikely to be overridden.	Low
	Fix test relying on non-existent file for checksum lookup The test registers a checksum with registerChecksum("_0_1.parquet", expectedChecksum, 1L) but then calls computeChecksum(fsDir, "_0_1.parquet") on a real FSDirectory that does not contain the file. If PrecomputedChecksumStrategy falls back to a file scan when the precomputed value is not found (or if the file doesn't exist), this test will throw an IOException rather than asserting the expected value. The test should verify the precomputed path is taken, e.g., by asserting the result without relying on file I/O fallback. server/src/test/java/org/opensearch/index/engine/dataformat/FormatChecksumStrategySharingTests.java [156-157] -long actualChecksum = directoryStrategy.computeChecksum(fsDir, "_0_1.parquet"); +// Verify the checksum registered by the engine is readable from the directory's strategy (O(1) lookup) +// Use the strategy directly to avoid file I/O fallback on a non-existent file +long actualChecksum = sharedStrategy.computeChecksum(fsDir, "_0_1.parquet"); assertEquals("Checksum registered by engine must be visible via directory strategy", expectedChecksum, actualChecksum); Suggestion importance[1-10]: 5 __ Why: The test calls computeChecksum(fsDir, "_0_1.parquet") on a real FSDirectory where the file doesn't exist; if PrecomputedChecksumStrategy falls back to file I/O, this would throw an IOException. The suggestion to call sharedStrategy.computeChecksum directly is reasonable, though the improved code still passes fsDir which may still trigger a fallback if the file is missing.	Low
	Prevent external mutation of internal strategies map The getChecksumStrategies() method returns the internal map directly. If the map was not initialized as unmodifiable (e.g., passed in as a mutable map via the builder), callers could mutate the engine's internal state. The returned map should be wrapped with Collections.unmodifiableMap to prevent accidental modification. server/src/main/java/org/opensearch/index/engine/EngineConfig.java [663-665] public Map<String, FormatChecksumStrategy> getChecksumStrategies() { - return this.checksumStrategies; + return Collections.unmodifiableMap(this.checksumStrategies); } Suggestion importance[1-10]: 4 __ Why: The checksumStrategies field is initialized as Collections.emptyMap() in the builder and assigned directly, so it may be mutable if a caller passes a mutable map via checksumStrategies(). Wrapping with Collections.unmodifiableMap is a defensive improvement, but the risk is low given the current usage patterns.	Low

Suggestions up to commit ed59d73

Category	Suggestion	Impact
General	Protect shared strategies map from external mutation The getChecksumStrategies() method returns the internal map directly. Since checksumStrategies is initialized as Collections.emptyMap() by default but can be set to any map via the builder, callers could potentially mutate it if a mutable map is passed. Return an unmodifiable view to prevent accidental mutation of the shared strategies map. server/src/main/java/org/opensearch/index/engine/EngineConfig.java [663-665] public Map<String, FormatChecksumStrategy> getChecksumStrategies() { - return this.checksumStrategies; + return Collections.unmodifiableMap(this.checksumStrategies); } Suggestion importance[1-10]: 4 __ Why: The suggestion is valid — returning a direct reference to an internal map can allow callers to mutate it. However, the checksumStrategies map is already initialized as Collections.emptyMap() by default and the builder pattern makes it unlikely to be mutated externally. The risk is low but the defensive practice is reasonable.	Low
Possible issue	Ensure composite child format strategies are included createChecksumStrategies calls getFormatDescriptors(indexSettings) which may itself call getFormatDescriptors on composite plugins, potentially creating new strategy instances for child formats. For composite indices, the child format strategies would not be included in the returned map, meaning the composite engine's child engines would not receive the shared strategies. Ensure that for composite formats, the child format descriptors are also included in the strategies map by iterating over all descriptors returned, including those from composite plugin delegation. server/src/main/java/org/opensearch/index/engine/dataformat/DataFormatRegistry.java [185-195] +public Map<String, FormatChecksumStrategy> createChecksumStrategies(IndexSettings indexSettings) { + Map<String, DataFormatDescriptor> descriptors = getFormatDescriptors(indexSettings); + Map<String, FormatChecksumStrategy> strategies = new HashMap<>(); + for (Map.Entry<String, DataFormatDescriptor> entry : descriptors.entrySet()) { + FormatChecksumStrategy strategy = entry.getValue().getChecksumStrategy(); + if (strategy != null) { + strategies.put(entry.getKey(), strategy); + } + } + return Collections.unmodifiableMap(strategies); +} - Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about composite plugins potentially missing child format strategies, but the improved_code is identical to the existing_code — no actual fix is provided. The concern is worth investigating but the suggestion offers no concrete code change to address it.	Low

Suggestions up to commit ed59d73

Category	Suggestion	Impact
General	Avoid overwriting explicitly provided Lucene checksum strategy The constructor copies the passed-in checksumStrategies into a new HashMap and then adds the default Lucene strategy. However, if the caller passes a map that already contains a "lucene" key with a custom strategy, it will be silently overwritten by the put call. Consider using putIfAbsent to preserve any explicitly provided Lucene strategy. server/src/main/java/org/opensearch/index/store/DataFormatAwareStoreDirectory.java [88-92] public DataFormatAwareStoreDirectory(Directory delegate, ShardPath shardPath, Map<String, FormatChecksumStrategy> checksumStrategies) { super(new SubdirectoryAwareDirectory(delegate, shardPath)); this.shardPath = shardPath; this.checksumStrategies = new HashMap<>(checksumStrategies); - this.checksumStrategies.put(DEFAULT_FORMAT, new LuceneChecksumHandler()); + this.checksumStrategies.putIfAbsent(DEFAULT_FORMAT, new LuceneChecksumHandler()); Suggestion importance[1-10]: 5 __ Why: Using putIfAbsent instead of put for the default DEFAULT_FORMAT strategy is a valid defensive improvement — it prevents silently overwriting a caller-provided Lucene strategy. The suggestion is accurate and the improved_code correctly reflects the change.	Low
	Prevent external mutation of checksumStrategies map The getChecksumStrategies() method returns the internal map directly, which could allow callers to mutate it. Since the map is initialized as Collections.emptyMap() by default but may be set to a mutable map via the builder, it should be wrapped in an unmodifiable view to prevent accidental mutation. server/src/main/java/org/opensearch/index/engine/EngineConfig.java [663-665] public Map<String, FormatChecksumStrategy> getChecksumStrategies() { - return this.checksumStrategies; + return Collections.unmodifiableMap(this.checksumStrategies); } Suggestion importance[1-10]: 4 __ Why: The suggestion is valid — returning the internal map directly could allow mutation. However, the builder's default is Collections.emptyMap() (already unmodifiable), and callers passing a map via checksumStrategies() builder method should be aware of ownership. The risk is low but the fix is simple and improves defensive programming.	Low
	Log warning for missing checksum strategies during creation The createChecksumStrategies method silently skips descriptors with a null checksum strategy. This could lead to a NullPointerException later when code assumes a strategy exists for a registered format. Consider logging a warning when a null strategy is encountered to aid debugging. server/src/main/java/org/opensearch/index/engine/dataformat/DataFormatRegistry.java [185-195] public Map<String, FormatChecksumStrategy> createChecksumStrategies(IndexSettings indexSettings) { Map<String, DataFormatDescriptor> descriptors = getFormatDescriptors(indexSettings); Map<String, FormatChecksumStrategy> strategies = new HashMap<>(); for (Map.Entry<String, DataFormatDescriptor> entry : descriptors.entrySet()) { FormatChecksumStrategy strategy = entry.getValue().getChecksumStrategy(); if (strategy != null) { strategies.put(entry.getKey(), strategy); + } else { + logger.warn("No checksum strategy defined for format [{}], skipping", entry.getKey()); } } return Collections.unmodifiableMap(strategies); } Suggestion importance[1-10]: 3 __ Why: Adding a warning log for null checksum strategies is a minor observability improvement. The current silent skip behavior is intentional (some formats may not have checksum strategies), so this is a low-impact suggestion that adds noise without addressing a real bug.	Low
Possible issue	Ensure FSDirectory is closed to prevent resource leaks The test opens an FSDirectory (fsDir) but never closes it explicitly — only directory (the DataFormatAwareStoreDirectory wrapper) is closed. If DataFormatAwareStoreDirectory does not close the underlying FSDirectory, this will leak a file handle. Add a try-finally or use IOUtils.close to ensure fsDir is always closed. server/src/test/java/org/opensearch/index/engine/dataformat/FormatChecksumStrategySharingTests.java [129-160] public void testChecksumVisibleAcrossSharedStrategy() throws IOException { - ... - long actualChecksum = directoryStrategy.computeChecksum(fsDir, "_0_1.parquet"); - assertEquals("Checksum registered by engine must be visible via directory strategy", expectedChecksum, actualChecksum); + MockDataFormat format = new MockDataFormat(FORMAT_NAME, 100L, Set.of()); + DataFormatRegistry registry = createRegistry(format); + IndexSettings indexSettings = createIndexSettings("test_index"); - directory.close(); + Map<String, FormatChecksumStrategy> strategies = registry.createChecksumStrategies(indexSettings); + FormatChecksumStrategy sharedStrategy = strategies.get(FORMAT_NAME); + + long expectedChecksum = 3847291056L; + sharedStrategy.registerChecksum("_0_1.parquet", expectedChecksum, 1L); + + Path tempDir = createTempDir(); + Path shardDataPath = tempDir.resolve("uuid").resolve("0"); + Files.createDirectories(shardDataPath.resolve(ShardPath.INDEX_FOLDER_NAME)); + ShardPath shardPath = new ShardPath(false, shardDataPath, shardDataPath, new ShardId("index", "uuid", 0)); + FSDirectory fsDir = FSDirectory.open(shardDataPath.resolve(ShardPath.INDEX_FOLDER_NAME)); + + try { + DataFormatAwareStoreDirectory directory = new DataFormatAwareStoreDirectory(fsDir, shardPath, strategies); + try { + FormatChecksumStrategy directoryStrategy = directory.getChecksumStrategy(FORMAT_NAME); + assertSame("Directory and engine must share the same strategy instance", sharedStrategy, directoryStrategy); + long actualChecksum = directoryStrategy.computeChecksum(fsDir, "_0_1.parquet"); + assertEquals("Checksum registered by engine must be visible via directory strategy", expectedChecksum, actualChecksum); + } finally { + directory.close(); + } + } finally { + fsDir.close(); + } } Suggestion importance[1-10]: 5 __ Why: The fsDir is opened but only directory is closed in the test. If DataFormatAwareStoreDirectory.close() doesn't propagate to the underlying FSDirectory, this leaks a file handle. The fix using try-finally is correct and prevents resource leaks in tests.	Low

[github-actions]

❌ Gradle check result for b94ced6: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 76721fc

[github-actions]

❌ Gradle check result for 76721fc: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 97671e1

[github-actions]

Persistent review updated to latest commit 69fd004

[github-actions]

Persistent review updated to latest commit 24f399b

[github-actions]

Persistent review updated to latest commit 15280e0

[github-actions]

❌ Gradle check result for 15280e0: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 9e8491b

[github-actions]

✅ Gradle check result for 9e8491b: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 84.37500% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.47%. Comparing base (4ddd084) to head (a8dc95f).
⚠️ Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
...ch/index/engine/dataformat/DataFormatRegistry.java	86.66%	0 Missing and 2 partials ⚠️
...c/main/java/org/opensearch/index/IndexService.java	75.00%	0 Missing and 1 partial ⚠️
...in/java/org/opensearch/index/shard/IndexShard.java	50.00%	1 Missing ⚠️
...e/DefaultDataFormatAwareStoreDirectoryFactory.java	50.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21232      +/-   ##
============================================
+ Coverage     73.45%   73.47%   +0.02%     
- Complexity    74315    74341      +26     
============================================
  Files          5961     5961              
  Lines        337610   337623      +13     
  Branches      48704    48706       +2     
============================================
+ Hits         247985   248072      +87     
+ Misses        69832    69755      -77     
- Partials      19793    19796       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Persistent review updated to latest commit f56754b

[github-actions]

✅ Gradle check result for f56754b: SUCCESS

[github-actions]

Persistent review updated to latest commit ed59d73

[github-actions]

❌ Gradle check result for ed59d73: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit ed59d73

[github-actions]

✅ Gradle check result for ed59d73: SUCCESS

[github-actions]

Persistent review updated to latest commit 72b7b35

[github-actions]

❌ Gradle check result for 72b7b35: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 72b7b35

[github-actions]

❌ Gradle check result for 72b7b35: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 72b7b35

[github-actions]

✅ Gradle check result for 72b7b35: SUCCESS

[github-actions]

Persistent review updated to latest commit a8dc95f

[github-actions]

✅ Gradle check result for a8dc95f: SUCCESS