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/02/06 20:29:32 UTC
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #4249: HDDS-7906. [Snapshot] Wait for checkpoint creation if snapshot in cache and not committed to DB.
hemantk-12 commented on code in PR #4249:
URL: https://github.com/apache/ozone/pull/4249#discussion_r1097826971
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/ratis/TestOzoneManagerDoubleBuffer.java:
##########
@@ -68,9 +69,9 @@ class TestOzoneManagerDoubleBuffer {
private static OMClientResponse omBucketCreateResponse =
mock(OMBucketCreateResponse.class);
private static OMClientResponse omSnapshotCreateResponse1 =
- mock(OMBucketCreateResponse.class);
+ mock(OMSnapshotCreateResponse.class);
Review Comment:
Thanks for fixing this.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerDoubleBuffer.java:
##########
@@ -440,10 +440,10 @@ private String addToBatch(Queue<DoubleBufferEntry<OMClientResponse>> buffer,
// 1. It is first element in the response,
// 2. Current request is createSnapshot request.
// 3. Previous request was createSnapshot request.
- if (response.isEmpty() ||
- omResponse.getCreateSnapshotResponse() != null ||
- (previousOmResponse != null &&
- previousOmResponse.getCreateSnapshotResponse() != null)) {
+ if (response.isEmpty() || omResponse.getCreateSnapshotResponse()
Review Comment:
Wouldn't `omResponse.getCreateSnapshotResponse().hasSnapshotInfo()` throw NPE if `omResponse.getCreateSnapshotResponse()` returns null? Or that's not possible?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -911,6 +918,22 @@ private boolean isKeyPresentInTableCache(String keyPrefix,
return false;
}
+
+ static boolean isSnapshotPresentInTableCache(String keyPrefix, Table table) {
Review Comment:
1. The function name is wrong because `isSnapshotPresentInTableCache` seems specific to snapshot but we are passing table to make it generic. Either change the function to something generic or move it to `OmSnapshotManager` .
1. Why do we need to iterator over the table instead of just using [`get`](https://github.com/apache/ozone/blob/074d225f2ab0c9658ffa35f021c6cba2116f40cc/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/Table.java#L85) or [getSkipCache](https://github.com/apache/ozone/blob/074d225f2ab0c9658ffa35f021c6cba2116f40cc/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/Table.java#L96)?
1. Wouldn't it always return `true` because [entry is in cache](https://github.com/hemantk-12/ozone/blob/ccc814ee7f8a7cac70b6d64dcae12056e375199d/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java#L149? Please correct me if I'm wrong.
1. One more doubt if flushing the RocksDB operations queue actually creates checkpointing dir because it could be another async operation. Please double check that.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -2209,4 +2209,46 @@ private void createLinkBucket(String linkVolume, String linkBucket,
OzoneVolume ozoneVolume = objectStore.getVolume(linkVolume);
ozoneVolume.createBucket(linkBucket, builder.build());
}
+
+ @Test
+ public void testSnapshotRead() throws Exception {
+ // Init data
+ OzoneBucket bucket1 =
+ TestDataUtil.createVolumeAndBucket(cluster, bucketLayout);
+ Path volume1Path = new Path(OZONE_URI_DELIMITER, bucket1.getVolumeName());
+ Path bucket1Path = new Path(volume1Path, bucket1.getName());
+ Path file1 = new Path(bucket1Path, "key1");
+ Path file2 = new Path(bucket1Path, "key2");
+
+ ContractTestUtils.touch(fs, file1);
+ ContractTestUtils.touch(fs, file2);
+
+ OzoneBucket bucket2 =
+ TestDataUtil.createVolumeAndBucket(cluster, bucketLayout);
+ Path volume2Path = new Path(OZONE_URI_DELIMITER, bucket2.getVolumeName());
+ Path bucket2Path = new Path(volume2Path, bucket2.getName());
+
+ fs.mkdirs(bucket2Path);
+ Path snapPath1 = fs.createSnapshot(bucket1Path, "snap1");
+ Path snapPath2 = fs.createSnapshot(bucket2Path, "snap1");
+
+ Path file3 = new Path(bucket1Path, "key3");
+ ContractTestUtils.touch(fs, file3);
+
+ Path snapPath3 = fs.createSnapshot(bucket1Path, "snap2");
+
+ try {
+ FileStatus[] f1 = fs.listStatus(snapPath1);
+ FileStatus[] f2 = fs.listStatus(snapPath2);
+ FileStatus[] f3 = fs.listStatus(snapPath3);
+ Assert.assertEquals(2, f1.length);
+ Assert.assertEquals(0, f2.length);
+ Assert.assertEquals(3, f3.length);
+ } catch (Exception e) {
+ Assert.fail("Failed to read/list on snapshotPath, exception: " + e);
+ }
+
+ }
+
Review Comment:
nit: please remove extra line.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java:
##########
@@ -2209,4 +2209,46 @@ private void createLinkBucket(String linkVolume, String linkBucket,
OzoneVolume ozoneVolume = objectStore.getVolume(linkVolume);
ozoneVolume.createBucket(linkBucket, builder.build());
}
+
+ @Test
+ public void testSnapshotRead() throws Exception {
+ // Init data
+ OzoneBucket bucket1 =
+ TestDataUtil.createVolumeAndBucket(cluster, bucketLayout);
+ Path volume1Path = new Path(OZONE_URI_DELIMITER, bucket1.getVolumeName());
+ Path bucket1Path = new Path(volume1Path, bucket1.getName());
+ Path file1 = new Path(bucket1Path, "key1");
+ Path file2 = new Path(bucket1Path, "key2");
+
+ ContractTestUtils.touch(fs, file1);
+ ContractTestUtils.touch(fs, file2);
+
+ OzoneBucket bucket2 =
+ TestDataUtil.createVolumeAndBucket(cluster, bucketLayout);
+ Path volume2Path = new Path(OZONE_URI_DELIMITER, bucket2.getVolumeName());
+ Path bucket2Path = new Path(volume2Path, bucket2.getName());
+
+ fs.mkdirs(bucket2Path);
+ Path snapPath1 = fs.createSnapshot(bucket1Path, "snap1");
+ Path snapPath2 = fs.createSnapshot(bucket2Path, "snap1");
+
+ Path file3 = new Path(bucket1Path, "key3");
+ ContractTestUtils.touch(fs, file3);
+
+ Path snapPath3 = fs.createSnapshot(bucket1Path, "snap2");
+
+ try {
+ FileStatus[] f1 = fs.listStatus(snapPath1);
+ FileStatus[] f2 = fs.listStatus(snapPath2);
+ FileStatus[] f3 = fs.listStatus(snapPath3);
+ Assert.assertEquals(2, f1.length);
+ Assert.assertEquals(0, f2.length);
+ Assert.assertEquals(3, f3.length);
+ } catch (Exception e) {
+ Assert.fail("Failed to read/list on snapshotPath, exception: " + e);
+ }
+
Review Comment:
nit: please remove extra line.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -322,9 +328,10 @@ private OmMetadataManagerImpl(OzoneConfiguration conf, String snapshotDirName)
* @throws IOException
*/
public static OmMetadataManagerImpl createSnapshotMetadataManager(
- OzoneConfiguration conf, String snapshotDirName) throws IOException {
- OmMetadataManagerImpl smm = new OmMetadataManagerImpl(conf,
- snapshotDirName);
+ OzoneConfiguration conf, String snapshotDirName,
+ boolean isSnapshotInCache) throws IOException {
+ OmMetadataManagerImpl smm = new OmMetadataManagerImpl(conf, snapshotDirName,
Review Comment:
nit: could be a one line.
```suggestion:
return new OmMetadataManagerImpl(conf, snapshotDirName,
isSnapshotInCache);
```
Also why this needs to be a static function. Why can't we use constructor directly? It is misuse of factory.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -302,14 +303,19 @@ protected OmMetadataManagerImpl() {
}
// metadata constructor for snapshots
- private OmMetadataManagerImpl(OzoneConfiguration conf, String snapshotDirName)
- throws IOException {
+ private OmMetadataManagerImpl(OzoneConfiguration conf, String snapshotDirName,
+ boolean isSnapshotInCache) throws IOException {
lock = new OmReadOnlyLock();
omEpoch = 0;
String snapshotDir = OMStorage.getOmDbDir(conf) +
OM_KEY_PREFIX + OM_SNAPSHOT_DIR;
- setStore(loadDB(conf, new File(snapshotDir),
- OM_DB_NAME + snapshotDirName, true));
+ File metaDir = new File(snapshotDir);
+ String dbName = OM_DB_NAME + snapshotDirName;
+ if (isSnapshotInCache) {
+ File checkpoint = Paths.get(metaDir.toPath().toString(), dbName).toFile();
+ RDBCheckpointManager.waitForCheckpointDirectoryExist(checkpoint);
Review Comment:
I doubt if this will solve this problem because it just waits for 5 secs and swallows the exception. This either doesn't make sure that checkpoint directory will exists.
Ideal solution for this should be wait and notify but because dir creation happens inside RocksDB we can't notify Ozone when it gets created..
--
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