You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "hemantk-12 (via GitHub)" <gi...@apache.org> on 2023/01/28 01:55:32 UTC
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #4214: HDDS-7845. Added wait for checkpoint directory creation in rocksDB
hemantk-12 commented on code in PR #4214:
URL: https://github.com/apache/ozone/pull/4214#discussion_r1089334227
##########
hadoop-hdds/framework/pom.xml:
##########
@@ -166,7 +166,11 @@ https://maven.apache.org/xsd/maven-4.0.0.xsd">
<artifactId>rocksdb-checkpoint-differ</artifactId>
<version>${hdds.version}</version>
</dependency>
-
+ <dependency>
+ <groupId>org.awaitility</groupId>
+ <artifactId>awaitility</artifactId>
+ <version>4.2.0</version>
+ </dependency>
Review Comment:
Thanks for the detailed steps to add dependency.
Updated as suggested.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBCheckpointManager.java:
##########
@@ -85,6 +87,8 @@ public RocksDBCheckpoint createCheckpoint(String parentDir, String name) {
LOG.info("Created checkpoint at {} in {} milliseconds",
checkpointPath, duration);
+ waitForCheckpointDirectoryExist(checkpointPath.toFile());
+
return new RocksDBCheckpoint(
checkpointPath,
currentTime,
Review Comment:
Checkpoint object is always valid. It is just that RocksDB has not flushed the queue and created the actual directory.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBCheckpointManager.java:
##########
@@ -96,6 +100,21 @@ public RocksDBCheckpoint createCheckpoint(String parentDir, String name) {
return null;
}
+ /**
+ * Wait for checkpoint directory gets created for 10 secs.
+ * <p>
+ * By default, Awaitility waits for 10 seconds and then throw
+ * a ConditionTimeoutException if the condition has not been fulfilled.
+ * The default poll interval and poll delay is 100 milliseconds.
+ */
+ private void waitForCheckpointDirectoryExist(File file) throws IOException {
+ try {
+ await().until(file::exists);
+ } catch (ConditionTimeoutException exception) {
+ LOG.info("Checkpoint directory didn't get created in 10 secs.");
Review Comment:
Added dir name.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBCheckpointManager.java:
##########
@@ -85,6 +87,8 @@ public RocksDBCheckpoint createCheckpoint(String parentDir, String name) {
LOG.info("Created checkpoint at {} in {} milliseconds",
checkpointPath, duration);
+ waitForCheckpointDirectoryExist(checkpointPath.toFile());
+
return new RocksDBCheckpoint(
checkpointPath,
currentTime,
Review Comment:
As discussed offline, it is not good idea to throw exception in case of timeout. We have to look into more details how we can make sure that rocksDB flush the queue and create checkpointing dir.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBCheckpointManager.java:
##########
@@ -85,6 +87,8 @@ public RocksDBCheckpoint createCheckpoint(String parentDir, String name) {
LOG.info("Created checkpoint at {} in {} milliseconds",
checkpointPath, duration);
Review Comment:
Checkpoint dir creation is a async task. As Prashant mentioned it is better to keep this separate from wait method.
Added another success log in `waitForCheckpointDirectoryExist` function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org