HDDS-8195. RDBStore.getUpdatesSince() throws RocksDBException: Requested array size exceeds VM limit by devmadhuu · Pull Request #4459 · apache/ozone

devmadhuu · 2023-03-23T16:43:18Z

Observations about below code in org.apache.hadoop.hdds.utils.db.RDBStore#getUpdatesSince(long, long)

dbUpdatesWrapper.addWriteBatch(result.writeBatch().data(),
result.sequenceNumber());

Above code is in while loop for all log files getting iterated and each log file batch allocates a byte[] array which results in accumulating the data in dbUpdatesWrapper in the form of dataList. This will increase JVM heap due to dataList growing with each allocated byte[] array getting added in dataList for dbUpdatesWrapper object and may further fail to allocate any byte[] array in the log iterator loop on calling "result.writeBatch().data()" code. If Recon has limited heap memory, this may fail frequently and may even fall into worse situation where a first call to "result.writeBatch().data()" may fail to allocate byte[] array and throw "org.rocksdb.RocksDBException: Requested array size exceeds VM limit".

So to reduce the chance of this byte[] array allocation failure, we need to ensure following 3 points:

Recon max memory -Xmx configuration should allocate sufficient heap.
Need to fix and avoid increase in recon heap due to increase in size of dataList wrapped in dbUpdatesWrapper object more than configured limit of default 1 GB. If it crosses more than 1 GB as default, break the loop and wait for delta updates since last sequence number in next periodic task run of delta updates.
Throw RocksDBException to client, so. that Recon client catches the exception and set fullSnapshot as True

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

Patch was tested locally on docker setup by creating very frequent writes and generated keys using freon tool and waited for recon to get OM DB sync updates.

…ted array size exceeds VM limit.

devmadhuu · 2023-03-23T16:44:01Z

@jojochuang @sumitagrawl @dombizita - pls review.

adoroszlai · 2023-03-23T16:45:43Z

Please create PR only after reasonably clean CI run in fork.

Checkstyle is failing:
https://github.com/devmadhuu/ozone/actions/runs/4503008611/jobs/7925644497#step:6:470

…ted array size exceeds VM limit.

devmadhuu · 2023-03-23T17:46:11Z

Please create PR only after reasonably clean CI run in fork.

Checkstyle is failing: https://github.com/devmadhuu/ozone/actions/runs/4503008611/jobs/7925644497#step:6:470

Ran both checkstyle and findbugs locally and all issues fixed. Kindly enable workflows.

…ted array size exceeds VM limit.

jojochuang

Look overall good to me. Is it possible to add a test?

jojochuang · 2023-03-24T18:23:36Z

 public class RDBStore implements DBStore {
  private static final Logger LOG =
      LoggerFactory.getLogger(RDBStore.class);
+  public static final int MAX_DB_UPDATES_SIZE_THRESHOLD = 1024 * 1024;


Shouldn't this be 1024 * 1024 * 1024?

@jojochuang - Thanks for the review. Yes you are right, I have changed that to 1024 * 1024 * 1024.

…ted array size exceeds VM limit.

devmadhuu · 2023-03-27T06:34:04Z

Thanks, I ran existing test cases of delta OM DB snapshots and covering the feature. They ran fine. If you are looking for any specific coverage of any usecase, please let me know.

sumitagrawl

@devmadhuu thanks for working over this, LGTM. UT case related to the scenario can be added, like crossing limit of sequence log file size

jojochuang

The code looks good to me. But it involves a few moving parts and a test is really needed to ensure different components respond properly, for example, does recon respond to OM properly when the DB update is not successful.

…ted array size exceeds VM limit.

devmadhuu · 2023-03-28T07:37:40Z

@sumitagrawl Thanks for the review. Additional UT and assertions have been updated. Pls re-review.

devmadhuu · 2023-03-28T07:37:55Z

The code looks good to me. But it involves a few moving parts and a test is really needed to ensure different components respond properly, for example, does recon respond to OM properly when the DB update is not successful.

@jojochuang - Thanks for the review. Additional UT and assertions have been updated. Pls re-review.

sumitagrawl

@devmadhuu Please handle one of minor comment

sumitagrawl · 2023-03-28T09:13:48Z

    this(dbFile, options, new ManagedWriteOptions(), families,
        new CodecRegistry(), false, 1000, null, false,
        TimeUnit.DAYS.toMillis(1), TimeUnit.HOURS.toMillis(1));
+    this.maxDbUpdatesSizeThreshold = MAX_DB_UPDATES_SIZE_THRESHOLD;


plz use common default value, do not use another default value 80 which will create confusion.

@sumitagrawl - I have handled this comment. Pls re-review.

adoroszlai · 2023-03-28T09:22:05Z

+  @SuppressWarnings("checkstyle:ParameterNumber")
+  @VisibleForTesting
+  public RDBStore(File dbFile, ManagedDBOptions rocksDBOption,
+                  ManagedWriteOptions writeOptions,
+                  Set<TableConfig> tableConfigs, CodecRegistry registry,
+                  boolean openReadOnly, int maxFSSnapshots,
+                  String dbJmxBeanNameName, boolean enableCompactionLog,
+                  long maxTimeAllowedForSnapshotInDag,
+                  long pruneCompactionDagDaemonRunInterval,
+                  long maxDbUpdatesSizeThreshold) throws IOException {
+    this(dbFile, rocksDBOption, writeOptions, tableConfigs, registry,
+        openReadOnly, maxFSSnapshots, dbJmxBeanNameName,
+        enableCompactionLog, maxTimeAllowedForSnapshotInDag,
+        pruneCompactionDagDaemonRunInterval);
+    this.maxDbUpdatesSizeThreshold = maxDbUpdatesSizeThreshold;
  }


Please add the new param to the existing constructor instead of creating a new one.

@adoroszlai - I have handled this review comment. Pls re-review.

adoroszlai · 2023-03-28T09:22:36Z

    LOG.info("Number of updates received from OM : {}, " +
            "SequenceNumber diff: {}, SequenceNumber Lag from OM {}.",
        numUpdates, getCurrentOMDBSequenceNumber() - fromSequenceNumber, lag);
+    return null != dbUpdates ? dbUpdates.isDBUpdateSuccess() : false;


nit:

Suggested change

return null != dbUpdates ? dbUpdates.isDBUpdateSuccess() : false;

return null != dbUpdates && dbUpdates.isDBUpdateSuccess();

@adoroszlai - I have handled this suggestion. Pls re-review.

…ted array size exceeds VM limit.

jojochuang

LGTM from me

adoroszlai · 2023-03-28T21:53:22Z

Thanks @devmadhuu for the patch, @jojochuang, @sumitagrawl for the review.

HDDS-8195. RDBStore.getUpdatesSince() throws RocksDBException: Reques…

fd67da4

…ted array size exceeds VM limit.

adoroszlai marked this pull request as draft March 23, 2023 16:45

deveshsingh added 4 commits March 23, 2023 22:18

HDDS-8195. RDBStore.getUpdatesSince() throws RocksDBException: Reques…

2adf01f

…ted array size exceeds VM limit.

HDDS-8195. RDBStore.getUpdatesSince() throws RocksDBException: Reques…

7c9fb3b

…ted array size exceeds VM limit.

HDDS-8195. RDBStore.getUpdatesSince() throws RocksDBException: Reques…

569ebe7

…ted array size exceeds VM limit.

HDDS-8195. RDBStore.getUpdatesSince() throws RocksDBException: Reques…

600a38a

…ted array size exceeds VM limit.

devmadhuu marked this pull request as ready for review March 23, 2023 17:45

deveshsingh added 3 commits March 23, 2023 23:17

HDDS-8195. RDBStore.getUpdatesSince() throws RocksDBException: Reques…

03a8ae6

…ted array size exceeds VM limit.

HDDS-8195. RDBStore.getUpdatesSince() throws RocksDBException: Reques…

962d505

…ted array size exceeds VM limit.

HDDS-8195. RDBStore.getUpdatesSince() throws RocksDBException: Reques…

df07437

…ted array size exceeds VM limit.

jojochuang reviewed Mar 24, 2023

View reviewed changes

HDDS-8195. RDBStore.getUpdatesSince() throws RocksDBException: Reques…

e011f1b

…ted array size exceeds VM limit.

sumitagrawl approved these changes Mar 27, 2023

View reviewed changes

jojochuang approved these changes Mar 27, 2023

View reviewed changes

HDDS-8195. RDBStore.getUpdatesSince() throws RocksDBException: Reques…

4e63faf

…ted array size exceeds VM limit.

adoroszlai requested review from jojochuang and sumitagrawl and removed request for sumitagrawl March 28, 2023 08:16

sumitagrawl requested changes Mar 28, 2023

View reviewed changes

adoroszlai reviewed Mar 28, 2023

View reviewed changes

deveshsingh added 2 commits March 28, 2023 20:30

HDDS-8195. RDBStore.getUpdatesSince() throws RocksDBException: Reques…

4793730

…ted array size exceeds VM limit.

HDDS-8195. RDBStore.getUpdatesSince() throws RocksDBException: Reques…

bcf9874

…ted array size exceeds VM limit.

jojochuang approved these changes Mar 28, 2023

View reviewed changes

adoroszlai merged commit 2a82613 into apache:master Mar 28, 2023

	return null != dbUpdates ? dbUpdates.isDBUpdateSuccess() : false;
	return null != dbUpdates && dbUpdates.isDBUpdateSuccess();

Conversation

devmadhuu commented Mar 23, 2023

Uh oh!

devmadhuu commented Mar 23, 2023

Uh oh!

adoroszlai commented Mar 23, 2023

Uh oh!

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

Uh oh!

Uh oh!

jojochuang left a comment • 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!

devmadhuu commented Mar 27, 2023

Uh oh!

sumitagrawl left a comment

Choose a reason for hiding this comment

Uh oh!

jojochuang left a comment

Choose a reason for hiding this comment

Uh oh!

devmadhuu commented Mar 28, 2023

Uh oh!

devmadhuu commented Mar 28, 2023

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

jojochuang left a comment

Choose a reason for hiding this comment

Uh oh!

adoroszlai commented Mar 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

devmadhuu commented Mar 23, 2023 •

edited

Loading

jojochuang left a comment •

edited

Loading