DataFormat-aware NRT replication engine and remote-store wiring

[ask-kamal-nayan]

Description

Summary

Wires up remote-store segment replication and peer recovery for DataFormatAware (DFA) indices. Adds DataFormatAwareNRTReplicationEngine and the upload/replica plumbing needed for a DFA primary and replica to agree on segment state via remote store.

Key changes

• CatalogSnapshot: polymorphic getSegmentInfosBytes() so uploader replica handle Lucene and DFA snapshots uniformly.
• DataFormatAwareEngine: attaches (SegmentInfos bytes, generation) after commit so uploads carry a consistent pair. UnblocksacquireSafeIndexCommit for peer recovery phase-1.
• DataFormatAwareNRTReplicationEngine: Full implementation of replica engine for DFA;
• Replication path: RemoteStoreReplicationSource, SegmentReplicationTarget, IndexShard.finalizeReplication, and RemoteStoreRefreshListener now route DFA snapshots end-to-end.

Tests

• New DataFormatAwareUploadIT (5) — parquet files uploaded; metadata round-trip; per-refresh checkpoint advance; format-aware keys; multi-flush guard.
• New DataFormatAwareReplicationIT (3) — single-refresh convergence; catalog-snapshot round-trip; metadata-format agreement.
• Unit tests updated;

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 3925bac)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue Line 159 contains an assertion that will always fail: assert false == committed.isEmpty(). The assertion expects at least one commit to be present for a replica, but the constructor logic immediately before (lines 150-160) bootstraps an empty commit if none exists. This means the assertion can fail during fresh replica initialization when committer.listCommittedSnapshots() returns an empty list, causing the engine to fail to start. The assertion should either be removed or the bootstrap logic should be moved after the assertion. assert false == committed.isEmpty() : "At least one commit should be present for replica"; // ???????? Possible Issue In snapshotStoreMetadata(), the code acquires wrappedCatalogSnapshot inside synchronized (engineMutex) at line 2326 but releases it outside the synchronized block at line 2339. If the engine is concurrently closed or replaced between releasing the mutex and closing the snapshot, the snapshot's underlying resources may already be invalid, leading to potential resource leaks or exceptions. The snapshot should be closed within the same synchronized block where it was acquired, or the code should handle AlreadyClosedException during the close operation. GatedCloseable<CatalogSnapshot> wrappedCatalogSnapshot = null; store.incRef(); try { synchronized (engineMutex) { // if the engine is not running, we can access the store directly, but we need to make sure no one starts // the engine on us. If the engine is running, we can get a snapshot via the deletion policy of the engine. final Indexer indexer = getIndexerOrNull(); if (indexer != null) { wrappedCatalogSnapshot = indexer.acquireLastCommittedSnapshot(false); } if (wrappedCatalogSnapshot == null) { Store.MetadataSnapshot ms = store.getMetadata(null, true); logSnapshotStoreMetadataSummary(ms, "null-engine"); return ms; } } Store.MetadataSnapshot ms = store.getMetadata(wrappedCatalogSnapshot.get()); logSnapshotStoreMetadataSummary(ms, "catalog"); return ms; } finally { store.decRef(); IOUtils.close(wrappedCatalogSnapshot); Possible Issue In commitCatalogSnapshot() at line 316, if committer.commit() returns null (indicating no commit was performed), the code still calls catalogSnapshotManager.updateLastCommitInfo(commitResult) with a null argument. This will cause a NullPointerException. The update should only be called when commitResult is non-null. new CommitInput(commitData.entrySet(), snapshot, bumpSICounter ? SI_COUNTER_INCREMENT : 0) ); if (commitResult != null) { catalogSnapshotManager.updateLastCommitInfo(commitResult); } Possible Issue In copySegmentFiles() at line 6042, the code attempts to parse a checksum string as a long with Long.parseLong(dfasd.calculateUploadChecksum(file)). If calculateUploadChecksum() returns a non-numeric string (e.g., a hex checksum or an error message), this will throw NumberFormatException. The catch block logs a warning and returns false, but this could mask legitimate checksum mismatches. The code should validate the checksum format or document the expected format contract. } catch (NumberFormatException e) { logger.warn("Invalid checksum format for file [{}]: {}", file, e.getMessage()); return false; } } else { Possible Issue In updateCatalogSnapshot() at line 266, if incomingCommitGeneration != lastReceivedPrimaryCommitGen, the code calls flush(false, true, false) which may throw IOException. If the flush fails, lastReceivedPrimaryCommitGen is never updated (line 270), but localCheckpointTracker.fastForwardProcessedSeqNo(maxSeqNo) at line 272 has already advanced the checkpoint. This leaves the replica in an inconsistent state where the checkpoint has advanced but the commit generation has not. The checkpoint advancement should occur after the flush succeeds, or the flush failure should roll back the checkpoint. if (incomingCommitGeneration != lastReceivedPrimaryCommitGen) { flush(false, true, false); translogManager.getDeletionPolicy().setLocalCheckpointOfSafeCommit(maxSeqNo); translogManager.rollTranslogGeneration(); } lastReceivedPrimaryCommitGen = incomingCommitGeneration; localCheckpointTracker.fastForwardProcessedSeqNo(maxSeqNo); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 3925bac

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix document count calculation logic Using findFirst() on a stream of numRows values returns only the first format's row count, which may not represent the total document count across all data formats. If multiple formats exist, this could return an incorrect count. Consider using sum() or clarifying the intended semantics. server/src/main/java/org/opensearch/index/engine/dataformat/merge/DataFormatAwareMergePolicy.java [202] private long calculateNumDocs(Segment segment) { - return segment.dfGroupedSearchableFiles().values().stream().mapToLong(WriterFileSet::numRows).findFirst().orElse(0L); + return segment.dfGroupedSearchableFiles().values().stream().mapToLong(WriterFileSet::numRows).sum(); } Suggestion importance[1-10]: 9 __ Why: The change from sum() to findFirst() in the PR appears incorrect. Using findFirst() only returns the row count from one data format, which would undercount documents when multiple formats exist. This is a critical logic error that could affect merge policy decisions and data integrity.	High
	Handle null commit result explicitly If committer.commit() returns null, updateLastCommitInfo() is skipped but markSuccess() is still called. This may leave the catalog manager in an inconsistent state where the snapshot is marked committed but the commit info is stale. Verify that null is a valid return value and handle it explicitly, or throw an exception if a commit must always produce a result. server/src/main/java/org/opensearch/index/engine/DataFormatAwareNRTReplicationEngine.java [313-319] -private void commitCatalogSnapshot(boolean bumpSICounter) throws IOException { - try (GatedConditionalCloseable<CatalogSnapshot> snapshotRef = catalogSnapshotManager.acquireSnapshotForCommit()) { - CatalogSnapshot snapshot = snapshotRef.get(); - ... - CommitResult commitResult = committer.commit( - new CommitInput(commitData.entrySet(), snapshot, bumpSICounter ? SI_COUNTER_INCREMENT : 0) - ); - if (commitResult != null) { - catalogSnapshotManager.updateLastCommitInfo(commitResult); - } - snapshotRef.markSuccess(); - } - translogManager.syncTranslog(); +CommitResult commitResult = committer.commit( + new CommitInput(commitData.entrySet(), snapshot, bumpSICounter ? SI_COUNTER_INCREMENT : 0) +); +if (commitResult == null) { + throw new IOException("Committer returned null result for snapshot " + snapshot.getId()); } +catalogSnapshotManager.updateLastCommitInfo(commitResult); +snapshotRef.markSuccess(); Suggestion importance[1-10]: 8 __ Why: If committer.commit() returns null, updateLastCommitInfo() is skipped but markSuccess() is still called, potentially leaving the catalog manager in an inconsistent state. The suggestion correctly identifies this as a possible issue and proposes throwing an exception to fail fast.	Medium
	Clarify replica bootstrap commit requirement The assertion message and comment suggest uncertainty about replica bootstrap state. If a fresh replica can legitimately have zero commits, this assertion will fail. Remove the assertion or clarify the bootstrap contract to ensure listCommittedSnapshots() always returns at least one synthetic commit for replicas. server/src/main/java/org/opensearch/index/engine/DataFormatAwareNRTReplicationEngine.java [158-159] List<CatalogSnapshot> committed = committer.listCommittedSnapshots(); -assert false == committed.isEmpty() : "At least one commit should be present for replica"; // ???????? +if (committed.isEmpty()) { + throw new EngineCreationFailureException(shardId, "No committed snapshots found for replica during bootstrap"); +} Suggestion importance[1-10]: 7 __ Why: The assertion with the "????????" comment indicates uncertainty about the bootstrap contract. The improved code replaces the assertion with an explicit exception, making the failure mode clearer and preventing potential assertion-disabled production issues.	Medium
General	Fix log level check mismatch The condition checks logger.isInfoEnabled() but then calls logger.debug(). This mismatch means debug logs will be skipped when only INFO level is enabled. Either change the condition to isDebugEnabled() or change the log call to logger.info() to match the intended log level. server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryTargetService.java [339-363] -if (logger.isInfoEnabled()) { +if (logger.isDebugEnabled()) { int total = metadataSnapshot.size(); java.util.List<String> dfaFiles = metadataSnapshot.asMap() .keySet() .stream() .filter(name -> name.contains("/")) .sorted() .collect(java.util.stream.Collectors.toList()); String luceneSample = metadataSnapshot.asMap() .keySet() .stream() .filter(name -> name.contains("/") == false) .sorted() .limit(5) .collect(java.util.stream.Collectors.joining(", ")); logger.debug( "[DFA-RECOVERY] peer recovery TARGET sending metadata to source shardId={} totalFiles={} " + "dfaFiles={} dfaList=[{}] luceneSample=[{}]", recoveryTarget.shardId(), total, dfaFiles.size(), String.join(", ", dfaFiles), luceneSample ); } Suggestion importance[1-10]: 8 __ Why: The condition checks logger.isInfoEnabled() but calls logger.debug(), which is a clear mismatch. This means the debug logs will never be printed when only INFO level is enabled, defeating the purpose of the guard condition. This is a functional bug that affects logging behavior.	Medium
	Handle missing files gracefully If directory.fileLength(file) or directory.openInput(file, ...) throws a NoSuchFileException for a file in the input collection, the method propagates the exception and the caller receives a partial result map. Wrap file-level operations in a try-catch to skip missing files or collect failures, ensuring the method either returns metadata for all requested files or throws a single aggregated exception. server/src/main/java/org/opensearch/index/store/Store.java [511-529] -public Map<String, StoreFileMetadata> getFileMetadata(Collection<String> files) throws IOException { - failIfCorrupted(); - Map<String, StoreFileMetadata> result = new HashMap<>(); - for (String file : files) { +Map<String, StoreFileMetadata> result = new HashMap<>(); +List<String> failures = new ArrayList<>(); +for (String file : files) { + try { final long length = directory.fileLength(file); final String checksum; final DataFormatAwareStoreDirectory dfasd = DataFormatAwareStoreDirectory.unwrap(directory); if (dfasd != null && DataFormatAwareStoreDirectory.isDefaultFormat(FileMetadata.parseDataFormat(file)) == false) { checksum = dfasd.calculateUploadChecksum(file); } else { try (IndexInput in = directory.openInput(file, IOContext.READONCE)) { checksum = Store.digestToString(CodecUtil.retrieveChecksum(in)); } } result.put(file, new StoreFileMetadata(file, length, checksum, org.opensearch.Version.CURRENT.luceneVersion)); + } catch (NoSuchFileException e) { + failures.add(file); } - return result; } +if (!failures.isEmpty()) { + throw new IOException("Missing files during metadata computation: " + failures); +} +return result; Suggestion importance[1-10]: 7 __ Why: The method can throw NoSuchFileException mid-iteration, leaving a partial result. The suggestion proposes collecting failures and throwing an aggregated exception, which improves robustness and provides better error reporting to the caller.	Medium
	Validate snapshot type before cast The method casts incoming to DataformatAwareCatalogSnapshot without validation. If a non-DFA snapshot is passed (e.g., during mixed-version cluster upgrades or plugin misconfiguration), this will throw ClassCastException. Add a type check before the cast to fail fast with a clear error message. server/src/main/java/org/opensearch/index/engine/exec/coord/CatalogSnapshotManager.java [288-316] public synchronized void applyReplicationSnapshot(CatalogSnapshot incoming) throws IOException { if (closed.get()) { throw new IllegalStateException("CatalogSnapshotManager is closed"); + } + if (!(incoming instanceof DataformatAwareCatalogSnapshot)) { + throw new IllegalArgumentException("incoming snapshot must be DataformatAwareCatalogSnapshot, got: " + incoming.getClass()); } for (CatalogSnapshotLifecycleListener listener : snapshotListeners) { listener.beforeRefresh(); } DataformatAwareCatalogSnapshot newSnapshot = new DataformatAwareCatalogSnapshot( latestCatalogSnapshot.getId() + 1, latestCatalogSnapshot.getGeneration() + 1, incoming.getVersion(), incoming.getSegments(), latestCatalogSnapshot.getLastWriterGeneration() + 1, incoming.getUserData(), latestCatalogSnapshot.getLastCommitFileName(), latestCatalogSnapshot.getLastCommitGeneration(), latestCatalogSnapshot.getCommitDataFormatVersion() ); newSnapshot.setReplicatingCommitData(((DataformatAwareCatalogSnapshot) incoming).getReplicatingCommitData()); installSnapshot(newSnapshot); } Suggestion importance[1-10]: 7 __ Why: The unchecked cast to DataformatAwareCatalogSnapshot at line 313 is a real risk during mixed-version upgrades or plugin misconfiguration. Adding a type check before the cast would prevent a cryptic ClassCastException and provide a clear error message. This is a correctness improvement with moderate impact.	Medium
	Complete cleanup despite partial failures The method silently ignores NoSuchFileException but may fail on other IOExceptions during container deletion, leaving some containers undeleted. Consider collecting all exceptions and attempting to delete all containers before throwing, ensuring cleanup is as complete as possible. server/src/main/java/org/opensearch/index/store/remote/DataFormatAwareRemoteDirectory.java [320-337] public void delete() throws IOException { + List<IOException> exceptions = new ArrayList<>(); for (String format : formatBlobRouter.registeredFormats()) { try { formatBlobRouter.containerFor(format).delete(); } catch (java.nio.file.NoSuchFileException ignored) { - // already deleted — nothing to do + } catch (IOException e) { + exceptions.add(e); } } try { super.delete(); } catch (java.nio.file.NoSuchFileException ignored) { - // already deleted + } catch (IOException e) { + exceptions.add(e); + } + if (!exceptions.isEmpty()) { + IOException first = exceptions.get(0); + exceptions.stream().skip(1).forEach(first::addSuppressed); + throw first; } logger.debug("Deleted all containers from DataFormatAwareRemoteDirectory"); } Suggestion importance[1-10]: 7 __ Why: The suggestion to collect all exceptions and attempt deletion of all containers before throwing improves cleanup robustness. This ensures that a failure in one container deletion does not prevent cleanup of others, which is important for resource management. The improved code correctly aggregates exceptions and throws them after attempting all deletions.	Medium
	Log commit failures during close On close, if commitCatalogSnapshot() throws an IOException and the store is not already corrupted, the code marks the store corrupted. However, if the engine is closing due to a prior failure (failEngineLock.isHeldByCurrentThread()), the exception is silently swallowed. This may hide critical commit failures during shutdown. Log the exception at ERROR level even when the engine is already failed to aid debugging. server/src/main/java/org/opensearch/index/engine/DataFormatAwareNRTReplicationEngine.java [571-584] -private void closeNoLock(String reason) { - if (isClosed.compareAndSet(false, true)) { - ... +} catch (IOException e) { + if (failEngineLock.isHeldByCurrentThread() == false && store.isMarkedCorrupted() == false) { try { - if (engineConfig.getIndexSettings().isWarmIndex() == false) { - try { - final boolean bumpCounter = engineConfig.getIndexSettings().isRemoteStoreEnabled() == false - && engineConfig.getIndexSettings().isAssignedOnRemoteNode() == false; - commitCatalogSnapshot(bumpCounter); - } catch (IOException e) { - if (failEngineLock.isHeldByCurrentThread() == false && store.isMarkedCorrupted() == false) { - try { - store.markStoreCorrupted(e); - } catch (IOException ex) { - logger.warn("Unable to mark store corrupted", ex); - } - } - } - } - ... + store.markStoreCorrupted(e); + } catch (IOException ex) { + logger.warn("Unable to mark store corrupted", ex); + } + } else { + logger.error("Failed to commit catalog snapshot during close (engine already failed or store corrupted)", e); + } +} Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that commit failures during close are silently swallowed when the engine is already failed. Adding error-level logging would aid debugging, though this is a minor improvement since the engine is already in a failed state.	Low
	Return immutable collection from getFiles The method returns a mutable HashSet that callers could accidentally modify, potentially corrupting the catalog's file list. Return an immutable collection to prevent external mutation and enforce the catalog's read-only contract. server/src/main/java/org/opensearch/index/engine/exec/coord/DataformatAwareCatalogSnapshot.java [391-401] @Override public Collection<String> getFiles(boolean includeSegmentsFile) throws IOException { Set<String> fileNames = new HashSet<>(); for (Segment segment : segments) { for (WriterFileSet wfs : segment.dfGroupedSearchableFiles().values()) { for (String file : wfs.files()) { fileNames.add(file); } } } if (includeSegmentsFile) { String segFile = getLastCommitFileName(); if (segFile != null) { fileNames.add(segFile); } } - return fileNames; + return Collections.unmodifiableSet(fileNames); } Suggestion importance[1-10]: 6 __ Why: Returning a mutable HashSet from getFiles violates the catalog's read-only contract and could allow external mutation. Wrapping the result in Collections.unmodifiableSet is a straightforward defensive programming improvement. The impact is moderate since the current code doesn't show evidence of mutation bugs, but the fix prevents future issues.	Low
	Preserve original exception type The method catches all exceptions from the callback but wraps them in RuntimeException, potentially losing critical error context. Consider preserving the original exception type when it's already a RuntimeException to maintain proper exception semantics and stack traces. server/src/main/java/org/opensearch/index/engine/exec/coord/IndexFileDeleter.java [317-335] void decRefAndMaybeDelete(CatalogSnapshot snapshot) { if (snapshot.decRef()) { Exception firstException = null; try { onSnapshotDeletedCallback.accept(snapshot); } catch (Exception e) { firstException = e; } try { this.removeFileReferences(snapshot); } catch (IOException e) { if (firstException == null) firstException = e; else firstException.addSuppressed(e); } if (firstException != null) { + if (firstException instanceof RuntimeException) { + throw (RuntimeException) firstException; + } throw new RuntimeException("Failed to clean up snapshot [gen=" + snapshot.getGeneration() + "]", firstException); } } } Suggestion importance[1-10]: 6 __ Why: Preserving the original RuntimeException type avoids unnecessary wrapping and maintains clearer stack traces. This is a valid improvement for exception handling semantics, though the impact is moderate since the wrapped exception still contains the original cause.	Low
	Log I/O errors at warn level The method swallows IOException during checksum eviction, which could mask critical issues like disk failures or corrupted catalog state. While eviction is optimization, persistent I/O errors should be logged at a higher level (warn) to alert operators. Consider logging IOException at warn level and only swallowing AlreadyClosedException at debug. server/src/main/java/org/opensearch/index/shard/IndexShard.java [1912-1922] private void evictStaleDownloadedChecksums(CatalogSnapshot catalogSnapshot) { try { DataFormatAwareStoreDirectory dfasd = DataFormatAwareStoreDirectory.unwrap(store.directory()); if (dfasd == null) { return; // non-DFA index — no precomputed cache to evict from } dfasd.evictStaleChecksums(catalogSnapshot.getFiles(true)); - } catch (AlreadyClosedException | IOException e) { + } catch (AlreadyClosedException e) { logger.debug("skipped precomputed checksum eviction on replica", e); + } catch (IOException e) { + logger.warn("failed to evict stale checksums on replica", e); } } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that IOException is swallowed at debug level, which could mask disk failures. However, the method's Javadoc explicitly states "Failures here are logged and swallowed — cache eviction is optimization, not correctness," so the current behavior is intentional. Elevating to warn is a reasonable improvement for persistent I/O errors, but the impact is moderate since eviction failures don't affect correctness.	Low
	Optimize checksum eviction iteration The method iterates over all checksum strategies but only processes non-default formats. Consider filtering the entry set first to avoid unnecessary iterations and instanceof checks for default formats. This improves performance when multiple formats are registered. server/src/main/java/org/opensearch/index/store/DataFormatAwareStoreDirectory.java [323-334] public void evictStaleChecksums(Collection<String> currentSnapshotFiles) { - for (Map.Entry<String, FormatChecksumStrategy> entry : checksumStrategies.entrySet()) { - if (isDefaultFormat(entry.getKey())) { - continue; - } - if (entry.getValue() instanceof PrecomputedChecksumStrategy precomputed) { + checksumStrategies.entrySet().stream() + .filter(entry -> !isDefaultFormat(entry.getKey())) + .filter(entry -> entry.getValue() instanceof PrecomputedChecksumStrategy) + .forEach(entry -> { String prefix = entry.getKey() + "/"; - Set<String> activeForFormat = currentSnapshotFiles.stream().filter(f -> f.startsWith(prefix)).collect(Collectors.toSet()); - precomputed.retainOnly(activeForFormat); - } - } + Set<String> activeForFormat = currentSnapshotFiles.stream() + .filter(f -> f.startsWith(prefix)) + .collect(Collectors.toSet()); + ((PrecomputedChecksumStrategy) entry.getValue()).retainOnly(activeForFormat); + }); } Suggestion importance[1-10]: 5 __ Why: The suggestion to use streams and filter upfront is a valid optimization that reduces unnecessary iterations and instanceof checks. However, the performance gain is marginal in typical scenarios (few formats registered), and the existing code is more readable. The suggestion is correct but offers only a minor improvement.	Low
	Warn on missing metadata entries The checksum registration loop iterates over toDownloadSegments but only registers checksums for files present in uploadedSegments. If a file is in toDownloadSegments but missing from uploadedSegments (e.g., due to metadata inconsistency), it's silently skipped. This could lead to cache misses during subsequent operations. Consider logging a warning when a downloaded file has no metadata entry. server/src/main/java/org/opensearch/index/shard/IndexShard.java [6009-6020] -private String copySegmentFiles( - ... -) throws IOException { - ... - if (toDownloadSegments.isEmpty() == false) { - try { - fileDownloader.download(sourceRemoteDirectory, storeDirectory, targetRemoteDirectory, toDownloadSegments, onFileSync); - DataFormatAwareStoreDirectory dfasd = DataFormatAwareStoreDirectory.unwrap(storeDirectory); - if (dfasd != null) { - Map<String, String> fileToChecksum = new HashMap<>(); - for (String file : toDownloadSegments) { - UploadedSegmentMetadata meta = uploadedSegments.get(file); - if (meta != null) { - fileToChecksum.put(file, meta.getChecksum()); - } +if (toDownloadSegments.isEmpty() == false) { + try { + fileDownloader.download(sourceRemoteDirectory, storeDirectory, targetRemoteDirectory, toDownloadSegments, onFileSync); + DataFormatAwareStoreDirectory dfasd = DataFormatAwareStoreDirectory.unwrap(storeDirectory); + if (dfasd != null) { + Map<String, String> fileToChecksum = new HashMap<>(); + for (String file : toDownloadSegments) { + UploadedSegmentMetadata meta = uploadedSegments.get(file); + if (meta != null) { + fileToChecksum.put(file, meta.getChecksum()); + } else { + logger.warn("downloaded file [{}] has no metadata entry; checksum not registered", file); } - dfasd.registerDownloadedChecksums(fileToChecksum); } - } catch (Exception e) { - throw new IOException("Error occurred when downloading segments from remote store", e); + dfasd.registerDownloadedChecksums(fileToChecksum); } + } catch (Exception e) { + throw new IOException("Error occurred when downloading segments from remote store", e); } - ... } Suggestion importance[1-10]: 4 __ Why: The suggestion identifies a silent skip when uploadedSegments.get(file) returns null. While logging a warning would improve observability, the current behavior is safe (checksums are simply not registered for missing metadata, and the fallback scan will compute them). The impact is low since the code already handles the missing-metadata case gracefully.	Low
	Avoid returning null from commit Returning null from the commit() method in a test stub can cause NullPointerException in code that expects a valid CommitResult object. Consider returning a mock or minimal valid CommitResult instance instead of null to prevent potential test failures. server/src/test/java/org/opensearch/index/engine/exec/coord/SafeBootstrapCommitterTests.java [60-62] @Override public CommitResult commit(CommitInput commitData) { - return null; + return mock(CommitResult.class); } Suggestion importance[1-10]: 4 __ Why: While returning null from a test stub could potentially cause NullPointerException, this is a test-only stub implementation. The suggestion to use mock(CommitResult.class) is reasonable but may not be necessary if the test doesn't actually invoke this method or expects null behavior.	Low

---

Previous suggestions

Suggestions up to commit f614045

Category	Suggestion	Impact
Possible issue	Fix log level guard mismatch The code checks logger.isInfoEnabled() but then calls logger.debug(). This mismatch means the debug statement will never execute when only INFO level is enabled. Either change the guard to isDebugEnabled() or change the log call to logger.info(). server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryTargetService.java [339-363] -if (logger.isInfoEnabled()) { +if (logger.isDebugEnabled()) { int total = metadataSnapshot.size(); java.util.List<String> dfaFiles = metadataSnapshot.asMap() .keySet() .stream() .filter(name -> name.contains("/")) .sorted() .collect(java.util.stream.Collectors.toList()); String luceneSample = metadataSnapshot.asMap() .keySet() .stream() .filter(name -> name.contains("/") == false) .sorted() .limit(5) .collect(java.util.stream.Collectors.joining(", ")); logger.debug( "[DFA-RECOVERY] peer recovery TARGET sending metadata to source shardId={} totalFiles={} " + "dfaFiles={} dfaList=[{}] luceneSample=[{}]", recoveryTarget.shardId(), total, dfaFiles.size(), String.join(", ", dfaFiles), luceneSample ); } Suggestion importance[1-10]: 9 __ Why: The code checks logger.isInfoEnabled() but calls logger.debug(), creating a critical mismatch. The debug statement will never execute when only INFO level is enabled, defeating the purpose of the guard and potentially hiding important diagnostic information.	High
	Ensure translog sync before snapshot release The translog sync occurs outside the try-with-resources block, meaning if syncTranslog() throws an exception, the snapshot reference may not be properly released. Move the translog sync inside the try block before markSuccess() to ensure proper resource cleanup on failure. server/src/main/java/org/opensearch/index/engine/DataFormatAwareNRTReplicationEngine.java [293-322] private void commitCatalogSnapshot(boolean bumpSICounter) throws IOException { try (GatedConditionalCloseable<CatalogSnapshot> snapshotRef = catalogSnapshotManager.acquireSnapshotForCommit()) { CatalogSnapshot snapshot = snapshotRef.get(); ... CommitResult commitResult = committer.commit( new CommitInput(commitData.entrySet(), snapshot, bumpSICounter ? SI_COUNTER_INCREMENT : 0) ); if (commitResult != null) { catalogSnapshotManager.updateLastCommitInfo(commitResult); } + translogManager.syncTranslog(); snapshotRef.markSuccess(); } - translogManager.syncTranslog(); } Suggestion importance[1-10]: 8 __ Why: Moving translogManager.syncTranslog() inside the try-with-resources block before markSuccess() ensures proper resource cleanup if the sync throws an exception. This is a valid correctness improvement for exception safety.	Medium
	Prevent stale snapshot from breaking monotonicity The method unconditionally increments latestCatalogSnapshot.getId() and generation even when the incoming snapshot is older or identical to the current one. This violates the monotonic-increase invariant if a stale replication message arrives. Add a guard to verify incoming.getVersion() is strictly greater than latestCatalogSnapshot.getVersion() before applying. server/src/main/java/org/opensearch/index/engine/exec/coord/CatalogSnapshotManager.java [288-316] public synchronized void applyReplicationSnapshot(CatalogSnapshot incoming) throws IOException { if (closed.get()) { throw new IllegalStateException("CatalogSnapshotManager is closed"); } - ... - DataformatAwareCatalogSnapshot newSnapshot = new DataformatAwareCatalogSnapshot( - latestCatalogSnapshot.getId() + 1, - latestCatalogSnapshot.getGeneration() + 1, - incoming.getVersion(), - incoming.getSegments(), - latestCatalogSnapshot.getLastWriterGeneration() + 1, - incoming.getUserData(), - latestCatalogSnapshot.getLastCommitFileName(), - latestCatalogSnapshot.getLastCommitGeneration(), - latestCatalogSnapshot.getCommitDataFormatVersion() - ); + if (incoming.getVersion() <= latestCatalogSnapshot.getVersion()) { + logger.debug("Ignoring stale replication snapshot: incoming version {} <= current {}", + incoming.getVersion(), latestCatalogSnapshot.getVersion()); + return; + } ... } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential monotonicity violation if a stale replication snapshot arrives. The method unconditionally increments generation and id without checking if incoming.getVersion() is newer. Adding a guard to reject stale snapshots is a critical correctness fix for distributed replication scenarios.	Medium
	Restore sum for multi-format counts Using findFirst() returns only the first format's row count instead of summing across all formats. This will produce incorrect document counts when multiple data formats exist. Restore the sum() operation to aggregate rows from all formats. server/src/main/java/org/opensearch/index/engine/dataformat/merge/DataFormatAwareMergePolicy.java [202] private long calculateNumDocs(Segment segment) { - return segment.dfGroupedSearchableFiles().values().stream().mapToLong(WriterFileSet::numRows).findFirst().orElse(0L); + return segment.dfGroupedSearchableFiles().values().stream().mapToLong(WriterFileSet::numRows).sum(); } Suggestion importance[1-10]: 8 __ Why: Using findFirst() instead of sum() returns only the first format's row count, producing incorrect document counts when multiple data formats exist. This is a significant logic error that affects merge policy decisions in multi-format scenarios.	Medium
	Handle empty committed snapshots gracefully Remove the assertion or replace it with proper error handling. The comment "????????" indicates uncertainty, and the assertion may fail in valid scenarios where a fresh replica has no prior commits. Consider initializing an empty snapshot if the list is empty instead of asserting. server/src/main/java/org/opensearch/index/engine/DataFormatAwareNRTReplicationEngine.java [157-158] List<CatalogSnapshot> committed = committer.listCommittedSnapshots(); -assert false == committed.isEmpty() : "At least one commit should be present for replica"; // ???????? +if (committed.isEmpty()) { + committed = List.of(CatalogSnapshotManager.createInitialSnapshot(0L, 0L, 0L, List.of(), 0L, Map.of())); +} Suggestion importance[1-10]: 7 __ Why: The assertion with the "????????" comment indicates uncertainty and could fail in valid scenarios. The suggestion to initialize an empty snapshot when the list is empty is a reasonable defensive approach, though the assertion may be intentionally strict for replica initialization.	Medium
General	Avoid unchecked exception propagation The method throws a RuntimeException when cleanup fails, which can propagate unchecked and crash the calling thread. This is risky during background deletion or commit finalization. Consider wrapping the exception in a more specific type (e.g., EngineException) or logging and continuing to prevent shard failure. server/src/main/java/org/opensearch/index/engine/exec/coord/IndexFileDeleter.java [317-335] void decRefAndMaybeDelete(CatalogSnapshot snapshot) { if (snapshot.decRef()) { Exception firstException = null; try { onSnapshotDeletedCallback.accept(snapshot); } catch (Exception e) { firstException = e; } try { this.removeFileReferences(snapshot); } catch (IOException e) { if (firstException == null) firstException = e; else firstException.addSuppressed(e); } if (firstException != null) { - throw new RuntimeException("Failed to clean up snapshot [gen=" + snapshot.getGeneration() + "]", firstException); + logger.error("Failed to clean up snapshot [gen={}]", snapshot.getGeneration(), firstException); } } } Suggestion importance[1-10]: 7 __ Why: Throwing an unchecked RuntimeException during cleanup can crash background threads and fail the shard. Logging the error instead of throwing prevents cascading failures, improving resilience. This is a meaningful stability improvement.	Medium
	Propagate file deletion failures explicitly The method catches IOException but does not rethrow or propagate the failure map in a way that halts the operation. If critical files fail to delete, the replica may operate with stale data. Consider throwing an exception or returning a failure indicator that the caller can act upon. server/src/main/java/org/opensearch/index/engine/DataFormatAwareNRTReplicationEngine.java [943-964] private FileDeleter buildReplicaFileDeleter() { return filesByFormat -> { Map<String, Collection<String>> failed = new HashMap<>(); for (Map.Entry<String, Collection<String>> entry : filesByFormat.entrySet()) { final String formatName = entry.getKey(); for (String name : entry.getValue()) { if (committer.isCommitManagedFile(name)) { continue; } try { store.directory().deleteFile(FileMetadata.serialize(formatName, name)); } catch (NoSuchFileException ignored) { // already gone — treat as success } catch (IOException e) { logger.warn("Failed to delete file [{}] in format [{}]: {}", name, formatName, e.getMessage()); failed.computeIfAbsent(formatName, k -> new ArrayList<>()).add(name); } } } + if (!failed.isEmpty()) { + throw new IOException("Failed to delete files: " + failed); + } return failed; }; } Suggestion importance[1-10]: 6 __ Why: The suggestion to throw an exception when file deletion fails is reasonable for ensuring data consistency. However, the current implementation returns a failed map, which may be intentionally designed for partial failure handling. The improvement depends on the caller's error-handling strategy.	Low
	Remove unreachable assertion in closed path The assertion getIndexer().isReplicaIndexer() is evaluated inside a try-catch that swallows AlreadyClosedException. If the indexer is closed, getIndexer() throws before the assertion runs, making the assertion unreachable in the failure case. Move the assertion outside the try-catch or remove it, since the method already handles the closed state gracefully. server/src/main/java/org/opensearch/index/shard/IndexShard.java [1894-1904] public void finalizeReplication(CatalogSnapshot catalogSnapshot) throws IOException { try { - assert getIndexer().isReplicaIndexer() : "finalizeReplication called on non-replica indexer"; getIndexer().finalizeReplication(catalogSnapshot); } catch (AlreadyClosedException e) { logger.debug("finalizeReplication skipped, indexer already closed", e); return; } cleanupPendingMergedSegments(catalogSnapshot); evictStaleDownloadedChecksums(catalogSnapshot); } Suggestion importance[1-10]: 6 __ Why: The assertion getIndexer().isReplicaIndexer() is indeed unreachable if getIndexer() throws AlreadyClosedException before the assertion runs. The suggestion to remove the assertion is valid since the method already handles the closed state gracefully. However, the assertion serves as documentation of the method's contract, so moving it outside the try-catch (before the call) would be a better fix than removing it entirely.	Low
	Handle non-fatal delete errors The method catches NoSuchFileException to tolerate concurrent deletions, but other IOException subtypes (e.g., permission errors, network failures) will propagate and fail the entire delete operation. Consider catching IOException and logging a warning for non-fatal errors to ensure partial cleanup succeeds even when some containers are inaccessible. server/src/main/java/org/opensearch/index/store/remote/DataFormatAwareRemoteDirectory.java [320-337] public void delete() throws IOException { for (String format : formatBlobRouter.registeredFormats()) { try { formatBlobRouter.containerFor(format).delete(); } catch (java.nio.file.NoSuchFileException ignored) { - // already deleted — nothing to do + // already deleted + } catch (IOException e) { + logger.warn("Failed to delete container for format [{}]", format, e); } } try { super.delete(); } catch (java.nio.file.NoSuchFileException ignored) { // already deleted + } catch (IOException e) { + logger.warn("Failed to delete base container", e); } logger.debug("Deleted all containers from DataFormatAwareRemoteDirectory"); } Suggestion importance[1-10]: 6 __ Why: Catching only NoSuchFileException leaves the method vulnerable to other IOException subtypes (e.g., permission errors, network failures) that can abort the entire delete operation. Logging and continuing for non-fatal errors improves robustness during cleanup, though the current code already handles the most common case (concurrent deletion).	Low
	Distinguish I/O errors from closed state The method swallows IOException during checksum eviction, which could mask legitimate I/O failures that should be surfaced. While eviction is optimization, persistent I/O errors may indicate underlying storage issues. Consider logging at WARN level instead of DEBUG, or re-throwing IOException while only catching AlreadyClosedException. server/src/main/java/org/opensearch/index/shard/IndexShard.java [1912-1922] private void evictStaleDownloadedChecksums(CatalogSnapshot catalogSnapshot) { try { DataFormatAwareStoreDirectory dfasd = DataFormatAwareStoreDirectory.unwrap(store.directory()); if (dfasd == null) { return; // non-DFA index — no precomputed cache to evict from } dfasd.evictStaleChecksums(catalogSnapshot.getFiles(true)); - } catch (AlreadyClosedException | IOException e) { + } catch (AlreadyClosedException e) { logger.debug("skipped precomputed checksum eviction on replica", e); + } catch (IOException e) { + logger.warn("I/O error during checksum eviction on replica", e); } } Suggestion importance[1-10]: 5 __ Why: The suggestion to separate IOException and AlreadyClosedException handling is reasonable for better observability. However, the method's Javadoc explicitly states "Failures here are logged and swallowed — cache eviction is optimization, not correctness," so the current DEBUG-level catch-all is intentional. Upgrading to WARN for I/O errors is a minor improvement but not critical.	Low
	Log invalid checksum strings The method silently swallows NumberFormatException when parsing the checksum string. This can hide data corruption or protocol errors during recovery. Consider logging a warning when the checksum string is invalid so operators can detect and investigate malformed metadata. server/src/main/java/org/opensearch/index/store/DataFormatAwareStoreDirectory.java [343-359] public void registerDownloadedChecksum(String fileName, String checksumStr) { if (fileName == null || checksumStr == null) { return; } final long checksum; try { checksum = Long.parseLong(checksumStr); } catch (NumberFormatException e) { + logger.warn("Invalid checksum string for file [{}]: [{}]", fileName, checksumStr); return; } FileMetadata fm = toFileMetadata(fileName); FormatChecksumStrategy strategy = checksumStrategies.get(fm.dataFormat()); if (strategy != null) { - // Pass FileMetadata; the strategy owns key derivation. strategy.registerChecksum(fm, checksum, 0L); } } Suggestion importance[1-10]: 5 __ Why: Logging invalid checksum strings during recovery helps operators detect malformed metadata or protocol errors. However, this is a defensive improvement rather than a critical bug fix, so the impact is moderate.	Low
	Clarify reference counting for reused reader When refreshed is null, the code increments currentReader's refCount without a corresponding decRef path for the old reader. This causes a reference leak: the previous currentReader instance is never released. Ensure the old reader is decRef'd before reassigning, or verify that the null-refresher path is intentionally reusing the same reader instance. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneReaderManager.java [96-117] public void afterRefresh(boolean didRefresh, CatalogSnapshot catalogSnapshot) throws IOException { if (didRefresh == false || readers.containsKey(catalogSnapshot.getId())) { return; } DirectoryReader refreshed = readerRefresher.apply(currentReader, LuceneReplicaCommitter.getSegmentInfos(catalogSnapshot)); if (refreshed != null) { currentReader = refreshed; } else { + // Same reader is reused; incRef to balance the decRef in onDeleted currentReader.incRef(); } assert readersAreSame(catalogSnapshot, currentReader); ... } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about reference counting when refreshed is null. However, the code comment "If same reader is used, assert that calalog snapshot is same" and the incRef() call indicate intentional reuse. The suggestion's "improved_code" is identical to the existing code, so it's more of a clarification request than a fix. The logic appears correct but could benefit from a clearer comment.	Low

Suggestions up to commit f614045

Category	Suggestion	Impact
General	Fix logger level check mismatch The code checks logger.isInfoEnabled() but then calls logger.debug(), which is a mismatch. This could cause the debug statement to be skipped even when debug logging is enabled. Either change the condition to isDebugEnabled() or change the log call to info(). server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryTargetService.java [339-363] -if (logger.isInfoEnabled()) { +if (logger.isDebugEnabled()) { int total = metadataSnapshot.size(); java.util.List<String> dfaFiles = metadataSnapshot.asMap() .keySet() .stream() .filter(name -> name.contains("/")) .sorted() .collect(java.util.stream.Collectors.toList()); String luceneSample = metadataSnapshot.asMap() .keySet() .stream() .filter(name -> name.contains("/") == false) .sorted() .limit(5) .collect(java.util.stream.Collectors.joining(", ")); logger.debug( "[DFA-RECOVERY] peer recovery TARGET sending metadata to source shardId={} totalFiles={} " + "dfaFiles={} dfaList=[{}] luceneSample=[{}]", recoveryTarget.shardId(), total, dfaFiles.size(), String.join(", ", dfaFiles), luceneSample ); } Suggestion importance[1-10]: 9 __ Why: The code checks logger.isInfoEnabled() but calls logger.debug(), which is a critical mismatch. This bug will cause debug logs to be skipped even when debug logging is enabled, breaking the intended logging behavior.	High
	Fallback on checksum parse failure The calculateUploadChecksum method returns a String that is parsed as a long, but if the string is not a valid number, the method logs a warning and returns false. This could silently skip files that should be downloaded. Consider treating parse failures as a critical error or falling back to CodecUtil.retrieveChecksum to ensure correctness. server/src/main/java/org/opensearch/index/shard/IndexShard.java [6036-6051] long localChecksum; if (indexSettings.isPluggableDataFormatEnabled()) { DataFormatAwareStoreDirectory dfasd = DataFormatAwareStoreDirectory.unwrap(localDirectory); if (dfasd != null) { try { localChecksum = Long.parseLong(dfasd.calculateUploadChecksum(file)); } catch (NumberFormatException e) { - logger.warn("Invalid checksum format for file [{}]: {}", file, e.getMessage()); - return false; + logger.warn("Invalid checksum format for file [{}], falling back to CodecUtil", file); + localChecksum = CodecUtil.retrieveChecksum(indexInput); } } else { localChecksum = CodecUtil.retrieveChecksum(indexInput); } } else { localChecksum = CodecUtil.retrieveChecksum(indexInput); } Suggestion importance[1-10]: 8 __ Why: The current code returns false on NumberFormatException, which silently skips the file and triggers a re-download. Falling back to CodecUtil.retrieveChecksum instead ensures correctness by computing the checksum from the actual file content, preventing unnecessary downloads and potential data integrity issues.	Medium
	Handle malformed checksum string gracefully The code calls Long.parseLong() on the result of calculateUploadChecksum() without handling potential NumberFormatException. If the checksum string is malformed, this will throw an uncaught exception instead of a CorruptIndexException, making error diagnosis harder. server/src/main/java/org/opensearch/index/store/Store.java [823-834] -public static void checkIntegrity(final StoreFileMetadata md, final Directory directory) throws IOException { - ... - final DataFormatAwareStoreDirectory dfasd = DataFormatAwareStoreDirectory.unwrap(directory); - final String checksum = dfasd != null +final DataFormatAwareStoreDirectory dfasd = DataFormatAwareStoreDirectory.unwrap(directory); +final String checksum; +try { + checksum = dfasd != null ? digestToString(Long.parseLong(dfasd.calculateUploadChecksum(md.name()))) : Store.digestToString(CodecUtil.checksumEntireFile(input)); - if (checksum.equals(md.checksum()) == false) { - throw new CorruptIndexException( - "inconsistent metadata: actual checksum=" + checksum + ", metadata checksum=" + md.checksum() + ", file=" + md.name(), - input - ); - } +} catch (NumberFormatException e) { + throw new CorruptIndexException("invalid checksum format for file: " + md.name(), input, e); +} +if (checksum.equals(md.checksum()) == false) { + throw new CorruptIndexException( + "inconsistent metadata: actual checksum=" + checksum + ", metadata checksum=" + md.checksum() + ", file=" + md.name(), + input + ); } Suggestion importance[1-10]: 7 __ Why: Wrapping Long.parseLong() in a try-catch to convert NumberFormatException to CorruptIndexException improves error handling consistency. However, the impact is moderate since malformed checksums should be rare in practice.	Medium
	Validate type before unsafe cast The cast (DataformatAwareCatalogSnapshot) incoming is unsafe if incoming is not of that type. While the current code path may guarantee this, future refactorings or subclass introductions could cause a ClassCastException. Add a type check before casting to fail fast with a clear error message. server/src/main/java/org/opensearch/index/engine/exec/coord/CatalogSnapshotManager.java [288-316] public synchronized void applyReplicationSnapshot(CatalogSnapshot incoming) throws IOException { if (closed.get()) { throw new IllegalStateException("CatalogSnapshotManager is closed"); + } + if (!(incoming instanceof DataformatAwareCatalogSnapshot)) { + throw new IllegalArgumentException("incoming snapshot must be DataformatAwareCatalogSnapshot, got: " + incoming.getClass()); } ... DataformatAwareCatalogSnapshot newSnapshot = new DataformatAwareCatalogSnapshot( latestCatalogSnapshot.getId() + 1, latestCatalogSnapshot.getGeneration() + 1, incoming.getVersion(), incoming.getSegments(), latestCatalogSnapshot.getLastWriterGeneration() + 1, incoming.getUserData(), latestCatalogSnapshot.getLastCommitFileName(), latestCatalogSnapshot.getLastCommitGeneration(), latestCatalogSnapshot.getCommitDataFormatVersion() ); newSnapshot.setReplicatingCommitData(((DataformatAwareCatalogSnapshot) incoming).getReplicatingCommitData()); installSnapshot(newSnapshot); } Suggestion importance[1-10]: 7 __ Why: The unsafe cast (DataformatAwareCatalogSnapshot) incoming at line 313 could cause a ClassCastException if the type contract is violated. Adding an explicit type check with a clear error message improves robustness and fail-fast behavior, which is important for replication correctness.	Medium
	Fix non-deterministic document count calculation Using findFirst() on a stream of values from a map is non-deterministic and may return different results depending on iteration order. If all formats should have the same numRows, verify this assumption. Otherwise, use sum() or explicitly select a specific format. server/src/main/java/org/opensearch/index/engine/dataformat/merge/DataFormatAwareMergePolicy.java [202] private long calculateNumDocs(Segment segment) { - return segment.dfGroupedSearchableFiles().values().stream().mapToLong(WriterFileSet::numRows).findFirst().orElse(0L); + return segment.dfGroupedSearchableFiles().values().stream().mapToLong(WriterFileSet::numRows).sum(); } Suggestion importance[1-10]: 7 __ Why: The change from sum() to findFirst() makes the document count calculation non-deterministic since map iteration order is not guaranteed. However, the suggestion to revert to sum() may not be correct if all formats should have identical numRows values, requiring verification of the intended behavior.	Medium
	Clarify reader reference ownership The incRef() call on currentReader when refreshed == null is not balanced by a corresponding decRef() in the same method. If the reader is reused across multiple snapshots without proper reference counting, this could lead to a reader leak. Ensure that every incRef() has a matching decRef() when the reader is no longer needed, or document why the reference is intentionally held. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneReaderManager.java [96-117] public void afterRefresh(boolean didRefresh, CatalogSnapshot catalogSnapshot) throws IOException { if (didRefresh == false || readers.containsKey(catalogSnapshot.getId())) { return; } DirectoryReader refreshed = readerRefresher.apply(currentReader, LuceneReplicaCommitter.getSegmentInfos(catalogSnapshot)); + DirectoryReader readerToRegister; if (refreshed != null) { + readerToRegister = refreshed; currentReader = refreshed; } else { currentReader.incRef(); + readerToRegister = currentReader; } - assert readersAreSame(catalogSnapshot, currentReader); + assert readersAreSame(catalogSnapshot, readerToRegister); ... + readers.put(catalogSnapshot.getId(), new LuceneReader(readerToRegister, generationToSegmentName)); } Suggestion importance[1-10]: 6 __ Why: The incRef() call at line 111 when refreshed == null is balanced by decRef() in onDeleted() (line 196), but the suggestion to clarify ownership by introducing readerToRegister improves code readability and makes the reference counting pattern more explicit, reducing the risk of future leaks.	Low
	Return non-empty serialized metadata bytes **The mock returns an empty byte array for catalogSnapshotToRemoteMetadataSerializer(), which violates the contract state...

[github-actions]

❌ Gradle check result for c7f0d9b: 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 098c570

[github-actions]

Persistent review updated to latest commit 2cfced8

[github-actions]

✅ Gradle check result for 2cfced8: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 57.59398% with 282 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.47%. Comparing base (8f2d058) to head (3925bac).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...rc/main/java/org/opensearch/index/store/Store.java	42.85%	60 Missing and 8 partials ⚠️
...in/java/org/opensearch/index/shard/IndexShard.java	52.72%	37 Missing and 15 partials ⚠️
.../main/java/org/opensearch/index/engine/Engine.java	15.38%	22 Missing ⚠️
...ndex/engine/exec/coord/CatalogSnapshotManager.java	76.31%	12 Missing and 6 partials ⚠️
...pensearch/index/engine/exec/MonoFileWriterSet.java	0.00%	17 Missing ⚠️
.../opensearch/index/engine/NRTReplicationEngine.java	22.22%	14 Missing ⚠️
...opensearch/index/engine/DataFormatAwareEngine.java	43.47%	11 Missing and 2 partials ⚠️
...ine/exec/coord/DataformatAwareCatalogSnapshot.java	74.00%	7 Missing and 6 partials ⚠️
...arch/index/engine/exec/coord/IndexFileDeleter.java	60.00%	7 Missing and 1 partial ⚠️
...g/opensearch/index/engine/EngineBackedIndexer.java	53.33%	5 Missing and 2 partials ⚠️
... and 19 more

Additional details and impacted files

@@            Coverage Diff             @@
##               main   #21311    +/-   ##
==========================================
  Coverage     73.46%   73.47%            
- Complexity    74825    74934   +109     
==========================================
  Files          5997     5999     +2     
  Lines        339688   340177   +489     
  Branches      48961    49006    +45     
==========================================
+ Hits         249558   249930   +372     
- Misses        70272    70285    +13     
- Partials      19858    19962   +104     

☔ 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 8309618

[github-actions]

❌ Gradle check result for 8309618: 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 b09225c

[github-actions]

❌ Gradle check result for b09225c: 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 b09225c

[github-actions]

✅ Gradle check result for b09225c: SUCCESS

[github-actions]

Persistent review updated to latest commit 79a5517

[github-actions]

Persistent review updated to latest commit be555e0

[github-actions]

❌ Gradle check result for be555e0: null

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 12a3eba

[github-actions]

✅ Gradle check result for 12a3eba: SUCCESS

[github-actions]

Persistent review updated to latest commit 4a0b61b

[github-actions]

Persistent review updated to latest commit 3925bac

[github-actions]

✅ Gradle check result for 3925bac: SUCCESS