HDDS-12501. Remove OMKeyInfo from SST files in backup directory. by SaketaChalamchala · Pull Request #8214 · apache/ozone

SaketaChalamchala · 2025-04-02T01:26:31Z

What changes were proposed in this pull request?

Currently hard links to the SST files are stored in the sst backup directories. The OMKeyInfo in the SST files are not used anywhere.

Proposed optimization would

Read the SST files and only store the keys including the tombstones into a temporary SST file.
Acquires a mutex and moves the temporary SST file to the original file, replacing it atomically.

This will significantly reduce the storage footprint and also improve bootstrap time.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-12501

How was this patch tested?

In progress.

SaketaChalamchala · 2025-04-02T01:26:59Z

@swamirishi and @hemantk-12 could you take a look?

swamirishi

@SaketaChalamchala thank you for the patch I have added comments on the implementation please look at the comments inline.

swamirishi · 2025-04-02T01:58:39Z

+ sstFileWriter.finish();
+
+ // Acquire a mutex
+ try (BootstrapStateHandler.Lock lock = getBootstrapStateLock().lock()) {


You need to take an ozone manager lock. Create an SST_FILE_LOCK resource in OzoneManagerLock just on the sst file not entire lock
ozone/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java
Line 460 in c4e78d9
SNAPSHOT_LOCK((byte) 7, "SNAPSHOT_LOCK"), // = 255

Using BootstrapLock is wrong here

We should take a lock while also computing the hardlinks during snapshot diff computation.
ozone/hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java
Lines 839 to 844 in b11b807
StringsstFullPath = getSSTFullPath(sst, src.getDbPath());
Pathlink = Paths.get(sstFilesDirForSnapDiffJob,
sst + SST_FILE_EXTENSION);
PathsrcFile = Paths.get(sstFullPath);
createLink(link, srcFile);
returnlink.toString();

swamirishi · 2025-04-02T02:03:19Z

+ managedOptions, sstFile.getAbsolutePath(), 2 * 1024 * 1024);
+ ManagedRawSSTFileIterator<String> itr = sstFileReader.newIterator(
+ keyValue -> StringUtils.bytes2String(keyValue.getKey()) + kVSeparator
+ + StringUtils.bytes2String(keyValue.getValue()), null, null);


We don't need the value.

We can instead look for the type of the entry

hemantk-12

Thanks @SaketaChalamchala for the optimization.

hemantk-12 · 2025-04-03T05:20:56Z

+ ManagedEnvOptions envOptions = new ManagedEnvOptions();
+ ManagedOptions managedOptions = new ManagedOptions();


These can be outside the for loop and reused.

hemantk-12 · 2025-04-03T05:26:59Z

+ ManagedSstFileWriter sstFileWriter = new ManagedSstFileWriter(envOptions, managedOptions))
+ sstFileWriter.open(prunedSSTFile.getAbsolutePath());
+
+ ManagedRawSSTFileReader sstFileReader = new ManagedRawSSTFileReader<>(


Can we reuse RDBSstFileWriter?

Yes we should use this and add the delete functionality there

hemantk-12 · 2025-04-03T05:42:47Z

+ Path sstBackupDirPath = Paths.get(sstBackupDir);
+ String kVSeparator = ",";
+
+ try (Stream<Path> files = Files.list(sstBackupDirPath)


How are we differentiating if a file has been pruned or not?

Discussed with @SaketaChalamchala we intend to add a boolean flag in CompactionFileInfo

We can improve it further by maintaining a BlockingQueue, adding not-cleaned files to the queue after service startup, and after the compilation, so that it doesn't have to go over the whole compaction DAG in each run to filter cleaned files.
cc: @swamirishi

peterxcli · 2025-04-08T04:20:12Z

+ // Take a write lock on the SST File
+ getSSTFileLock(sstFile.getAbsolutePath()).writeLock().lock();
+ try {
+ // Move file.sst.tmp file.sst and replace existing file atomically


Suggested change
// Move file.sst.tmp file.sst and replace existing file atomically
// Move file.sst.tmp to file.sst and replace existing file atomically

peterxcli · 2025-04-08T04:26:43Z

+ // Move file.sst.tmp file.sst and replace existing file atomically
+ Files.move(prunedSSTFile.toPath(), file, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING);
+ } catch (Exception e) {
+ throw new RuntimeException(e);


Suggested change
thrownewRuntimeException(e);
Files.deleteIfExists(prunedSSTFile.toPath());
thrownewRuntimeException(e);

swamirishi

Let us not add a new implementation for locks

swamirishi · 2025-04-08T05:34:12Z

 private final long maxAllowedTimeInDag;
 private final BootstrapStateHandler.Lock lock
 = new BootstrapStateHandler.Lock();
+ private final Striped<ReadWriteLock> stripedSSTLock;


Use IOzoneManagerLock directly. Add a resource SST_FILE_LOCK
ozone/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java
Line 571 in 0f52a34
publicenumResource {

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java
Lines 924 to 927 in 6cce2bb
@Override
publicIOzoneManagerLockgetLock() {
returnlock;
}

Ozone Manager jar has a dependency rocksdb-checkpoint-differ jar to be compiled first. So, I can't reference org.apache.hadoop.ozone.om.lock.OzoneManagerLock packaged in the ozone manager jar in org.apache.ozone.rocksdiff.RocksDBCheckpointDiffer

swamirishi · 2025-04-08T05:34:58Z

 TimeUnit.MILLISECONDS);
 this.suspended = new AtomicBoolean(false);

+ this.stripedSSTLock = SimpleStriped.readWriteLock(


Let us not do add a new configuration for locks, reuse OzoneManagerLock

See reply above.

swamirishi · 2025-04-08T05:36:52Z

- createLink(link, srcFile);
- return link.toString();
+ // Take a read lock on the SST FILE
+ getSSTFileLock(link.toFile().getAbsolutePath()).readLock().lock();


Lock needs to be taken on the sst file name or InodeId of the file

…n complete. Using SST lock from OM.

hemantk-12 · 2025-04-22T05:00:20Z

+ TimeUnit.MILLISECONDS);
+
+ try {
+ isNativeLibsLoaded = ManagedRawSSTFileReader.loadLibrary();


Add a check if ozone.om.snapshot.load.native.lib is enabled.
Can we load ManagedRawSSTFileReader.loadLibrary() twice? Because it is initialized in SnapshotDiffManager#initNativeLibraryForEfficientDiff.

Do you mean if we can move the native library initialization to RocksDBCheckpointDiffer and just check if native library is loaded in SnapshotDiffManager? That might be better.

I missed adding twice (updated previous comment).
native library initialization to RocksDBCheckpointDiffer and just check if native library is loaded in SnapshotDiffManager?
Nah, I meant "Initialize ManagedRawSSTFileReader only when it is not already initialized, like lazy load".
Not related to this patch, why do we have a T typed ManagedRawSSTFileReader? It should be independent of type IMO.

hemantk-12 · 2025-04-22T05:03:49Z

 private ConcurrentMap<String, CompactionFileInfo> inflightCompactions;
+ private boolean isNativeLibsLoaded;
+ private BlockingQueue<Pair<byte[], CompactionLogEntry>> pruneQueue =
+ new LinkedBlockingQueue<Pair<byte[], CompactionLogEntry>>();


We should limit the BlockingQueue to a few thousand.

We shouldn't do that. To begin with on init there could be a lot of entries in the table which might not have been pruned

Fair enough.

hemantk-12 · 2025-04-22T05:09:35Z

- createLink(link, srcFile);
- return link.toString();
+ // Take a read lock on the SST FILE
+ ReentrantReadWriteLock.ReadLock sstReadLock = getSSTFileLock(sstFullPath).readLock();


IMO, we don't need file-level granularity lock. We can define a simple lock in RocksDBCheckpointDiffer and use that.

@SaketaChalamchala Do we even need a lock? Given that we are relying on the filesystem doing an Atomic Move I believe we don't even need a lock

hemantk-12 · 2025-04-22T05:57:05Z

+ ManagedSstFileWriter sstFileWriter = new ManagedSstFileWriter(envOptions, managedOptions)) {
+
+ Pair<byte[], CompactionLogEntry> compactionLogTableEntry;
+ while ((compactionLogTableEntry = pruneQueue.poll()) != null) {


We should loop this to max to the size of the queue that is used to initialize the queue.

hemantk-12 · 2025-04-22T06:07:04Z

+ CompactionLogEntry compactionLogEntry = builder.build();
+ byte[] key = addToCompactionLogTable(compactionLogEntry);
+ // Add the compaction log entry to the prune queue so that the backup input sst files can be pruned.
+ if (isNativeLibsLoaded) {
+ try {
+ pruneQueue.put(Pair.of(key, compactionLogEntry));
+ } catch (InterruptedException e) {
+ LOG.error("Could not add backup SST files in queue to be pruned. {}",
+ compactionLogEntry.getInputFileInfoList(), e);
+ }
+ }


I'll suggest moving this code here. Otherwise, we will miss new entries from the table.

Agree. Just out of curiosity, is there any thread other than during OM startup and onCompactionComplete listener thread that can update the compactionLogTable?

No. onCompactionComplete mainly adds entries to compactionLogTable and OM startup is for backward compatibility.

hemantk-12 · 2025-04-22T06:20:49Z

+ }
+
+ // Write the file.sst => file.sst.tmp
+ File prunedSSTFile = Files.createFile(sstBackupDirPath.resolve(sstFile.getName() + ".tmp")).toFile();


Files.createFile will throw FileAlreadyExistsException if the file already exists, which is possible if there was a failure in the previous run. Handle the file exists scenario.

hemantk-12 · 2025-04-22T06:24:28Z

+ }
+
+ // Put pruned flag in compaction DAG. Update CompactionNode in Map.
+ compactionNodeMap.get(sstFileName).setPruned();


Why do we need to update the compactionNode? A file will be pruned only once during the life cycle of the process, either added by the compaction listener or the OM start-up process.

Yeah.. the thought was to verify if the file was pruned against the compactioNodeMap in heap if the thread was stopped. But it doesn't look like its necessary.

hemantk-12 · 2025-04-22T06:28:57Z

+ }
+ sstFileWriter.finish();
+
+ // Take a write lock on the SST File


nit: no need to add this comment or the next one.

hemantk-12 · 2025-04-22T22:00:35Z

+ // Take a write lock on the SST File
+ sstWriteLock.lock();
+ // Move file.sst.tmp to file.sst and replace existing file atomically
+ Files.move(prunedSSTFile.toPath(), sstFile.toPath(),


Don't we need to acquire the BootstrapStateHandler lock? What if the tarball request is streaming the file when the move happens?

yeah we would need a bootstrap handler lock here

I removed the file level locking and an just taking the Bootstrap lock now.

swamirishi

@SaketaChalamchala thanks for addressing the previous review comments. I have left a few more review comments. Please take a look at them. I believe you can remove the lock change as we are relying on the atomic rename.

swamirishi · 2025-04-30T18:02:09Z

- if (!NativeLibraryLoader.getInstance().loadLibrary(ROCKS_TOOLS_NATIVE_LIBRARY_NAME, Arrays.asList(
- ManagedRocksObjectUtils.getRocksDBLibFileName()))) {
- throw new NativeLibraryNotLoadedException(ROCKS_TOOLS_NATIVE_LIBRARY_NAME);
+ if (!isLibraryLoaded) {


@SaketaChalamchala I see this is an optimization. Can we do it as a separate patch? This doesn't seem to be related to the change. Lets limit this patch to just pruning key info from the sst files.

BTW look at NativeLibraryLoader class we are already doing this inside. We don't need this change here.

swamirishi · 2025-04-30T18:20:50Z

 .setCreateCheckpointDirs(true)
 .setEnableRocksDbMetrics(true)
 .setMaxNumberOfOpenFiles(maxOpenFiles)
+ .setLock(lock)


Why add lock here? Can't we directly pass lock here?
ozone/hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java
Lines 1419 to 1425 in dc9952e
publicstaticRocksDBCheckpointDiffergetInstance(
StringmetadataDirName,
StringsstBackupDirName,
StringcompactionLogDirName,
StringactiveDBLocationName,
ConfigurationSourceconfiguration
) {

Pass StripedLock <ReadWriteLock> directly here.

swamirishi · 2025-04-30T18:29:25Z

- createLink(link, srcFile);
- return link.toString();
+ // Take a read lock on the SST FILE
+ ReentrantReadWriteLock.ReadLock sstReadLock = getSSTFileLock(sstFullPath).readLock();


@SaketaChalamchala Do we even need a lock? Given that we are relying on the filesystem doing an Atomic Move I believe we don't even need a lock

swamirishi · 2025-04-30T19:00:23Z

 this.startKey = startKey;
 this.endKey = endKey;
 this.columnFamily = columnFamily;
+ this.pruned = pruned;


We dont need this change from what I understand.

swamirishi · 2025-04-30T19:53:22Z

 private ManagedRocksDB activeRocksDB;
 private ConcurrentMap<String, CompactionFileInfo> inflightCompactions;
+ private boolean isNativeLibsLoaded = false;
+ private BlockingQueue<Pair<byte[], CompactionLogEntry>> pruneQueue =


Why BlockingQueue? Why not ConcurrentLinkedQueue?

yes, we could use ConcurrentLinkedQueue since it's thread-safe and unbounded.

SaketaChalamchala · 2025-05-01T21:14:13Z

Thanks @hemantk-12 @swamirishi and @peterxcli for the review. Please review the updated patch.

swamirishi

@SaketaChalamchala thanks for addressing the previous review comments. Overall the changes are good barring a few implementation level changes.

swamirishi · 2025-05-02T11:32:58Z

+ try {
+ sstFileWriter.finish();
+ } catch (RocksDBException e) {
+ throw new RuntimeException("Failed to finish writing to " + prunedSSTFilePath, e);


Why runtime exception? Convert RocksdbException into RocksDatabaseException.

swamirishi · 2025-05-02T11:46:33Z

+ continue;
+ } finally {
+ try {
+ Files.deleteIfExists(prunedSSTFilePath);


How about having the same path for all the tmp files and just deleting it at the start of the sstFileWriter instead of having this rollback finally block in multiple places?
Something like "pruned.sst.tmp"

Let us just break the entire thread run if an exception is thrown. This exception handling logic is convoluting the piece of code which IMO is unnecessary.

Thanks for the review @swamirishi.
Do you see any reason why pruning files in one compactionEntry would fail every time because that would cause the queue to be backed up forever. Otherwise, these changes make sense.

swamirishi · 2025-05-02T11:47:46Z

+ byte[] key = compactionLogTableEntry.getKey();
+ CompactionLogEntry compactionLogEntry = compactionLogTableEntry.getValue();
+ boolean shouldUpdateTable = false;
+ boolean shouldReprocess = false;


How about just peeking and not polling at the start? We wouldn't need this reprocess boolean value.

swamirishi · 2025-05-02T11:51:14Z

+ // Add the compaction log entry to the prune queue
+ // so that the backup input sst files can be pruned.
+ if (isNativeLibsLoaded) {
+ pruneQueue.offer(Pair.of(key, compactionLogEntry));


Instead of a pair just add the key to the queue. We need not keep the compactionLogEntry in memory

swamirishi · 2025-05-02T11:53:08Z

 private ManagedRocksDB activeRocksDB;
 private ConcurrentMap<String, CompactionFileInfo> inflightCompactions;
+ private boolean isNativeLibsLoaded = false;
+ private ConcurrentLinkedQueue<Pair<byte[], CompactionLogEntry>> pruneQueue =


Keep this null or Optional.empty() by default and only initialize if the prune thread is going to run.

swamirishi · 2025-05-02T11:53:57Z

 }
+ // Add the compaction log entry to the prune queue
+ // so that the backup input sst files can be pruned.
+ if (isNativeLibsLoaded) {


I don't like this logic present in multiple places. We should just check pruneQueue != null

swamirishi · 2025-05-02T11:56:39Z

 package org.apache.hadoop.ozone.om.snapshot;

 import static org.apache.hadoop.hdds.utils.db.DBStoreBuilder.DEFAULT_COLUMN_FAMILY_NAME;
+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_OM_SNAPSHOT_LOAD_NATIVE_LIB;


Why this change?

swamirishi · 2025-05-02T11:59:31Z

+ /**
+ * Test that backup SST files are pruned on loading previous compaction logs.
+ */
+ @EnabledIfSystemProperty(named = ROCKS_TOOLS_NATIVE_PROPERTY, matches = "true")


We need not check if rocks tools is loaded. We can just mock the raw sst file iterator construction. This test should be more of a unit test case.

swamirishi

@SaketaChalamchala Found some more minor issues in the implementation. I am looking at the test cases you can look into fixing this in the meanwhile.

swamirishi · 2025-05-08T17:52:54Z

+ try {
+ if (configuration.getBoolean(OZONE_OM_SNAPSHOT_LOAD_NATIVE_LIB, OZONE_OM_SNAPSHOT_LOAD_NATIVE_LIB_DEFAULT)
+ && ManagedRawSSTFileReader.loadLibrary()) {
+ pruneQueue = new LinkedList<>();


This is not thread safe. Use either synchronized queue or ConcurrentLinkedQueue

swamirishi · 2025-05-08T18:04:24Z

+ }
+
+ // Update Compaction Log table. Track keys that need updating.
+ if (shouldUpdateTable) {


Wouldn't this be always true since we are throwing all exceptions if this line is reached? Seems redundant

This is not always true. Say, if all source files in a compactionLogEntry have been removed by pruneSstFiles thread then, we skip pruning those files without throwing an error. If we do a bootstrap after pruning some files then, there might be some entries that have all source files pruned. In both cases we skip updating the compactionLogTable. We only update the table if atleast one of the source files in the log has been pruned.

Wouldn't this be all or none? Aren't we pruning all files in the compactionLogEntry and adding to the table?

We are processing all the files in a compaction log entry, one file at a time. Sometimes, the compaction entry might exist but pruneSstFiles might delete the sst files if they are non-leaf nodes. In which case, we will skip pruning the deleted file.
If all the files in the compaction log are deleted, then we don't have to update the compaction entry, If some files were deleted and some files are still present on the disk we prune what files are available, if all files are present on the disk we prune everything.
In the latter 2 cases, we update the compaction log table. shouldUpdateTable indicates if at least of the source files has been pruned.

swamirishi · 2025-05-08T18:06:20Z

+
+ // Update Compaction Log table. Track keys that need updating.
+ if (shouldUpdateTable) {
+ CompactionLogEntry.Builder builder = new CompactionLogEntry.Builder(compactionLogEntry.getDbSequenceNumber(),


nit: Would prefer a toBuilder() method in CompactionLogEntry class.

InputFileInfo list in CompactionLogEntry is final which makes sense. Do we want to make the inputfiles not final and and update the input file list later?

swamirishi · 2025-05-08T18:08:14Z

+ if (compactionReason != null) {
+ builder.setCompactionReason(compactionReason);
+ }
+ synchronized (this) {


Is this synchronized because of DAG pruner? If that is the case this entire operation is not thread safe. It could so be the case that the dag pruner might have deleted the entry.

Yes, DAG pruner is removing the older compactionLogEntries (30d by default). The SST files would have been pruned by then already. I could have everything under the while loop in a synchronized block just to be safe.

yeah we should synchornize based on the entry. It is ok to put in one synchronized block as well.

Lmk once this is done we can look into merging this after that

swamirishi · 2025-05-08T18:09:57Z

+ }
+ pruneQueue.poll();
+ }
+ Files.deleteIfExists(prunedSSTFilePath);


Why delete it should not exist here. Do this in the beginning before writing the sst file.

I was thinking that we could remove the tmp file after if we have the chance (if the pruner runs successfully). It would be cleaner not to leave the tmp file until the next time pruner runs.

It is a tmp file it shouldn't be of much consequence it is ok to let dangle around. Anyways we are not guaranteed this will run if the jvm crashes.

swamirishi · 2025-05-08T18:28:31Z

+
+ private void createSSTFileWithKeys(String filePath, List<Pair<byte[], Integer>> keys)
+ throws Exception {
+ try (ManagedSstFileWriter sstFileWriter = new ManagedSstFileWriter(new ManagedEnvOptions(), new ManagedOptions())) {


Can we mock sst file writer to write a text file instead and also mock rawSstFileReader to read the text file

@SaketaChalamchala Please create a follow up jira to change this test case as per the above commit.

@swamirishi Sure I can do this. But I wanted to understand, if we just want to mock what the rawSSTFileReader is reading would it matter if it was from a txt file or from a list?

having it in an in memory map is also fine. I would say writing to a txt file might be a bit better from building an understanding standpoint. you can actually create a static file to read from by default.

swamirishi · 2025-05-08T21:54:02Z

+ if (compactionReason != null) {
+ builder.setCompactionReason(compactionReason);
+ }
+ synchronized (this) {


yeah we should synchornize based on the entry. It is ok to put in one synchronized block as well.

swamirishi · 2025-05-08T21:55:36Z

+ }
+
+ // Update Compaction Log table. Track keys that need updating.
+ if (shouldUpdateTable) {


Wouldn't this be all or none? Aren't we pruning all files in the compactionLogEntry and adding to the table?

swamirishi · 2025-05-09T15:15:29Z

+ if (shouldUpdateTable) {
+ CompactionLogEntry.Builder builder = new CompactionLogEntry.Builder(
+ compactionLogEntry.getDbSequenceNumber(), compactionLogEntry.getCompactionTime(),
+ updatedFileInfoList, compactionLogEntry.getOutputFileInfoList());


I was saying if we can create a toBuilder() function in CompactionLogEntry class which would just copy this object and then we can update inputFileInfoList in the builder with updatedFileInfoList

Maybe we can take this up in follow up patch

I was only wondering above if we need to allow updating input file info list. Otherwise, this is a small change. I can push this in with the final updates.
https://github.com/apache/ozone/pull/8214/files#discussion_r2080758187

SaketaChalamchala · 2025-05-09T18:34:28Z

@swamirishi Addressed all the changes. The CI is green on my fork.

swamirishi · 2025-05-12T17:15:11Z

+
+ // Write the file.sst => pruned.sst.tmp
+ Files.deleteIfExists(prunedSSTFilePath);
+ try (ManagedRawSSTFileReader<Pair<byte[], Integer>> sstFileReader = new ManagedRawSSTFileReader<>(


nit: Can we move the pruning part into a separate function. The function gotten too long

swamirishi · 2025-05-12T17:17:00Z

+ if (keyValue.getValue() == 0) {
+ sstFileWriter.delete(keyValue.getKey());
+ } else {
+ sstFileWriter.put(keyValue.getKey(), new byte[0]);


why create a new empty byte array object for every value? This can have gc impact.

You're right. I can use org.apache.commons.lang3.ArrayUtils.EMPTY_BYTE_ARRAY here.

swamirishi · 2025-05-12T17:20:50Z

+ Path prunedSSTFilePath = sstBackupDirPath.resolve(PRUNED_SST_FILE_TEMP);
+ try (ManagedOptions managedOptions = new ManagedOptions();
+ ManagedEnvOptions envOptions = new ManagedEnvOptions();
+ ManagedSstFileWriter sstFileWriter = new ManagedSstFileWriter(envOptions, managedOptions)) {


Why are we using the same sst file writer for all instances? We should have a test case for this. I would prefer to have this inside the for loop

swamirishi

@SaketaChalamchala I have a few doubts please look at the comments in line

…Entry.toBuilder().

swamirishi

@SaketaChalamchala LGTM

swamirishi · 2025-05-12T20:06:47Z

+
+ private void createSSTFileWithKeys(String filePath, List<Pair<byte[], Integer>> keys)
+ throws Exception {
+ try (ManagedSstFileWriter sstFileWriter = new ManagedSstFileWriter(new ManagedEnvOptions(), new ManagedOptions())) {


having it in an in memory map is also fine. I would say writing to a txt file might be a bit better from building an understanding standpoint. you can actually create a static file to read from by default.

swamirishi · 2025-05-12T20:08:54Z

+ private void removeValueFromSSTFile(ManagedOptions options, ManagedEnvOptions envOptions,
+ String sstFilePath, String prunedFilePath)
+ throws IOException {
+ ManagedSstFileWriter sstFileWriter = new ManagedSstFileWriter(envOptions, options);


why not make this part of the try with resource block?

Co-authored-by: Swaminathan Balachandran <47532440+swamirishi@users.noreply.github.com>

swamirishi · 2025-05-13T20:59:08Z

Thanks for working on the patch @SaketaChalamchala and thanks @peterxcli & @hemantk-12 for reviewing the patch

…239-container-reconciliation Commits: 113 commits 8ceb5c3 Revert "HDDS-11232. Spare InfoBucket RPC call for the FileSystem#getFileStatus calls for the general case. (apache#6988)" (apache#8358) 8ecf4a8 HDDS-12993. Use DatanodeID in ReconNodeManager. (apache#8456) 3e09a11 HDDS-13024. HTTP connect to 0.0.0.0 failed (apache#8439) c3cb14f HDDS-12914. Bump awssdk to 2.31.40, test ResumableFileDownload (apache#8455) c6b47e9 HDDS-4677. Document Ozone Ports and Connection End Points (apache#8226) ae0d757 HDDS-13044. Remove DatanodeDetails#getUuid usages from hdds-common/client/container-service (apache#8462) 2bf3f16 HDDS-12568. Implement MiniOzoneCluster.Service for Recon (apache#8452) e59d251 HDDS-13046. Add .vscode to .gitignore (apache#8461) 50e6d61 HDDS-13022. Split up exclusive size tracking for key and directory cleanup in SnapshotInfo (apache#8433) 6db647d HDDS-12966. DBDefinitionFactory should not throw InvalidArnException (apache#8454) fe2875c HDDS-12948. [Snapshot] Increase `ozone.om.fs.snapshot.max.limit` default value to 10k (apache#8376) 215bdc2 HDDS-11904. Fix HealthyPipelineSafeModeRule logic. (apache#8386) dba7557 HDDS-12989. Throw CodecException for the Codec byte[] methods (apache#8444) d9c73f0 HDDS-11298. Support owner field in the S3 listBuckets request (apache#8315) 7a5129d HDDS-12810. Check and reserve space atomically in VolumeChoosingPolicy (apache#8360) d51ccf7 HDDS-13016. Add a getAllNodeCount() method to NodeManager. (apache#8445) 8b27fb9 HDDS-12299. Merge OzoneAclConfig into OmConfig (apache#8383) 96f4f79 HDDS-12501. Remove OMKeyInfo from SST files in backup directory. (apache#8214) 8adb500 HDDS-13021. Make callId unique for each request in AbstractKeyDeletingService (apache#8432) faeadd6 HDDS-13000. [Snapshot] listSnapshotDiffJobs throws NPE. (apache#8418) 7c957cc HDDS-11954. List keys command gets wrong info from BasicOmKeyInfo (apache#8401) 0697383 HDDS-13027. Workaround for GitHub connection failures (apache#8443) 0d5f933 HDDS-12919. Remove redundant FileLock from ChunkWrite (apache#8435) 395892a HDDS-13012. [Snapshot] Add audit logs for listSnapshotDiffJobs, snapshotDiff and cancelSnapshotDiff. (apache#8425) d92f0ed HDDS-12653. Add option in `ozone debug log container list` to filter by Health State (apache#8415) 78f8a02 HDDS-12645. Improve output of `ozone debug replicas chunk-info` (apache#8146) f32470c HDDS-12995. Split ListOptions for better reuse (apache#8413) cdd1b13 HDDS-13019. Bump jline to 3.30.0 (apache#8428) 1651408 HDDS-13018. Bump common-custom-user-data-maven-extension to 2.0.2 (apache#8429) 8f6222e HDDS-12949. Cleanup SCMSafeModeManager (apache#8390) adb385f HDDS-13002. Use DatanodeID in ContainerBalancerTask (apache#8421) 2b2a796 HDDS-12969. Use DatanodeID in XceiverClientGrpc (apache#8430) 3c0bafa HDDS-13001. Use DatanodeID in SCMBlockDeletingService. (apache#8420) 8448bc2 HDDS-12187. Use hadoop-dependency-client as parent, not dependency (apache#8416) 582fd03 HDDS-13015. Bump zstd-jni to 1.5.7-3 (apache#8426) ec12f98 HDDS-12970. Use DatanodeID in Pipeline (apache#8414) 3f48a18 HDDS-12179. Improve Container Log to also capture the Volume Information (apache#7858) 034b497 HDDS-12964. Add minimum required version of Docker Compose to RunningViaDocker page (apache#8385) c0a6b42 HDDS-12947. Add CodecException (apache#8411) 70949c5 HDDS-11603. Reclaimable Key Filter for Snapshots garbage reclaimation (apache#8392) 08283f3 HDDS-12087. TransactionToDNCommitMap too large causes GC to pause for a long time (apache#8347) f47df78 HDDS-12958. [Snapshot] Add ACL check regression tests for snapshot operations. (apache#8419) 1cc8445 HDDS-12207. Unify output of ozone debug replicas verify checks (apache#8248) f087d0b HDDS-12994. Use DatanodeID in ReconSCMDBDefinition. (apache#8417) d6a7723 HDDS-12776. ozone debug CLI command to list all Duplicate open containers (apache#8409) c78aeb0 HDDS-12951. EC: Log when falling back to reconstruction read (apache#8408) 412f22d HDDS-12959. Eliminate hdds-hadoop-dependency-server (apache#8384) df701dc HDDS-12996. Workaround for Docker Compose concurrent map writes (apache#8412) e6daae4 HDDS-12972. Use DatanodeID in ContainerReplica. (apache#8396) c1103ae HDDS-12877. Support StorageClass field in the S3 HeadObject request (apache#8351) 4135384 HDDS-12973. Add javadoc for CompactionNode() and make getCompactionNodeGraph return ConcurrentMap (apache#8395) 4f2e13c HDDS-12954. Do not throw IOException for checksum. (apache#8387) 49b8fbd HDDS-12971. Use DatanodeID in Node2PipelineMap (apache#8403) d2da18f HDDS-12346. Reduce code duplication among TestNSSummaryTask classes (apache#8287) 82b73e3 HDDS-11856. Set DN state machine thread priority higher than command handler thread. (apache#8253) d4f2734 HDDS-12689. Import BOM for AWS SDK, declare dependencies (apache#8406) 4775e76 HDDS-12975. Fix percentage of blocks deleted in grafana dashboard (apache#8398) ac0d696 HDDS-12968. [Recon] Fix column visibility issue in Derby during schema upgrade finalization. (apache#8393) e71dcf6 HDDS-11981. Add annotation for registering feature validator based on a generic version (apache#7603) 254297c HDDS-12562. Reclaimable Directory entry filter for reclaiming deleted directory entries (apache#8055) 4f467c8 HDDS-12978. Remove TestMultipartObjectGet (apache#8400) a99f207 HDDS-12967. Skip CommonChunkManagerTestCases.testFinishWrite if fuser cannot be started (apache#8389) d29d76b HDDS-12697. Ozone debug CLI to display details of a single container (apache#8264) 1d1bc88 HDDS-12974. Docker could not parse extra host IP (apache#8397) 7e675d7 HDDS-12053. Make print-log-dag command run locally and offline (apache#8016) d3faab3 HDDS-12561. Reclaimable Rename entry filter for reclaiming renaming entries (apache#8054) af1f98c HDDS-10822. Tool to omit raft log in OM. (apache#8154) 3201ca4 HDDS-12952. Make OmSnapshotManager#snapshotLimitCheck thread-safe and consistent (apache#8381) 522c88d HDDS-12963. Clean up io.grpc dependencies (apache#8382) fa8bd9d HDDS-12916. Support ETag in listObjects response (apache#8356) fdc77db HDDS-12300. Merge OmUpgradeConfig into OmConfig (apache#8378) 623e144 HDDS-12956. Bump vite to 4.5.14 (apache#8375) 8c8eaf1 HDDS-12944. Reduce timeout for integration check (apache#8374) 40d2e00 HDDS-11141. Avoid log flood due due pipeline close in XceiverServerRatis (apache#8325) 9fe1dba HDDS-12942. Init layout version config should not be public (apache#8373) 452e7aa HDDS-12596. OM fs snapshot max limit is not enforced (apache#8377) 8b095d5 HDDS-12795. Rename heartbeat and first election configuration name (apache#8249) bee8164 HDDS-12920. Configure log4j to gzip rolled over service log files (apache#8357) e16a50f HDDS-12934. Split submodule for Freon. (apache#8367) b1e9511 HDDS-12925. Update datanode volume used space on container deletion (apache#8364) 440bc82 Revert "HDDS-12596. OM fs snapshot max limit is not enforced (apache#8157)" f345492 HDDS-12596. OM fs snapshot max limit is not enforced (apache#8157) ee7b1dc HDDS-12901. Introduce EventExecutorMetrics instead of setting the metrics props unsafely (apache#8371) 810e148 HDDS-12939. Remove UnknownPipelineStateException. (apache#8372) 560fcdf HDDS-12728. Add Ozone 2.0.0 to compatibility acceptance tests (apache#8361) 5815a47 HDDS-12933. Remove the table names declared in OmMetadataManagerImpl (apache#8370) ee32fa5 HDDS-12560. Reclaimable Filter for Snaphost Garbage Collections (apache#8053) 45374ea HDDS-12932. Rewrite OMDBDefinition (apache#8362) 5cb6dd8 HDDS-12575. Set default JUnit5 timeout via property (apache#8348) 8efc0cd HDDS-11633. Delete message body too large, causing SCM to fail writing raft log (apache#8354) ac9d9fd HDDS-12915. Intermittent failure in testCreatePipelineThrowErrorWithDataNodeLimit (apache#8359) 86039e8 HDDS-12848. Create new submodule for ozone admin (apache#8292) 2d0f8cb HDDS-12833. Remove the CodecRegistry field from DBStoreBuilder (apache#8327) c71b393 HDDS-12921. UnusedPrivateField violations in tests (apache#8353) 9f3dd01 HDDS-12917. cp: option '--update' doesn't allow an argument (apache#8346) a14b395 HDDS-12922. Use OMDBDefinition in GeneratorOm and FSORepairTool (apache#8355) c8a98d6 HDDS-12892. OM Tagging Request incorrectly sets full path as key name for FSO (apache#8345) ade69e3 HDDS-12649. Include name of volume or bucket in length validation error (apache#8322) c68308d HDDS-12599. Create an ozone debug CLI command to list all the containers based on final state (apache#8282) 319d5a4 HDDS-12773. bad substitution in bats test (apache#8290) 6f5e02a HDDS-12900. (addendum: fix pmd) Use OMDBDefinition in OmMetadataManagerImpl (apache#8337) 2a7000d HDDS-12900. Use OMDBDefinition in OmMetadataManagerImpl (apache#8337) 9c0c66c HDDS-12915. Mark testCreatePipelineThrowErrorWithDataNodeLimit as flaky a73e052 HDDS-12907. Enable FieldDeclarationsShouldBeAtStartOfClass PMD rule (apache#8344) d083f82 HDDS-12905. Move field declarations to start of class in ozone-common (apache#8342) 3f90e1c HDDS-12906. Move field declarations to start of class in ozone-manager module (apache#8343) 403fb97 HDDS-12878. Move field declarations to start of class in tests (apache#8308) 63d5c73 HDDS-12912. Remove deprecated `PipelineManager#closePipeline(Pipeline, boolean)` (apache#8340) cf1fb88 HDDS-12902. Shutdown executor in CloseContainerCommandHandler and ECReconstructionCoordinator (apache#8341) 825ba02 HDDS-9585. Improve import/export log in ContainerLogger (apache#8330) b70d35a HDDS-12889. Enable AppendCharacterWithChar PMD rule (apache#8324) cd308ea HDDS-12904. Move field declarations to start of class in other hdds modules (apache#8336) 4905286 HDDS-12899. Move field declarations to start of class in hdds-server-scm (apache#8332) Conflicts: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerLogger.java hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReplica.java hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/ReconcileSubcommand.java hadoop-ozone/recon-codegen/src/main/java/org/apache/ozone/recon/schema/ContainerSchemaDefinition.java

…anner-builds-mt * HDDS-10239-container-reconciliation: (433 commits) Revert "HDDS-11232. Spare InfoBucket RPC call for the FileSystem#getFileStatus calls for the general case. (apache#6988)" (apache#8358) HDDS-12993. Use DatanodeID in ReconNodeManager. (apache#8456) HDDS-13024. HTTP connect to 0.0.0.0 failed (apache#8439) HDDS-12914. Bump awssdk to 2.31.40, test ResumableFileDownload (apache#8455) HDDS-4677. Document Ozone Ports and Connection End Points (apache#8226) HDDS-13044. Remove DatanodeDetails#getUuid usages from hdds-common/client/container-service (apache#8462) HDDS-12568. Implement MiniOzoneCluster.Service for Recon (apache#8452) HDDS-13046. Add .vscode to .gitignore (apache#8461) HDDS-13022. Split up exclusive size tracking for key and directory cleanup in SnapshotInfo (apache#8433) HDDS-12966. DBDefinitionFactory should not throw InvalidArnException (apache#8454) HDDS-12948. [Snapshot] Increase `ozone.om.fs.snapshot.max.limit` default value to 10k (apache#8376) HDDS-11904. Fix HealthyPipelineSafeModeRule logic. (apache#8386) HDDS-12989. Throw CodecException for the Codec byte[] methods (apache#8444) HDDS-11298. Support owner field in the S3 listBuckets request (apache#8315) HDDS-12810. Check and reserve space atomically in VolumeChoosingPolicy (apache#8360) HDDS-13016. Add a getAllNodeCount() method to NodeManager. (apache#8445) HDDS-12299. Merge OzoneAclConfig into OmConfig (apache#8383) HDDS-12501. Remove OMKeyInfo from SST files in backup directory. (apache#8214) HDDS-13021. Make callId unique for each request in AbstractKeyDeletingService (apache#8432) HDDS-13000. [Snapshot] listSnapshotDiffJobs throws NPE. (apache#8418) ...

…che#8214)

…ectory. (apache#8214) (cherry picked from commit 96f4f79)

swamirishi requested changes Apr 2, 2025
View reviewed changes

hemantk-12 reviewed Apr 3, 2025
View reviewed changes

errose28 added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Apr 7, 2025

peterxcli reviewed Apr 8, 2025
View reviewed changes

swamirishi requested changes Apr 8, 2025
View reviewed changes

saketa added 2 commits April 18, 2025 14:59

HDDS-12501. Remove OMKeyInfo from SST files in backup directory.
a383c2b

HDDS-12501. Added Read/Write SSt File lock.
37edeeb

SaketaChalamchala force-pushed the HDDS-12501 branch from 80c3e3e to b7c9663Compare April 18, 2025 22:07

HDDS-12501. Used BlockingQueue to prune backup SST files on compactio…
73e0d2a
…n complete. Using SST lock from OM.

SaketaChalamchala force-pushed the HDDS-12501 branch from b7c9663 to 73e0d2aCompare April 18, 2025 22:11

hemantk-12 reviewed Apr 22, 2025
View reviewed changes

Comment thread...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/compaction/log/CompactionFileInfo.java

hemantk-12 reviewed Apr 22, 2025
View reviewed changes

saketaand others added 4 commits April 28, 2025 22:32

HDDS-12501. Added Test cases. Addressed comments.
d311255

Merge branch 'master' into HDDS-12501
2922ba6

HDDS-12501. Findbugs and pmd fix.
53c6185

HDDS-12501. More findbugs fixes. Fixed exception handling.
7cfbb1d

swamirishi requested changes Apr 30, 2025
View reviewed changes

swamirishi reviewed Apr 30, 2025
View reviewed changes

saketa added 2 commits April 30, 2025 19:42

HDDS-12501. Removed locks and native library loader optimization.
17d24c1

HDDS-12501. Fixed integration(snapshot) test.
cb90a23

Merge branch 'master' into HDDS-12501
453111f

swamirishi requested changes May 2, 2025
View reviewed changes

saketaand others added 4 commits May 7, 2025 00:29

HDDS-12501. Simplified Exception handling.
d833f8a

Merge branch 'master' into HDDS-12501
fe4ee50

HDDS-12501. Fixed findbugs and compilation issue.
e2f8821

HDDS-12501. Fixed unit test.
b11fe0a

jojochuang requested review from peterxcli and swamirishi May 7, 2025 23:22

swamirishi requested changes May 8, 2025
View reviewed changes

swamirishi reviewed May 8, 2025
View reviewed changes

HDDS-12501. Addressed comments.
2272837

swamirishi reviewed May 9, 2025
View reviewed changes

SaketaChalamchala marked this pull request as ready for review May 9, 2025 18:32

swamirishi reviewed May 12, 2025
View reviewed changes

swamirishi requested changes May 12, 2025
View reviewed changes

HDDS-12501. Moved pruning to a separate function. Added CompactionLog…
13e6465
…Entry.toBuilder().

swamirishi approved these changes May 12, 2025
View reviewed changes

SaketaChalamchala commented May 12, 2025
View reviewed changes

Comment thread...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/compaction/log/CompactionLogEntry.java Outdated

SaketaChalamchala commented May 12, 2025
View reviewed changes

Comment thread...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java Outdated

SaketaChalamchala commented May 12, 2025
View reviewed changes

Comment thread...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java Outdated

SaketaChalamchala commented May 12, 2025
View reviewed changes

Comment thread...ksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java Outdated

SaketaChalamchalaand others added 5 commits May 12, 2025 14:34

Apply suggestions from code review
97ac54f

Update toBuilder()
e1487b6
Co-authored-by: Swaminathan Balachandran <47532440+swamirishi@users.noreply.github.com>

HDDS-12501. Fixed checkstyle and build issue.
1a774a5

Delete hadoop-ozone/integration-test/audit.log
6aa7525

Merge branch 'apache:master' into HDDS-12501
1bddeef

swamirishi merged commit 96f4f79 into apache:masterMay 13, 2025

Tejaskriya pushed a commit to Tejaskriya/ozone that referenced this pull request May 29, 2025
HDDS-12501. Remove OMKeyInfo from SST files in backup directory. (apa…
4669746
…che#8214)

SaketaChalamchala mentioned this pull request May 30, 2025
HDDS-13011. Disable Snapshot DAG Pruner Thread. #8529
Closed

swamirishi pushed a commit to swamirishi/ozone that referenced this pull request Dec 3, 2025
CDPD-80349. HDDS-12501. Remove OMKeyInfo from SST files in backup dir…
ca59e00
…ectory. (apache#8214) (cherry picked from commit 96f4f79)

	StringsstFullPath = getSSTFullPath(sst, src.getDbPath());
	Pathlink = Paths.get(sstFilesDirForSnapDiffJob,
	sst + SST_FILE_EXTENSION);
	PathsrcFile = Paths.get(sstFullPath);
	createLink(link, srcFile);
	returnlink.toString();

		ManagedEnvOptions envOptions = new ManagedEnvOptions();
		ManagedOptions managedOptions = new ManagedOptions();

	// Move file.sst.tmp file.sst and replace existing file atomically
	// Move file.sst.tmp to file.sst and replace existing file atomically

	thrownewRuntimeException(e);
	Files.deleteIfExists(prunedSSTFile.toPath());
	thrownewRuntimeException(e);

	publicstaticRocksDBCheckpointDiffergetInstance(
	StringmetadataDirName,
	StringsstBackupDirName,
	StringcompactionLogDirName,
	StringactiveDBLocationName,
	ConfigurationSourceconfiguration
	) {

Uh oh!

Conversation

SaketaChalamchala commented Apr 2, 2025

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

SaketaChalamchala commented Apr 2, 2025

Uh oh!

swamirishi left a comment

Choose a reason for hiding this comment

Uh oh!

swamirishiApr 2, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hemantk-12Apr 7, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swamirishi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hemantk-12Apr 22, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SaketaChalamchalaApr 23, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hemantk-12Apr 23, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

swamirishiApr 2, 2025•
edited
Loading

hemantk-12Apr 7, 2025•
edited
Loading

hemantk-12Apr 22, 2025•
edited
Loading

SaketaChalamchalaApr 23, 2025•
edited
Loading

hemantk-12Apr 23, 2025•
edited
Loading