HDDS-8128. Deduplicate the ops in RDBBatchOperation. #4424

szetszwo · 2023-03-20T03:06:49Z

What changes were proposed in this pull request?

OM rocksdb uses a lot of space.

This PR adds a cache in RDBBatchOperation for caching the put ops. When there are multiple ops with the same key, it will overwrite the previous op in the cache so the previous op won't be written into RocksDB. In a test, the cache can reduce the disk usage in OM rocksdb of a single upload with 1000 parts from 755MB to 23MB.

What is the link to the Apache JIRA

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

How was this patch tested?

The is an optimization. The existing test already has covered. Also have tested this with ozone-1.3...szetszwo:ozone:multipartUpload manually.

Galsza

@szetszwo Thank you for working on this patch. I've left a few comments.

Please fix the batchSize resetting, the rest are nitpicks.

Galsza · 2023-03-21T13:27:29Z

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBBatchOperation.java

 public class RDBBatchOperation implements BatchOperation {
+  static final Logger LOG = LoggerFactory.getLogger(RDBBatchOperation.class);

+  static void debug(Supplier<String> message) {


I think this method should be private. Are we going to use it anywhere else?

Galsza · 2023-03-21T13:27:42Z

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBBatchOperation.java

+  class PutOpCache {
+    class FamilyCache {
+      private final ColumnFamily family;
+      /** A (key -> value) map. */


Suggested change

/** A (key -> value) map. */

/**

* The map used to store the key and value byte array pairs.

* <p>

* {@link ByteArray} is used for the keys to avoid incorrect map

* operations.

*/

Thanks for the suggestion. Will update it.

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBBatchOperation.java

Galsza · 2023-03-21T13:27:52Z

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBBatchOperation.java

+      }
+
+      void put(byte[] key, byte[] value) {
+        batchSize += key.length + value.length;


If batchSize is not reset anywhere this is going to overflow. Also, could this overflow even with a reset included? (Will cached put operations ever reach 2 gigs in size?)

Good point. We should use long for sizes.

Galsza · 2023-03-21T13:28:03Z

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBBatchOperation.java

+    }
+
+    /** A (family name -> {@link FamilyCache}) map. */
+    private final Map<String, FamilyCache> map = new HashMap<>();


I suggest renaming the variable to nameToCacheMap for easier understanding.

Galsza

Thank you for the changes, looks good to me.

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBBatchOperation.java

szetszwo · 2023-03-22T11:25:30Z

The force-pushed was to sync with master since TestEndPoint.testTmpDirCleanup will fail without #4445 (misused value as key). The values (misused as keys) in the tests are the same, this change removes the duplications and cause failure.

adoroszlai · 2023-03-22T11:28:08Z

force-pushed was to sync with master

Thanks for the info. Sync with master can also be done by merging instead of rebasing. The commits in the PR will be squashed anyway, so linearity doesn't matter.

szetszwo · 2023-03-22T14:29:54Z

@adoroszlai , thanks for the advices. I tend to prefer rebase over merge since it is easier to see the new code. I agree that when everything works fine, rebase or merge do not matter. When something does not work, rebase seems easier to trace the problem.

adoroszlai · 2023-03-22T15:28:37Z

I tend to prefer rebase over merge since it is easier to see the new code.

How does the reviewer see what changes were made to the PR since the previous round of review, i.e. to address the review comments?

szetszwo · 2023-03-23T03:21:19Z

If we rebase but not squash, the comments in Github may still work.

Also, I usually keep the old branch https://github.com/szetszwo/ozone/tree/HDDS-8128b . Reviewers may download the patches and the run diff locally (this is like posting patches on the JIRA).

szetszwo · 2023-03-23T03:24:20Z

Indeed, a previous comment still show in https://github.com/apache/ozone/pull/4424/files .

The other comments become outdated. I guess it is because the code has changed. It has nothing to do with merge or rebase.

adoroszlai · 2023-03-23T07:30:25Z

When posting patches to Jira, old ones are usually kept. Force-pushing is like keeping only the latest patch. Keeping around old branches helps, but you have to let others know about them, and even then it's still not as streamlined as incremental commits.

You are right that comments are still visible after force-push. However, it is hard to see the state of code they were referring to and how code was changed in response to them.

https://www.freecodecamp.org/news/optimize-pull-requests-for-reviewer-happiness#request-a-review

szetszwo · 2023-03-23T08:06:02Z

For merging, the downloaded patch file could be very messy since it may include the merged code from other commit. This is another reason I prefer rebase over merge.

... However, it is hard to see the state of code they were referring to and how code was changed in response to them.

Since the old branch is still around, we still can see the newer commits addressing the comments.

duongkame

Thanks @szetszwo for the change. This looks like a nice improvement and maybe RocksDB should've done it internally. I only put a few minor comments inline.

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBBatchOperation.java

szetszwo · 2023-03-24T01:25:18Z

@duongkame , thanks a lot for reviewing this! Just have pushed a new commit for addressing your comments.

swamirishi · 2025-07-09T23:29:53Z

@szetszwo Did we see the disk usage go up because of the rocksdb wal? or did we see an sst redundant sst flush happen?

szetszwo · 2025-07-10T05:22:57Z

WAL

szetszwo force-pushed the HDDS-8128 branch from 74a3ba8 to cff54ac Compare March 20, 2023 03:26

kerneltime requested review from duongkame and kerneltime March 20, 2023 05:20

Galsza suggested changes Mar 21, 2023

View reviewed changes

Galsza approved these changes Mar 22, 2023

View reviewed changes

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBBatchOperation.java Show resolved Hide resolved

szetszwo changed the title ~~HDDS-8128. OM rocksdb uses a lot of space.~~ HDDS-8128. Deduplicate the ops in RDBBatchOperation. Mar 22, 2023

HDDS-8128. Deduplicate the ops in RDBBatchOperation.

c7b1b87

szetszwo force-pushed the HDDS-8128 branch from 213a735 to c7b1b87 Compare March 22, 2023 11:21

duongkame approved these changes Mar 24, 2023

View reviewed changes

Address review comments.

56355af

szetszwo merged commit 386d1be into apache:master Mar 25, 2023

-      /** A (key -> value) map. */
+      /**
+       * The map used to store the key and value byte array pairs.
+       * <p>
+       * {@link ByteArray} is used for the keys to avoid incorrect map
+       * operations.
+       */

HDDS-8128. Deduplicate the ops in RDBBatchOperation. #4424

HDDS-8128. Deduplicate the ops in RDBBatchOperation. #4424

Uh oh!

Conversation

szetszwo commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Galsza 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!

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!

Galsza left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

szetszwo commented Mar 22, 2023

Uh oh!

adoroszlai commented Mar 22, 2023

Uh oh!

szetszwo commented Mar 22, 2023

Uh oh!

adoroszlai commented Mar 22, 2023

Uh oh!

szetszwo commented Mar 23, 2023

Uh oh!

szetszwo commented Mar 23, 2023

Uh oh!

adoroszlai commented Mar 23, 2023

Uh oh!

szetszwo commented Mar 23, 2023

Uh oh!

duongkame left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

szetszwo commented Mar 24, 2023

Uh oh!

swamirishi commented Jul 9, 2025

Uh oh!

szetszwo commented Jul 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

szetszwo commented Mar 20, 2023 •

edited

Loading