You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "swamirishi (via GitHub)" <gi...@apache.org> on 2023/07/07 21:17:10 UTC
[GitHub] [ozone] swamirishi opened a new pull request, #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
swamirishi opened a new pull request, #5035:
URL: https://github.com/apache/ozone/pull/5035
Currently when only incremental sst files are copied. The sst files are listed in the candidate rocksdb directory & are excluded from copying from leader. But SSTFiltering service could interfere with this and go about deleting the sst files which have been excluded while the data is being copied. Thus when the leader state is copied and installed certain sst files may be missing which could be present in the leader's sst manifest file.
## What changes were proposed in this pull request?
sstfiltering service should be stopped before copying the sst files.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8987
## How was this patch tested?
Existing unit tests & integration tests.
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261968372
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -231,4 +241,10 @@ public AtomicLong getSnapshotFilteredCount() {
public BootstrapStateHandler.Lock getBootstrapStateLock() {
return lock;
}
+
+ @Override
+ public void shutdown() {
Review Comment:
done resolving this
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1262594823
##########
hadoop-hdds/rocksdb-checkpoint-differ/pom.xml:
##########
@@ -82,6 +82,10 @@ https://maven.apache.org/xsd/maven-4.0.0.xsd">
<groupId>org.apache.ozone</groupId>
<artifactId>hdds-rocks-native</artifactId>
</dependency>
+ <dependency>
Review Comment:
removed it
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1258629967
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java:
##########
@@ -979,9 +982,17 @@ public void deleteFilesNotMatchingPrefix(
+ " {} from db: {}", sstFileName,
liveFileMetaData.columnFamilyName(), db.get().getName());
db.get().deleteFile(sstFileName);
+ filesToBeDeleted.add(new File(liveFileMetaData.path(),
+ liveFileMetaData.fileName()));
}
}
}
+
+ Iterator<File> files = filesToBeDeleted.iterator();
+ while (files.hasNext()) {
Review Comment:
I see.
If delete to be synchronous, I think `waitForFileDelete` should be internal to `RocksDatabase` or should be moved to [RocksDBUtils](https://github.com/apache/ozone/blob/master/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/RocksDBUtils.java)/[RdbUtil](https://github.com/apache/ozone/blob/master/hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/RdbUtil.java). As the name says `RDBCheckpointUtils` is for Checkpoint and not for a general RDB util.
--
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
[GitHub] [ozone] GeorgeJahad commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1258708697
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3577,18 +3578,24 @@ public List<OzoneAcl> getAcl(OzoneObj obj) throws IOException {
* @return If checkpoint is installed successfully, return the
* corresponding termIndex. Otherwise, return null.
*/
- public TermIndex installSnapshotFromLeader(String leaderId) {
+ public synchronized TermIndex installSnapshotFromLeader(String leaderId) {
if (omRatisSnapshotProvider == null) {
LOG.error("OM Snapshot Provider is not configured as there are no peer " +
"nodes.");
return null;
}
+ java.util.Optional<SstFilteringService> sstFilteringService =
+ java.util.Optional.ofNullable(
+ keyManager.getSnapshotSstFilteringService());
DBCheckpoint omDBCheckpoint;
+ sstFilteringService.ifPresent(BackgroundService::shutdown);
Review Comment:
yes, I believe the sst filtering service is already being stopped here: https://github.com/apache/ozone/blob/ffd84e747dc43d08a23269a8f411d52fef41c432/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L3640 Why do we need to do it again?
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. [Snapshot] Make RocksDB delete sstFile api synchronous. Invalidate Snapshot cache before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1267498449
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksDB.java:
##########
@@ -78,4 +86,20 @@ public static ManagedRocksDB open(
RocksDB.open(path, columnFamilyDescriptors, columnFamilyHandles)
);
}
+
+ /**
+ * Delete liveMetaDataFile from rocks db using RocksDB#deleteFile Api.
+ * This function makes the RocksDB#deleteFile Api synchronized by waiting
+ * for the deletes to happen.
+ * @param fileToBeDeleted File to be deleted.
+ * @throws RocksDBException In the underlying db throws an exception.
+ * @throws IOException In the case file is not deleted.
+ */
+ public void deleteFile(LiveFileMetaData fileToBeDeleted)
+ throws RocksDBException, IOException {
+ String sstFileName = fileToBeDeleted.fileName();
+ this.get().deleteFile(sstFileName);
+ File file = new File(fileToBeDeleted.path(), fileToBeDeleted.fileName());
+ ManagedRocksObjectUtils.waitForFileDelete(file, Duration.ofSeconds(60));
Review Comment:
Based on the current usage of this function, only SST filtering service will fail.
https://github.com/apache/ozone/blob/7a19afa71401e356cd86a99648e94278ea2850c8/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java#L193
--
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
[GitHub] [ozone] GeorgeJahad commented on pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on PR #5035:
URL: https://github.com/apache/ozone/pull/5035#issuecomment-1629799737
> excluded SST files list based on current state of the follower.
There is some ambiguity here, so let me try to be precise.
Both the leader and follower are running their own version of the sst filtering service simultaneously.
The excluded list is only generated from the followers candidate directory, not from the followers active fs. (The candidate directory is a seperate directory where each tarball is untarred until the follower catches up.)
Since the SFS is not deleting files from the candidate directory, the follower's SFS can't effect this part of the bootstrap process.
So @hemantk-12 statement above is more precisely: *excluded SST files list based on current state of the **follower's candidate directory***
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1258811904
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3577,18 +3578,24 @@ public List<OzoneAcl> getAcl(OzoneObj obj) throws IOException {
* @return If checkpoint is installed successfully, return the
* corresponding termIndex. Otherwise, return null.
*/
- public TermIndex installSnapshotFromLeader(String leaderId) {
+ public synchronized TermIndex installSnapshotFromLeader(String leaderId) {
if (omRatisSnapshotProvider == null) {
LOG.error("OM Snapshot Provider is not configured as there are no peer " +
"nodes.");
return null;
}
+ java.util.Optional<SstFilteringService> sstFilteringService =
+ java.util.Optional.ofNullable(
+ keyManager.getSnapshotSstFilteringService());
DBCheckpoint omDBCheckpoint;
+ sstFilteringService.ifPresent(BackgroundService::shutdown);
Review Comment:
Had an offline discussion with Swami and Prashant. Race condition Swami is trying to solve here is when follower requests leader for tarball (incremental diff) with ~excluded SST files list based on current state of the follower~ excluded SST files list based on current state of the follower's candidate directory, leader will only send the diff SST files and new manifest file. In meanwhile SST Filtering service might delete irrelevant SST files. Follower loads DB based on new manifest from leader and when it does that some files might not existed on follower because SST Filtering service has removed them in between fetching and loading. This leaves DB (manifest and data) into inconsistent state. Because manifest contains SST files which actual doesn't exist on follower.
I hope this help @GeorgeJahad.
@swamirishi added more details here: https://github.com/apache/ozone/pull/5035/#issuecomment-1629504655
--
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
[GitHub] [ozone] GeorgeJahad commented on pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on PR #5035:
URL: https://github.com/apache/ozone/pull/5035#issuecomment-1633050325
> The rocksdb delete api is asynchronous. The files to be deleted are added to a scheduler.
This makes sense; that means there really is a bug in my locking code, because I was assuming the delete api was synchronous. Thanks for figuring it out!
However, now I see that the fix you created to make it synchronous was removed in this commit: https://github.com/apache/ozone/pull/5035/commits/2a14794d916e9e667a2ff073c8360db39c47f576
Why was it removed?
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1262588394
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksDB.java:
##########
@@ -78,4 +86,21 @@ public static ManagedRocksDB open(
RocksDB.open(path, columnFamilyDescriptors, columnFamilyHandles)
);
}
+
+ /**
+ * Delete liveMetaDataFile from rocks db.
+ * @param fileToBeDeleted File to be deleted.
+ * @throws RocksDBException In the underlying db throws an exception.
+ * @throws IOException In the case file is not deleted.
+ */
+
+ public void deleteFile(LiveFileMetaData fileToBeDeleted)
+ throws RocksDBException, IOException {
+ String sstFileName = fileToBeDeleted.fileName();
+ this.get().deleteFile(sstFileName);
+ File file = new File(fileToBeDeleted.path(), fileToBeDeleted.fileName());
+ LOG.info("Deleting file {} from db: {}", file.getAbsolutePath(),
Review Comment:
anyhow removed the log
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksDB.java:
##########
@@ -78,4 +87,15 @@ public static ManagedRocksDB open(
RocksDB.open(path, columnFamilyDescriptors, columnFamilyHandles)
);
}
+
+ public void deleteFile(LiveFileMetaData fileToBeDeleted)
Review Comment:
done
--
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
[GitHub] [ozone] prashantpogde merged pull request #5035: HDDS-8987. [Snapshot] Make RocksDB delete sstFile api synchronous. Invalidate Snapshot cache before copying incremental snapshot
Posted by "prashantpogde (via GitHub)" <gi...@apache.org>.
prashantpogde merged PR #5035:
URL: https://github.com/apache/ozone/pull/5035
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1258936736
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -231,4 +241,10 @@ public AtomicLong getSnapshotFilteredCount() {
public BootstrapStateHandler.Lock getBootstrapStateLock() {
return lock;
}
+
+ @Override
Review Comment:
The order is correct. when running is set to false the loop will break for processing the next snapshot. It would be better if we do it this way.
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1258629967
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java:
##########
@@ -979,9 +982,17 @@ public void deleteFilesNotMatchingPrefix(
+ " {} from db: {}", sstFileName,
liveFileMetaData.columnFamilyName(), db.get().getName());
db.get().deleteFile(sstFileName);
+ filesToBeDeleted.add(new File(liveFileMetaData.path(),
+ liveFileMetaData.fileName()));
}
}
}
+
+ Iterator<File> files = filesToBeDeleted.iterator();
+ while (files.hasNext()) {
Review Comment:
I see.
If delete needs to be synchronous, I think `waitForFileDelete` should be internal to `RocksDatabase` or should be moved to [RocksDBUtils](https://github.com/apache/ozone/blob/master/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/RocksDBUtils.java)/[RdbUtil](https://github.com/apache/ozone/blob/master/hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/RdbUtil.java). As the name says `RDBCheckpointUtils` is for Checkpoint and not for a general RDB util.
--
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
[GitHub] [ozone] GeorgeJahad commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261831836
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -692,11 +692,23 @@ public void testInstallIncrementalSnapshotWithFailure() throws Exception {
// TODO: Add wait check here, to avoid flakiness.
// Verify the metrics
- DBCheckpointMetrics dbMetrics = leaderOM.getMetrics().
- getDBCheckpointMetrics();
- assertEquals(0, dbMetrics.getLastCheckpointStreamingNumSSTExcluded());
- assertTrue(dbMetrics.getNumIncrementalCheckpoints() >= 1);
- assertTrue(dbMetrics.getNumCheckpoints() >= 3);
+ GenericTestUtils.waitFor(() -> {
Review Comment:
It seems like this change is meant to address the "todo" comment listed above: https://github.com/apache/ozone/blob/2a14794d916e9e667a2ff073c8360db39c47f576/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java#L688-L693
If so, please remove that comment.
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261895603
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/util/ManagedObjectUtil.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.utils.db.managed.util;
+
+import org.awaitility.Awaitility;
+import org.awaitility.core.ConditionTimeoutException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.time.Duration;
+import java.time.Instant;
+
+
+/**
+ * Util classes for managed objects.
+ */
+public final class ManagedObjectUtil {
Review Comment:
I don't think it is make sense to create `ManagedObjectUtil` just for `waitForFileDelete`. Ideally it should be internal function to `ManagedRocksDB` because it is not generic enough that it will be applicable to any type of `ManagedObject` other than `ManagedRocksDB`.
If you really want this to be a util function, either move it to existing util class `ManagedRocksObjectUtils` or change the name to `ManagedRocksDBUtils`.
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261965233
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -692,11 +692,23 @@ public void testInstallIncrementalSnapshotWithFailure() throws Exception {
// TODO: Add wait check here, to avoid flakiness.
// Verify the metrics
- DBCheckpointMetrics dbMetrics = leaderOM.getMetrics().
- getDBCheckpointMetrics();
- assertEquals(0, dbMetrics.getLastCheckpointStreamingNumSSTExcluded());
- assertTrue(dbMetrics.getNumIncrementalCheckpoints() >= 1);
- assertTrue(dbMetrics.getNumCheckpoints() >= 3);
+ GenericTestUtils.waitFor(() -> {
Review Comment:
done
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261965412
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/util/ManagedObjectUtil.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.utils.db.managed.util;
+
+import org.awaitility.Awaitility;
+import org.awaitility.core.ConditionTimeoutException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.time.Duration;
+import java.time.Instant;
+
+
+/**
+ * Util classes for managed objects.
+ */
+public final class ManagedObjectUtil {
+
+ static final Logger LOG =
+ LoggerFactory.getLogger(ManagedObjectUtil.class);
+ private static final Duration POLL_DELAY_DURATION = Duration.ZERO;
+ private static final Duration POLL_INTERVAL_DURATION = Duration.ofMillis(100);
+ private static final Duration POLL_MAX_DURATION = Duration.ofSeconds(5);
Review Comment:
removed it.
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/util/ManagedObjectUtil.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.utils.db.managed.util;
+
+import org.awaitility.Awaitility;
+import org.awaitility.core.ConditionTimeoutException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.time.Duration;
+import java.time.Instant;
+
+
+/**
+ * Util classes for managed objects.
+ */
+public final class ManagedObjectUtil {
Review Comment:
done
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261964721
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -123,7 +133,7 @@ public BackgroundTaskResult call() throws Exception {
long snapshotLimit = snapshotLimitPerTask;
- while (iterator.hasNext() && snapshotLimit > 0) {
+ while (iterator.hasNext() && snapshotLimit > 0 && running.get()) {
Review Comment:
shutdown() will wait for the existing run method to run. It would be better if we break the loop & finish the run gracefully instead of killing the thread. Otherwise it could possibly lead to another set of problems.
--
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
[GitHub] [ozone] smengcl commented on pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #5035:
URL: https://github.com/apache/ozone/pull/5035#issuecomment-1629739566
Pls fix checkstyle:
https://github.com/swamirishi/ozone/actions/runs/5512689286/jobs/10049848161#step:6:520
```
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/RdbUtil.java
41: Unused import - org.awaitility.Awaitility.with.
```
--
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
[GitHub] [ozone] hemantk-12 commented on pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on PR #5035:
URL: https://github.com/apache/ozone/pull/5035#issuecomment-1629856309
> > excluded SST files list based on current state of the follower.
>
> There is some ambiguity here, so let me try to be precise.
>
> Both the leader and follower are running their own version of the sst filtering service simultaneously.
>
> The excluded list is only generated from the followers candidate directory, not from the followers active fs. (The candidate directory is a seperate directory where each tarball is untarred until the follower catches up.)
>
> Since the SFS is not deleting files from the candidate directory, the follower's SFS can't effect this part of the bootstrap process.
>
> So @hemantk-12 statement above is more precisely: _excluded SST files list based on current state of the **follower's candidate directory**_
I updated my previous comment as suggested by you.
Based on your comment _"excluded SST files list based on current state of the **follower's candidate directory**"_ , you are right that SFS should not interfere with bootstrapping because SFS only works on snapshot's DB dir not the candidate dir or even AFS dir.
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1262016533
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksObjectUtils.java:
##########
@@ -64,4 +73,46 @@ static String formatStackTrace(StackTraceElement[] elements) {
return HddsUtils.formatStackTrace(elements, 3);
}
+ /**
+ * Wait for file to be deleted.
+ * @param file File to be deleted.
+ * @param maxDuration poll max duration.
+ * @param interval poll interval.
+ * @param pollDelayDuration poll delay val.
+ * @return true if deleted.
+ */
+ public static void waitForFileDelete(File file, Duration maxDuration,
+ Duration interval,
+ Duration pollDelayDuration)
+ throws IOException {
+ Instant start = Instant.now();
+ try {
+ Awaitility.with().atMost(maxDuration)
+ .pollDelay(pollDelayDuration)
+ .pollInterval(interval)
+ .await()
+ .until(() -> !file.exists());
+ LOG.info("Waited for {} milliseconds for file {} deletion.",
+ Duration.between(start, Instant.now()).toMillis(),
+ file.getAbsoluteFile());
+ } catch (ConditionTimeoutException exception) {
+ LOG.info("File: {} didn't get deleted in {} secs.",
+ file.getAbsolutePath(), maxDuration.getSeconds());
+ throw new IOException(exception);
+ }
+ }
+
+ /**
+ * Wait for file to be deleted.
+ * @param file File to be deleted.
+ * @param maxDuration poll max duration.
+ * @throws IOException in case of failure.
+ */
+ public static void waitForFileDelete(File file, Duration maxDuration)
+ throws IOException {
+ waitForFileDelete(file, maxDuration, POLL_INTERVAL_DURATION,
+ POLL_DELAY_DURATION);
+ }
+
+
Review Comment:
Remove extra line.
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksDB.java:
##########
@@ -78,4 +86,21 @@ public static ManagedRocksDB open(
RocksDB.open(path, columnFamilyDescriptors, columnFamilyHandles)
);
}
+
+ /**
+ * Delete liveMetaDataFile from rocks db.
+ * @param fileToBeDeleted File to be deleted.
+ * @throws RocksDBException In the underlying db throws an exception.
+ * @throws IOException In the case file is not deleted.
+ */
+
+ public void deleteFile(LiveFileMetaData fileToBeDeleted)
+ throws RocksDBException, IOException {
+ String sstFileName = fileToBeDeleted.fileName();
+ this.get().deleteFile(sstFileName);
+ File file = new File(fileToBeDeleted.path(), fileToBeDeleted.fileName());
+ LOG.info("Deleting file {} from db: {}", file.getAbsolutePath(),
Review Comment:
Please remove this.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -123,7 +133,7 @@ public BackgroundTaskResult call() throws Exception {
long snapshotLimit = snapshotLimitPerTask;
- while (iterator.hasNext() && snapshotLimit > 0) {
+ while (iterator.hasNext() && snapshotLimit > 0 && running.get()) {
Review Comment:
I don't agree with it.
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksDB.java:
##########
@@ -78,4 +87,15 @@ public static ManagedRocksDB open(
RocksDB.open(path, columnFamilyDescriptors, columnFamilyHandles)
);
}
+
+ public void deleteFile(LiveFileMetaData fileToBeDeleted)
Review Comment:
Can you please amend the java doc comment that it synchronizes the `RocksDB#deleteFile()`? Function name says itself that it deleted dataFile from rocks db.
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1262835344
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -123,7 +133,7 @@ public BackgroundTaskResult call() throws Exception {
long snapshotLimit = snapshotLimitPerTask;
- while (iterator.hasNext() && snapshotLimit > 0) {
+ while (iterator.hasNext() && snapshotLimit > 0 && running.get()) {
Review Comment:
If it was me, I would have left it is as it was before because you can argue the same for other background services e.g. `KeyDeletingService`, `SnapshotDeletingService` and others.
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261745811
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3632,7 +3633,7 @@ TermIndex installCheckpoint(String leaderId, Path checkpointLocation,
keyManager.stop();
stopSecretManager();
stopTrashEmptier();
-
+ omSnapshotManager.getSnapshotCache().invalidateAll();
Review Comment:
Had a offline discussion and problem is that in case of reloading OmMetadataManager, previous [snapshotCache](https://github.com/apache/ozone/blob/5a6f08ce48eb7053894596a7bb2d3993072f8c8a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java#L116) object may/may not get GC'ed and new snapshotCache object get created. If previous snapshotCache is not GC'ed and has opened RocksDB instance, it will keep deleting file because of RocksDB async delete (delete command was executed by previous SSTFilteringService run). New cache will try to open RocksDB instance based on new manifest and will fail because file is missing.
Based on logs: On followed node,
1. File 000077.sst was deleted by SSTFilteringService at hemant_sst_filtering.log line 20972
1. File 000077.sst was copied from candidate dir to snapshot db dir at hemant_sst_filtering.log line 30871
1. Background services including SSTFilteringService get started at hemant_sst_filtering.log line 31062
1. Later SSTFilteringService run fails because file doesn't exist. at hemant_sst_filtering.log line 31111-31112
Which fails the overall `testInstallIncrementalSnapshot`.
Attached the log file for details: [testInstallIncrementalSnapshot.log](https://github.com/apache/ozone/files/12033026/testInstallIncrementalSnapshot.log)
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261745811
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3632,7 +3633,7 @@ TermIndex installCheckpoint(String leaderId, Path checkpointLocation,
keyManager.stop();
stopSecretManager();
stopTrashEmptier();
-
+ omSnapshotManager.getSnapshotCache().invalidateAll();
Review Comment:
Had a offline discussion and problem is that in case of reloading OmMetadataManager, previous [snapshotCache](https://github.com/apache/ozone/blob/5a6f08ce48eb7053894596a7bb2d3993072f8c8a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java#L116) object may/may not get GC'ed and new snapshotCache object get created. If previous snapshotCache is not GC'ed and has opened RocksDB instance, it will keep deleting file because of RocksDB async delete (delete command was executed by previous SSTFilteringService run). New cache will try to open RocksDB instance based on new manifest and will fail because file is missing.
Based on logs: On followed node,
1. File 000077.sst was deleted by SSTFilteringService at hemant_sst_filtering.log line 20972.
1. File 000077.sst was copied from candidate dir to snapshot db dir at hemant_sst_filtering.log line 30871
1. Background services including SSTFilteringService get started at hemant_sst_filtering.log line 31062
1. Later SSTFilteringService run fails because file doesn't exist. at hemant_sst_filtering.log line 31111-31112.
Which was the overall `testInstallIncrementalSnapshot`.
Attached the log file for details: [testInstallIncrementalSnapshot.log](https://github.com/apache/ozone/files/12033026/testInstallIncrementalSnapshot.log)
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261801745
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3632,7 +3633,7 @@ TermIndex installCheckpoint(String leaderId, Path checkpointLocation,
keyManager.stop();
stopSecretManager();
stopTrashEmptier();
-
+ omSnapshotManager.getSnapshotCache().invalidateAll();
Review Comment:
> Is that a correct understanding of your theory?
Yes, that's the correct. I don't have logs for that but Swami might have because he enabled native RocksDB logs. @swamirishi, please add the logs either here or in description if you still have.
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261963799
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3632,7 +3633,7 @@ TermIndex installCheckpoint(String leaderId, Path checkpointLocation,
keyManager.stop();
stopSecretManager();
stopTrashEmptier();
-
+ omSnapshotManager.getSnapshotCache().invalidateAll();
Review Comment:
I have attached the log in the jira.
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1262587881
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksObjectUtils.java:
##########
@@ -64,4 +73,46 @@ static String formatStackTrace(StackTraceElement[] elements) {
return HddsUtils.formatStackTrace(elements, 3);
}
+ /**
+ * Wait for file to be deleted.
+ * @param file File to be deleted.
+ * @param maxDuration poll max duration.
+ * @param interval poll interval.
+ * @param pollDelayDuration poll delay val.
+ * @return true if deleted.
+ */
+ public static void waitForFileDelete(File file, Duration maxDuration,
+ Duration interval,
+ Duration pollDelayDuration)
+ throws IOException {
+ Instant start = Instant.now();
+ try {
+ Awaitility.with().atMost(maxDuration)
+ .pollDelay(pollDelayDuration)
+ .pollInterval(interval)
+ .await()
+ .until(() -> !file.exists());
+ LOG.info("Waited for {} milliseconds for file {} deletion.",
+ Duration.between(start, Instant.now()).toMillis(),
+ file.getAbsoluteFile());
+ } catch (ConditionTimeoutException exception) {
+ LOG.info("File: {} didn't get deleted in {} secs.",
+ file.getAbsolutePath(), maxDuration.getSeconds());
+ throw new IOException(exception);
+ }
+ }
+
+ /**
+ * Wait for file to be deleted.
+ * @param file File to be deleted.
+ * @param maxDuration poll max duration.
+ * @throws IOException in case of failure.
+ */
+ public static void waitForFileDelete(File file, Duration maxDuration)
+ throws IOException {
+ waitForFileDelete(file, maxDuration, POLL_INTERVAL_DURATION,
+ POLL_DELAY_DURATION);
+ }
+
+
Review Comment:
done
--
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
[GitHub] [ozone] GeorgeJahad commented on pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on PR #5035:
URL: https://github.com/apache/ozone/pull/5035#issuecomment-1629464973
Hey @swamirishi: I'm trying to understand what the actual problem is from the stack trace. This is the error I see:
* IO error: No such file or directory: While open a file for random read: /Users/sbalachandran/Documents/ozone/hadoop-ozone/integration-test/target/test-dir/MiniOzoneClusterImpl-2ff22622-5883-4e2b-8090-8276162d8e5a/omNode-3/db.snapshots/checkpointState/om.db-aa3c01d4-a80a-47b1-8cde-979b62f0390d/000076.ldb: No such file or directory in file /Users/sbalachandran/Documents/ozone/hadoop-ozone/integration-test/target/test-dir/MiniOzoneClusterImpl-2ff22622-5883-4e2b-8090-8276162d8e5a/omNode-3/db.snapshots/checkpointState/om.db-aa3c01d4-a80a-47b1-8cde-979b62f0390d/MANIFEST-000005 *
I'm not sure what that means. Is it saying that the manifest file is missing? or that the ldb file is missing? In either case, the sst filtering service doesn't delete either of those types of files, does it? So why do we think the sst filtering service needs to be changed?
--
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
[GitHub] [ozone] GeorgeJahad commented on pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on PR #5035:
URL: https://github.com/apache/ozone/pull/5035#issuecomment-1633156350
> I just realized this kind of a problem can only occur if the database is open
Yes but on the leader the database may be open, and if the delete is not synchronous, it won't happen while the lock is held here: https://github.com/apache/ozone/blob/e950ecee512fd87a9c20c233ba6ed224acd2c6e4/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java#L164-L168
That will invalidate the tarball creation.
--
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
[GitHub] [ozone] GeorgeJahad commented on pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on PR #5035:
URL: https://github.com/apache/ozone/pull/5035#issuecomment-1632819050
@swamirishi
As I said above, I'm not sure the solution to the delete problem is complete. Let me ask a few questions:
1. Is the problem that this method returns before the sst file is deleted?: https://github.com/apache/ozone/blob/4aa49370b47397ceee3e7f29069195923f5b8a1e/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java#L993
2. Does the delete rename the sst file to the ldb file, and then later deletes the ldb file?
3. If so, then don't we also need to wait for the non-existance of the ldb file?
4. Does the delete do anything else? How do we know when it is complete?
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1262578423
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksDB.java:
##########
@@ -78,4 +86,21 @@ public static ManagedRocksDB open(
RocksDB.open(path, columnFamilyDescriptors, columnFamilyHandles)
);
}
+
+ /**
+ * Delete liveMetaDataFile from rocks db.
+ * @param fileToBeDeleted File to be deleted.
+ * @throws RocksDBException In the underlying db throws an exception.
+ * @throws IOException In the case file is not deleted.
+ */
+
+ public void deleteFile(LiveFileMetaData fileToBeDeleted)
+ throws RocksDBException, IOException {
+ String sstFileName = fileToBeDeleted.fileName();
+ this.get().deleteFile(sstFileName);
+ File file = new File(fileToBeDeleted.path(), fileToBeDeleted.fileName());
+ LOG.info("Deleting file {} from db: {}", file.getAbsolutePath(),
Review Comment:
I beleive we should have a log here instead of outside, this is where delete is actually happening.
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261954779
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java:
##########
@@ -34,6 +34,7 @@
import org.apache.hadoop.hdds.utils.db.managed.ManagedTransactionLogIterator;
import org.apache.hadoop.hdds.utils.db.managed.ManagedWriteBatch;
import org.apache.hadoop.hdds.utils.db.managed.ManagedWriteOptions;
+import org.apache.ozone.rocksdb.util.RdbUtil;
Review Comment:
done
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. [Snapshot] SstFilteringService should be stopped before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1262928354
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -123,7 +133,7 @@ public BackgroundTaskResult call() throws Exception {
long snapshotLimit = snapshotLimitPerTask;
- while (iterator.hasNext() && snapshotLimit > 0) {
+ while (iterator.hasNext() && snapshotLimit > 0 && running.get()) {
Review Comment:
yeah sure thanks.
--
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
[GitHub] [ozone] GeorgeJahad commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261781961
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3632,7 +3633,7 @@ TermIndex installCheckpoint(String leaderId, Path checkpointLocation,
keyManager.stop();
stopSecretManager();
stopTrashEmptier();
-
+ omSnapshotManager.getSnapshotCache().invalidateAll();
Review Comment:
@hemantk-12 So this all sounds good to me, but one thing that isn't clear. It sounds, from the explanation above, that we believe, somewhere between steps 2 and 4, the old cache entry deleted 77.sst a second time.
Is that a correct understanding of your theory?
Is there evidence of that second delete in the logs?
--
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
[GitHub] [ozone] GeorgeJahad commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1258975478
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -231,4 +241,10 @@ public AtomicLong getSnapshotFilteredCount() {
public BootstrapStateHandler.Lock getBootstrapStateLock() {
return lock;
}
+
+ @Override
Review Comment:
Is this needed? As I mentioned above, the keymanager is already stopping the service.
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261745811
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3632,7 +3633,7 @@ TermIndex installCheckpoint(String leaderId, Path checkpointLocation,
keyManager.stop();
stopSecretManager();
stopTrashEmptier();
-
+ omSnapshotManager.getSnapshotCache().invalidateAll();
Review Comment:
Had a offline discussion and problem is that in case of reloading OmMetadataManager, previous [snapshotCache](https://github.com/apache/ozone/blob/5a6f08ce48eb7053894596a7bb2d3993072f8c8a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java#L116) object may/may not get GC'ed and new snapshotCache object get created. If previous snapshotCache is not GC'ed and has opened RocksDB instance, it will keep deleting file because of RocksDB async delete (delete command was executed by previous SSTFilteringService run). New cache will try to open RocksDB instance based on new manifest and will fail because file is missing.
Based on logs: On followed node,
1. File 000077.sst was deleted by SSTFilteringService attestInstallIncrementalSnapshot.log line 20972
1. File 000077.sst was copied from candidate dir to snapshot db dir at testInstallIncrementalSnapshot.log line 30871
1. Background services including SSTFilteringService get started at testInstallIncrementalSnapshot.log line 31062
1. Later SSTFilteringService run fails because file doesn't exist. at testInstallIncrementalSnapshot.log line 31111-31112
Which fails the overall `testInstallIncrementalSnapshot`.
Attached the log file for details: [testInstallIncrementalSnapshot.log](https://github.com/apache/ozone/files/12033026/testInstallIncrementalSnapshot.log)
So based on above explanation, we need to invalidate the case in both the cases when OmMetadataManager is stopped or not stopped.
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261745811
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3632,7 +3633,7 @@ TermIndex installCheckpoint(String leaderId, Path checkpointLocation,
keyManager.stop();
stopSecretManager();
stopTrashEmptier();
-
+ omSnapshotManager.getSnapshotCache().invalidateAll();
Review Comment:
Had a offline discussion and problem is that in case of reloading OmMetadataManager, previous [snapshotCache](https://github.com/apache/ozone/blob/5a6f08ce48eb7053894596a7bb2d3993072f8c8a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java#L116) object may/may not get GC'ed and new snapshotCache object get created. If previous snapshotCache is not GC'ed and has opened RocksDB instance, it will keep deleting file because of RocksDB async delete (delete command was executed by previous SSTFilteringService run). New cache will try to open RocksDB instance based on new manifest and will fail because file is missing.
Based on logs: On followed node,
1. File 000077.sst was deleted by SSTFilteringService at hemant_sst_filtering.log line 20972.=
1. File 000077.sst was copied from candidate dir to snapshot db dir at hemant_sst_filtering.log line 30871
1. Background services including SSTFilteringService get started at hemant_sst_filtering.log line 31062
1. Later SSTFilteringService run fails because file doesn't exist. at hemant_sst_filtering.log line 31111-31112
Which fails the overall `testInstallIncrementalSnapshot`.
Attached the log file for details: [testInstallIncrementalSnapshot.log](https://github.com/apache/ozone/files/12033026/testInstallIncrementalSnapshot.log)
--
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
[GitHub] [ozone] GeorgeJahad commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1260305869
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -231,4 +241,10 @@ public AtomicLong getSnapshotFilteredCount() {
public BootstrapStateHandler.Lock getBootstrapStateLock() {
return lock;
}
+
+ @Override
+ public void shutdown() {
Review Comment:
> We need the sstFiltering service to be shutdown & all rocksdb
> instances to be closed before creating the hardlinks b/w the
> checkpoint directory & the actual follower meta directory.
This doesn't sound correct to me. I'm not sure what "the actual follower meta directory" is, but the hard links are created where the tarball is untarred, (in the candidate directory.) The SFS has no impact on it.
Then tarball data gets linked over to the actual active fs in installCheckpoint() here: https://github.com/apache/ozone/blob/ffd84e747dc43d08a23269a8f411d52fef41c432/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L3696
That method also stops the keymanager here: https://github.com/apache/ozone/blob/ffd84e747dc43d08a23269a8f411d52fef41c432/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L3640
That is why these extra start/shutdown methods aren't needed.
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261965555
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksDB.java:
##########
@@ -78,4 +87,15 @@ public static ManagedRocksDB open(
RocksDB.open(path, columnFamilyDescriptors, columnFamilyHandles)
);
}
+
+ public void deleteFile(LiveFileMetaData fileToBeDeleted)
Review Comment:
I need the db path as well.
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261966509
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksDB.java:
##########
@@ -78,4 +87,15 @@ public static ManagedRocksDB open(
RocksDB.open(path, columnFamilyDescriptors, columnFamilyHandles)
);
}
+
+ public void deleteFile(LiveFileMetaData fileToBeDeleted)
Review Comment:
it is better we pass this.
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. [Snapshot] SstFilteringService should be stopped before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1262839148
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -123,7 +133,7 @@ public BackgroundTaskResult call() throws Exception {
long snapshotLimit = snapshotLimitPerTask;
- while (iterator.hasNext() && snapshotLimit > 0) {
+ while (iterator.hasNext() && snapshotLimit > 0 && running.get()) {
Review Comment:
those services are not running on follower. As far as in understand we are saving on a many redundant operations.
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1258629967
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java:
##########
@@ -979,9 +982,17 @@ public void deleteFilesNotMatchingPrefix(
+ " {} from db: {}", sstFileName,
liveFileMetaData.columnFamilyName(), db.get().getName());
db.get().deleteFile(sstFileName);
+ filesToBeDeleted.add(new File(liveFileMetaData.path(),
+ liveFileMetaData.fileName()));
}
}
}
+
+ Iterator<File> files = filesToBeDeleted.iterator();
+ while (files.hasNext()) {
Review Comment:
I see.
If delete to be synchronous, I think `waitForFileDelete` should be internal to `RocksDatabase` or should be moved to [RocksDBUtils`](https://github.com/apache/ozone/blob/master/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/RocksDBUtils.java) or [RdbUtil](https://github.com/apache/ozone/blob/master/hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/RdbUtil.java). As the name says `RDBCheckpointUtils` is for Checkpoint and not for a general RDB util.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java:
##########
@@ -979,9 +982,17 @@ public void deleteFilesNotMatchingPrefix(
+ " {} from db: {}", sstFileName,
liveFileMetaData.columnFamilyName(), db.get().getName());
db.get().deleteFile(sstFileName);
+ filesToBeDeleted.add(new File(liveFileMetaData.path(),
+ liveFileMetaData.fileName()));
}
}
}
+
+ Iterator<File> files = filesToBeDeleted.iterator();
+ while (files.hasNext()) {
Review Comment:
I see.
If delete to be synchronous, I think `waitForFileDelete` should be internal to `RocksDatabase` or should be moved to [RocksDBUtils](https://github.com/apache/ozone/blob/master/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/RocksDBUtils.java) or [RdbUtil](https://github.com/apache/ozone/blob/master/hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/RdbUtil.java). As the name says `RDBCheckpointUtils` is for Checkpoint and not for a general RDB util.
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1258574640
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java:
##########
@@ -979,9 +982,17 @@ public void deleteFilesNotMatchingPrefix(
+ " {} from db: {}", sstFileName,
liveFileMetaData.columnFamilyName(), db.get().getName());
db.get().deleteFile(sstFileName);
+ filesToBeDeleted.add(new File(liveFileMetaData.path(),
+ liveFileMetaData.fileName()));
}
}
}
+
+ Iterator<File> files = filesToBeDeleted.iterator();
+ while (files.hasNext()) {
Review Comment:
Why do we need to wait for file deletion? Once manifest is updated, it doesn't matter if file exist or got deleted.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3577,18 +3578,24 @@ public List<OzoneAcl> getAcl(OzoneObj obj) throws IOException {
* @return If checkpoint is installed successfully, return the
* corresponding termIndex. Otherwise, return null.
*/
- public TermIndex installSnapshotFromLeader(String leaderId) {
+ public synchronized TermIndex installSnapshotFromLeader(String leaderId) {
if (omRatisSnapshotProvider == null) {
LOG.error("OM Snapshot Provider is not configured as there are no peer " +
"nodes.");
return null;
}
+ java.util.Optional<SstFilteringService> sstFilteringService =
+ java.util.Optional.ofNullable(
+ keyManager.getSnapshotSstFilteringService());
DBCheckpoint omDBCheckpoint;
+ sstFilteringService.ifPresent(BackgroundService::shutdown);
Review Comment:
I don't think it is a good idea to shutdown and start the service like this. I think we should use lock as it is done in other places like following:
* https://github.com/apache/ozone/blob/0862a76ef178f427a272c335374760c87c885119/hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java#L1167
* https://github.com/apache/ozone/blob/1ae0401b1ac662c021b37b68ef46f4b9a0909890/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/DBCheckpointServlet.java#L174
@GeorgeJahad can add or correct me if we can't use that.
I see we have a lock: https://github.com/apache/ozone/blob/e950ecee512fd87a9c20c233ba6ed224acd2c6e4/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java#L164
We may need to place this lock before starting the SST file cleanup.
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1258598867
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java:
##########
@@ -979,9 +982,17 @@ public void deleteFilesNotMatchingPrefix(
+ " {} from db: {}", sstFileName,
liveFileMetaData.columnFamilyName(), db.get().getName());
db.get().deleteFile(sstFileName);
+ filesToBeDeleted.add(new File(liveFileMetaData.path(),
+ liveFileMetaData.fileName()));
}
}
}
+
+ Iterator<File> files = filesToBeDeleted.iterator();
+ while (files.hasNext()) {
Review Comment:
The deletes are asynchronous so we need the deletes to be synchronous here.
--
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
[GitHub] [ozone] swamirishi commented on pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #5035:
URL: https://github.com/apache/ozone/pull/5035#issuecomment-1629504655
> Hey @swamirishi: I'm trying to understand what the actual problem is from the stack trace. This is the error I see:
>
> * IO error: No such file or directory: While open a file for random read: /Users/sbalachandran/Documents/ozone/hadoop-ozone/integration-test/target/test-dir/MiniOzoneClusterImpl-2ff22622-5883-4e2b-8090-8276162d8e5a/omNode-3/db.snapshots/checkpointState/om.db-aa3c01d4-a80a-47b1-8cde-979b62f0390d/000076.ldb: No such file or directory in file /Users/sbalachandran/Documents/ozone/hadoop-ozone/integration-test/target/test-dir/MiniOzoneClusterImpl-2ff22622-5883-4e2b-8090-8276162d8e5a/omNode-3/db.snapshots/checkpointState/om.db-aa3c01d4-a80a-47b1-8cde-979b62f0390d/MANIFEST-000005 *
>
> I'm not sure what that means. Is it saying that the manifest file is missing? or that the ldb file is missing? In either case, the sst filtering service doesn't delete either of those types of files, does it? So why do we think the sst filtering service needs to be changed?
SST Filtering service deletes sst files through the rocksdb api. The incremental snapshot figures out the exclude list by walking through the entire OM metadata & getting the list of sst files. Manifest files are always taken from the leader it is only the sst files which are incremental. While the copy is happening some of the sst files acutally might be deleted by sst filtering service. Since the manifest file is coming from the leader the db might get corrupted since the list of sst files in manifest file & actual filesystems don't match. For this we need to stop the sst filtering service before we start the copy. @hemantk-12 BTW the keyManager service is shutdown before installation the leader snapshot on the follower, so all we are doing here is shutting down the sst filtering service before the download to ensure, we don't corrupt the db.
--
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
[GitHub] [ozone] swamirishi commented on pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #5035:
URL: https://github.com/apache/ozone/pull/5035#issuecomment-1629931455
> Just a side note. As I was reviewing this PR, I realized we can have other potential problem areas here. When follower tries to catch up with the leader, it should start with initial state as absolutely empty. That is because, we can not assume that sst files between leader and follower will have the same content. RocksDB instances and the content of the rocksDB directory on the OM nodes are only logically equal but physically they could be spread over completely different sst files. cc : @GeorgeJahad @smengcl @hemantk-12
This sst file walk happens on the candidate directory we exclude the list of sst files already present. There is also a race condition happening when multiple requests downloading & installing the snapshot from the leader comes together. The problem with multiple request is one request might be copying data from the same directory checkpoint & the request might be actually doing the ls. So if one request finishes the installation & starts the sst filtering service, it can so happen we might end up deleting files which have not been copied. But the manifest files are getting updated. Thus there is also a need to make this function syunchronized to ensure there is only one thread downloading & installing the snapshot. It will be eventually consistent.
--
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
[GitHub] [ozone] swamirishi commented on pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #5035:
URL: https://github.com/apache/ozone/pull/5035#issuecomment-1633092677
> > The rocksdb delete api is asynchronous. The files to be deleted are added to a scheduler.
>
> This makes sense; that means there really is a bug in my locking code, because I was assuming the delete api was synchronous. Thanks for figuring it out!
>
> However, now I see that the fix you created to make it synchronous was removed in this commit: [2a14794](https://github.com/apache/ozone/commit/2a14794d916e9e667a2ff073c8360db39c47f576)
>
> Why was it removed?
I just realized this kind of a problem can only occur if the database is open, & shouldn't happen if the database is closed. By invalidating the whole cache I am avoiding this which would close all open rocksdb instances.
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1260249364
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -231,4 +241,10 @@ public AtomicLong getSnapshotFilteredCount() {
public BootstrapStateHandler.Lock getBootstrapStateLock() {
return lock;
}
+
+ @Override
+ public void shutdown() {
Review Comment:
We need the sstFiltering service to be shutdown & all rocksdb instances to be closed before creating the hardlinks b/w the checkpoint directory & the actual follower meta directory. So yeah because of that we need to shutdown. Since we are going to get all the sst files from the leader anyways & going to restart the service anyhow we should start everything cleanslate. There is no point in running the loop till the end and shutting it down before will be more optmial
--
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
[GitHub] [ozone] GeorgeJahad commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1259982532
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -231,4 +241,10 @@ public AtomicLong getSnapshotFilteredCount() {
public BootstrapStateHandler.Lock getBootstrapStateLock() {
return lock;
}
+
+ @Override
+ public void shutdown() {
Review Comment:
Is this needed? As I mentioned above, the keymanager is already stopping the service.
--
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
[GitHub] [ozone] sadanand48 commented on pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on PR #5035:
URL: https://github.com/apache/ozone/pull/5035#issuecomment-1631265392
IIRC, this issue occurs when transferring of checkpoint b/w leader to follower, however SSTFilteringService only operates on checkpoints of Ozone 'Snapshots' that are actually persisted to the SnapshotInfoTable, Does calling an installSnapshot() create an OM Snapshot on the leader? If not then this should not be an issue.
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1262594359
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -80,6 +80,7 @@
import org.apache.hadoop.hdds.scm.ScmInfo;
import org.apache.hadoop.hdds.scm.client.HddsClientUtils;
import org.apache.hadoop.hdds.server.OzoneAdmins;
+import org.apache.hadoop.hdds.utils.BackgroundService;
Review Comment:
done
--
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
[GitHub] [ozone] swamirishi commented on pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #5035:
URL: https://github.com/apache/ozone/pull/5035#issuecomment-1632965951
> @swamirishi As I said above, I'm not sure the solution to the delete problem is complete. Let me ask a few questions:
>
> 1. Is the problem that this method returns before the sst file is deleted?: https://github.com/apache/ozone/blob/4aa49370b47397ceee3e7f29069195923f5b8a1e/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java#L993
The rocksdb delete api is asynchronous. The files to be deleted are added to a scheduler.
> 2. Does the delete rename the sst file to the ldb file, and then later deletes the ldb file?
No, rocksdb supports both sst files & ldb files(To support older level db migrated data). The logging in rocksdb just shows it to be an ldb file. But in actuality it is just an sst file.
> 3. If so, then don't we also need to wait for the non-existance of the ldb file?
ldb files don't exist in ozone rocksdb
> 4. Does the delete do anything else? How do we know when it is complete?
Delete just deletes data and updates the manifest file.
The actual problem is the snapshot cache & sst filtering service acting up together. So when we actually try to install a om metadata snapshot from leader we actually copy individual files from checkpoint directory to ozone metadata location. So if we have an open rocksdb instance for that particular db while we are copying. The delete scheduler could kick in and delete the sst file, which wouldn't happen if the rocksdb is closed. This problem becomes more prominent in https://github.com/apache/ozone/pull/5008 when sst filtering service also uses snapshot cache & can be reproduced more easily. So if we have an open instance of rocksdb in the snapshot cache we cannot really determine when the delete is going to happen, so we need to stop the sst filtering service before installation (which is already being done) and also invalidate the snapshot cache. We also should have only one instance install running at any given time thus I have made changes to also synchronize the particular
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1258660391
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3577,18 +3578,24 @@ public List<OzoneAcl> getAcl(OzoneObj obj) throws IOException {
* @return If checkpoint is installed successfully, return the
* corresponding termIndex. Otherwise, return null.
*/
- public TermIndex installSnapshotFromLeader(String leaderId) {
+ public synchronized TermIndex installSnapshotFromLeader(String leaderId) {
if (omRatisSnapshotProvider == null) {
LOG.error("OM Snapshot Provider is not configured as there are no peer " +
"nodes.");
return null;
}
+ java.util.Optional<SstFilteringService> sstFilteringService =
+ java.util.Optional.ofNullable(
+ keyManager.getSnapshotSstFilteringService());
DBCheckpoint omDBCheckpoint;
+ sstFilteringService.ifPresent(BackgroundService::shutdown);
Review Comment:
Just curious how other clean up and deletion services are working and not interfering with it. And it is not done in the same way.
--
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
[GitHub] [ozone] hemantk-12 commented on pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on PR #5035:
URL: https://github.com/apache/ozone/pull/5035#issuecomment-1629587027
> > Hey @swamirishi: I'm trying to understand what the actual problem is from the stack trace. This is the error I see:
> >
> > * IO error: No such file or directory: While open a file for random read: /Users/sbalachandran/Documents/ozone/hadoop-ozone/integration-test/target/test-dir/MiniOzoneClusterImpl-2ff22622-5883-4e2b-8090-8276162d8e5a/omNode-3/db.snapshots/checkpointState/om.db-aa3c01d4-a80a-47b1-8cde-979b62f0390d/000076.ldb: No such file or directory in file /Users/sbalachandran/Documents/ozone/hadoop-ozone/integration-test/target/test-dir/MiniOzoneClusterImpl-2ff22622-5883-4e2b-8090-8276162d8e5a/omNode-3/db.snapshots/checkpointState/om.db-aa3c01d4-a80a-47b1-8cde-979b62f0390d/MANIFEST-000005 *
> >
> > I'm not sure what that means. Is it saying that the manifest file is missing? or that the ldb file is missing? In either case, the sst filtering service doesn't delete either of those types of files, does it? So why do we think the sst filtering service needs to be changed?
> SST Filtering service deletes sst files through the rocksdb api. The incremental snapshot figures out the exclude list by walking through the entire OM metadata & getting the list of sst files. Manifest files are always taken from the leader it is only the sst files which are incremental. While the copy is happening some of the sst files acutally might be deleted by sst filtering service. Since the manifest file is coming from the leader the db might get corrupted since the list of sst files in manifest file & actual filesystems don't match. For this we need to stop the sst filtering service before we start the copy. @hemantk-12 BTW the keyManager service is shutdown before installation the leader snapshot on the follower, so all we are doing here is shutting down the sst filtering service before the download to ensure, we don't corrupt the db.
> @GeorgeJahad the ldb file is actually the same sst file that got deleted by the sst filtering service.
@swamirishi Can you please update the PR description what you explained here? In current description, it is not clear what race condition you are trying and creating confusion.
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1258594365
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3577,18 +3578,24 @@ public List<OzoneAcl> getAcl(OzoneObj obj) throws IOException {
* @return If checkpoint is installed successfully, return the
* corresponding termIndex. Otherwise, return null.
*/
- public TermIndex installSnapshotFromLeader(String leaderId) {
+ public synchronized TermIndex installSnapshotFromLeader(String leaderId) {
if (omRatisSnapshotProvider == null) {
LOG.error("OM Snapshot Provider is not configured as there are no peer " +
"nodes.");
return null;
}
+ java.util.Optional<SstFilteringService> sstFilteringService =
+ java.util.Optional.ofNullable(
+ keyManager.getSnapshotSstFilteringService());
DBCheckpoint omDBCheckpoint;
+ sstFilteringService.ifPresent(BackgroundService::shutdown);
Review Comment:
We are shutting down the service anyway before installation. Locking doesn't make sense.
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1258811904
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3577,18 +3578,24 @@ public List<OzoneAcl> getAcl(OzoneObj obj) throws IOException {
* @return If checkpoint is installed successfully, return the
* corresponding termIndex. Otherwise, return null.
*/
- public TermIndex installSnapshotFromLeader(String leaderId) {
+ public synchronized TermIndex installSnapshotFromLeader(String leaderId) {
if (omRatisSnapshotProvider == null) {
LOG.error("OM Snapshot Provider is not configured as there are no peer " +
"nodes.");
return null;
}
+ java.util.Optional<SstFilteringService> sstFilteringService =
+ java.util.Optional.ofNullable(
+ keyManager.getSnapshotSstFilteringService());
DBCheckpoint omDBCheckpoint;
+ sstFilteringService.ifPresent(BackgroundService::shutdown);
Review Comment:
Had an offline discussion with Swami and Prashant. Race condition Swami is trying to solve here is when follower requests leader for tarball (incremental diff) with excluded SST files list based on current state of the follower, leader will only send the diff SST files and new manifest file. In meanwhile SST Filtering service might delete irrelevant SST files. Follower loads DB based on new manifest from leader and when it does that some files might not existed on follower because SST Filtering service has removed them in between fetching and loading. This leaves DB (manifest and data) into inconsistent state. Because manifest contains SST files which actual doesn't exist on follower.
I hope this help @GeorgeJahad.
@swamirishi added some details here: https://github.com/apache/ozone/pull/5035/#issuecomment-1629504655
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3577,18 +3578,24 @@ public List<OzoneAcl> getAcl(OzoneObj obj) throws IOException {
* @return If checkpoint is installed successfully, return the
* corresponding termIndex. Otherwise, return null.
*/
- public TermIndex installSnapshotFromLeader(String leaderId) {
+ public synchronized TermIndex installSnapshotFromLeader(String leaderId) {
if (omRatisSnapshotProvider == null) {
LOG.error("OM Snapshot Provider is not configured as there are no peer " +
"nodes.");
return null;
}
+ java.util.Optional<SstFilteringService> sstFilteringService =
+ java.util.Optional.ofNullable(
+ keyManager.getSnapshotSstFilteringService());
DBCheckpoint omDBCheckpoint;
+ sstFilteringService.ifPresent(BackgroundService::shutdown);
Review Comment:
Had an offline discussion with Swami and Prashant. Race condition Swami is trying to solve here is when follower requests leader for tarball (incremental diff) with excluded SST files list based on current state of the follower, leader will only send the diff SST files and new manifest file. In meanwhile SST Filtering service might delete irrelevant SST files. Follower loads DB based on new manifest from leader and when it does that some files might not existed on follower because SST Filtering service has removed them in between fetching and loading. This leaves DB (manifest and data) into inconsistent state. Because manifest contains SST files which actual doesn't exist on follower.
I hope this help @GeorgeJahad.
@swamirishi added more details here: https://github.com/apache/ozone/pull/5035/#issuecomment-1629504655
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1258816893
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -231,4 +241,10 @@ public AtomicLong getSnapshotFilteredCount() {
public BootstrapStateHandler.Lock getBootstrapStateLock() {
return lock;
}
+
+ @Override
Review Comment:
nit: stop is in opposite order of start.
```suggestion
@Override
public void shutdown() {
super.shutdown();
running.set(false);
}
```
--
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
[GitHub] [ozone] smengcl commented on a diff in pull request #5035: HDDS-8987. [Snapshot] Make RocksDB delete sstFile api synchronous. Invalidate Snapshot cache before copying incremental snapshot
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1267459843
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksDB.java:
##########
@@ -78,4 +86,20 @@ public static ManagedRocksDB open(
RocksDB.open(path, columnFamilyDescriptors, columnFamilyHandles)
);
}
+
+ /**
+ * Delete liveMetaDataFile from rocks db using RocksDB#deleteFile Api.
+ * This function makes the RocksDB#deleteFile Api synchronized by waiting
+ * for the deletes to happen.
+ * @param fileToBeDeleted File to be deleted.
+ * @throws RocksDBException In the underlying db throws an exception.
+ * @throws IOException In the case file is not deleted.
+ */
+ public void deleteFile(LiveFileMetaData fileToBeDeleted)
+ throws RocksDBException, IOException {
+ String sstFileName = fileToBeDeleted.fileName();
+ this.get().deleteFile(sstFileName);
+ File file = new File(fileToBeDeleted.path(), fileToBeDeleted.fileName());
+ ManagedRocksObjectUtils.waitForFileDelete(file, Duration.ofSeconds(60));
Review Comment:
Will the `IOException` thrown inside when exceeding 60s crash OM?
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261745811
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3632,7 +3633,7 @@ TermIndex installCheckpoint(String leaderId, Path checkpointLocation,
keyManager.stop();
stopSecretManager();
stopTrashEmptier();
-
+ omSnapshotManager.getSnapshotCache().invalidateAll();
Review Comment:
Had a offline discussion and problem is that in case of reloading OmMetadataManager, previous [snapshotCache](https://github.com/apache/ozone/blob/5a6f08ce48eb7053894596a7bb2d3993072f8c8a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java#L116) object may/may not get GC'ed and new snapshotCache object get created. If previous snapshotCache is not GC'ed and has opened RocksDB instance, it will keep deleting file because of RocksDB async delete (delete command was executed by previous SSTFilteringService run). New cache will try to open RocksDB instance based on new manifest and will fail because file is missing.
Based on logs: On followed node,
1. File 000077.sst was deleted by SSTFilteringService attestInstallIncrementalSnapshot.log line 20972
1. File 000077.sst was copied from candidate dir to snapshot db dir at testInstallIncrementalSnapshot.log line 30871
1. Background services including SSTFilteringService get started at testInstallIncrementalSnapshot.log line 31062
1. Later SSTFilteringService run fails because file doesn't exist. at testInstallIncrementalSnapshot.log line 31111-31112
Which fails the overall `testInstallIncrementalSnapshot`.
Attached the log file for details: [testInstallIncrementalSnapshot.log](https://github.com/apache/ozone/files/12033026/testInstallIncrementalSnapshot.log)
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1262578670
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/util/package-info.java:
##########
@@ -0,0 +1,22 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+/**
Review Comment:
removed the file
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1262589682
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -123,7 +133,7 @@ public BackgroundTaskResult call() throws Exception {
long snapshotLimit = snapshotLimitPerTask;
- while (iterator.hasNext() && snapshotLimit > 0) {
+ while (iterator.hasNext() && snapshotLimit > 0 && running.get()) {
Review Comment:
What do you propose here? I believe we dont have to run the whole loop for no reason.
--
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
[GitHub] [ozone] GeorgeJahad commented on pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on PR #5035:
URL: https://github.com/apache/ozone/pull/5035#issuecomment-1629805028
This lock was meant to handle the problem that we are running into: https://github.com/apache/ozone/blob/e950ecee512fd87a9c20c233ba6ed224acd2c6e4/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java#L164
But it sounds like you have discovered that it is insufficient, because this file delete call is not synchronous: https://github.com/apache/ozone/blob/6da1b13e6fc760a4b1be349c44cb5943a8fa5cc7/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java#L985 Because the file may not be completely deleted when that call returns. Is that correct?
If so, we need to understand exactly when it is safe to make a copy of the manifest and other files. How do we know that it is sufficient just to check for the non existence of the sst file, as is done here: https://github.com/apache/ozone/blob/6da1b13e6fc760a4b1be349c44cb5943a8fa5cc7/hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/RdbUtil.java#L92
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1258599319
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java:
##########
@@ -979,9 +982,17 @@ public void deleteFilesNotMatchingPrefix(
+ " {} from db: {}", sstFileName,
liveFileMetaData.columnFamilyName(), db.get().getName());
db.get().deleteFile(sstFileName);
+ filesToBeDeleted.add(new File(liveFileMetaData.path(),
+ liveFileMetaData.fileName()));
}
}
}
+
+ Iterator<File> files = filesToBeDeleted.iterator();
+ while (files.hasNext()) {
Review Comment:
Manifest files get updated only when the deletes are processed.
--
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
[GitHub] [ozone] prashantpogde commented on pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "prashantpogde (via GitHub)" <gi...@apache.org>.
prashantpogde commented on PR #5035:
URL: https://github.com/apache/ozone/pull/5035#issuecomment-1629749150
Just a side note. As I was reviewing this PR, I realized we can have other potential problem areas here. When follower tries to catch up with the leader, it should start with initial state as absolutely empty. That is because, we can not assume that sst files between leader and follower will have the same content. RocksDB instances and the content of the rocksDB directory on the OM nodes are only logically equal but physically they could be spread over completely different sst files. cc : @GeorgeJahad @smengcl @hemantk-12
--
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
[GitHub] [ozone] swamirishi commented on pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #5035:
URL: https://github.com/apache/ozone/pull/5035#issuecomment-1626882536
@GeorgeJahad Can you take a look at this PR?
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261888870
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/util/ManagedObjectUtil.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.utils.db.managed.util;
+
+import org.awaitility.Awaitility;
+import org.awaitility.core.ConditionTimeoutException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.time.Duration;
+import java.time.Instant;
+
+
+/**
+ * Util classes for managed objects.
+ */
+public final class ManagedObjectUtil {
+
+ static final Logger LOG =
+ LoggerFactory.getLogger(ManagedObjectUtil.class);
+ private static final Duration POLL_DELAY_DURATION = Duration.ZERO;
+ private static final Duration POLL_INTERVAL_DURATION = Duration.ofMillis(100);
+ private static final Duration POLL_MAX_DURATION = Duration.ofSeconds(5);
Review Comment:
`POLL_MAX_DURATION` is not used anywhere.
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksDB.java:
##########
@@ -78,4 +87,15 @@ public static ManagedRocksDB open(
RocksDB.open(path, columnFamilyDescriptors, columnFamilyHandles)
);
}
+
+ public void deleteFile(LiveFileMetaData fileToBeDeleted)
Review Comment:
1. Can you please add a java doc for this function describing how it is different for RocksDB's deleteFile?
1. I think it would be better to just pass the `fileName` to `ManagedRocksDB#deleteFile()` here instead of getting it from LiveFileMetaData. Because you are just making `RocksDB#deleteFile()` synchronous. Also the log statement should be moved to RocksDatabase where it was before. ManagedRocksDB doesn't need to know why you are deleting a file.
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/util/package-info.java:
##########
@@ -0,0 +1,22 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+/**
Review Comment:
Please add an empty like here.
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/util/ManagedObjectUtil.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.utils.db.managed.util;
+
+import org.awaitility.Awaitility;
+import org.awaitility.core.ConditionTimeoutException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.time.Duration;
+import java.time.Instant;
+
+
+/**
+ * Util classes for managed objects.
+ */
+public final class ManagedObjectUtil {
Review Comment:
I don't think it is make sense to create `ManagedObjectUtil` just for `waitForFileDelete`. Ideally it should be internal function to ` ManagedRocksDB` because it is not generic enough that it will be applicable to any type of `ManagedObject` other than `ManagedRocksDB`.
If you really want this to be a util function, either move it to existing util class `ManagedRocksObjectUtils` or change the name to `ManagedRocksDBUtils`.
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. [Snapshot] SstFilteringService should be stopped before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1262873864
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -123,7 +133,7 @@ public BackgroundTaskResult call() throws Exception {
long snapshotLimit = snapshotLimitPerTask;
- while (iterator.hasNext() && snapshotLimit > 0) {
+ while (iterator.hasNext() && snapshotLimit > 0 && running.get()) {
Review Comment:
My point is you are not achieving much with this because `SSTFilteringService` will run after the reloading and do the clean-up again. You can argue that it is failing fast but I don't see much value of that here because it is background service and no client is waiting on it.
Anyways, if you believe this is the best and cleaner approach, please go ahead.
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261965555
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksDB.java:
##########
@@ -78,4 +87,15 @@ public static ManagedRocksDB open(
RocksDB.open(path, columnFamilyDescriptors, columnFamilyHandles)
);
}
+
+ public void deleteFile(LiveFileMetaData fileToBeDeleted)
Review Comment:
I need the db path as well for polling the existance of the file.
--
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
[GitHub] [ozone] GeorgeJahad commented on pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on PR #5035:
URL: https://github.com/apache/ozone/pull/5035#issuecomment-1633377598
@prashantpogde and I discussed this offline. We agreed it won't be a problem, based on the use of the candidate dir as the initial empty state.
> Just a side note. As I was reviewing this PR, I realized we can have other potential problem areas here. When follower tries to catch up with the leader, it should start with initial state as absolutely empty. That is because, we can not assume that sst files between leader and follower will have the same content. RocksDB instances and the content of the rocksDB directory on the OM nodes are only logically equal but physically they could be spread over completely different sst files. cc : @GeorgeJahad @smengcl @hemantk-12
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261952134
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/RdbUtil.java:
##########
@@ -20,24 +20,37 @@
import org.apache.hadoop.hdds.utils.db.managed.ManagedDBOptions;
import org.apache.hadoop.hdds.utils.db.managed.ManagedRocksDB;
+import org.awaitility.Awaitility;
+import org.awaitility.core.ConditionTimeoutException;
import org.rocksdb.ColumnFamilyDescriptor;
import org.rocksdb.ColumnFamilyHandle;
import org.rocksdb.RocksDBException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import java.io.File;
import java.nio.charset.StandardCharsets;
+import java.time.Duration;
+import java.time.Instant;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
+
/**
* Temporary class to test snapshot diff functionality.
* This should be removed later.
*/
public final class RdbUtil {
+ static final Logger LOG =
+ LoggerFactory.getLogger(RdbUtil.class);
+ private static final Duration POLL_DELAY_DURATION = Duration.ZERO;
+ private static final Duration POLL_INTERVAL_DURATION = Duration.ofMillis(100);
+ private static final Duration POLL_MAX_DURATION = Duration.ofSeconds(5);
Review Comment:
done
--
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
[GitHub] [ozone] swamirishi commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1262601366
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java:
##########
@@ -979,9 +982,17 @@ public void deleteFilesNotMatchingPrefix(
+ " {} from db: {}", sstFileName,
liveFileMetaData.columnFamilyName(), db.get().getName());
db.get().deleteFile(sstFileName);
+ filesToBeDeleted.add(new File(liveFileMetaData.path(),
+ liveFileMetaData.fileName()));
}
}
}
+
+ Iterator<File> files = filesToBeDeleted.iterator();
+ while (files.hasNext()) {
Review Comment:
done
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261681974
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/RdbUtil.java:
##########
@@ -20,24 +20,37 @@
import org.apache.hadoop.hdds.utils.db.managed.ManagedDBOptions;
import org.apache.hadoop.hdds.utils.db.managed.ManagedRocksDB;
+import org.awaitility.Awaitility;
+import org.awaitility.core.ConditionTimeoutException;
import org.rocksdb.ColumnFamilyDescriptor;
import org.rocksdb.ColumnFamilyHandle;
import org.rocksdb.RocksDBException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import java.io.File;
import java.nio.charset.StandardCharsets;
+import java.time.Duration;
+import java.time.Instant;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
+
/**
* Temporary class to test snapshot diff functionality.
* This should be removed later.
*/
public final class RdbUtil {
+ static final Logger LOG =
+ LoggerFactory.getLogger(RdbUtil.class);
+ private static final Duration POLL_DELAY_DURATION = Duration.ZERO;
+ private static final Duration POLL_INTERVAL_DURATION = Duration.ofMillis(100);
+ private static final Duration POLL_MAX_DURATION = Duration.ofSeconds(5);
Review Comment:
Please remove these constants and unused imports.
##########
hadoop-hdds/rocksdb-checkpoint-differ/pom.xml:
##########
@@ -82,6 +82,10 @@ https://maven.apache.org/xsd/maven-4.0.0.xsd">
<groupId>org.apache.ozone</groupId>
<artifactId>hdds-rocks-native</artifactId>
</dependency>
+ <dependency>
Review Comment:
Do we need this now?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -123,7 +133,7 @@ public BackgroundTaskResult call() throws Exception {
long snapshotLimit = snapshotLimitPerTask;
- while (iterator.hasNext() && snapshotLimit > 0) {
+ while (iterator.hasNext() && snapshotLimit > 0 && running.get()) {
Review Comment:
I don't think if `running.get()` check is really needed because [BackgroundService#shutdown()](https://github.com/apache/ozone/blob/a66e43e2838f087229cb4b0fac1e1dc969ab80dd/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/BackgroundService.java#L140) makes sure that all the active threads get terminated. Same for overriding `start()` and `shutdown()` functions.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3632,7 +3633,7 @@ TermIndex installCheckpoint(String leaderId, Path checkpointLocation,
keyManager.stop();
stopSecretManager();
stopTrashEmptier();
-
+ omSnapshotManager.getSnapshotCache().invalidateAll();
Review Comment:
I see it solves the problem but ideally we should only invalidate cache when [OmMetadataManager was stopped](https://github.com/apache/ozone/blob/a66e43e2838f087229cb4b0fac1e1dc969ab80dd/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L3743) because when `OmMetadataManager` gets stopped we load [everything including cache fresh](https://github.com/apache/ozone/blob/a66e43e2838f087229cb4b0fac1e1dc969ab80dd/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone)/om/OzoneManager.java#L3736C9-L3736C22).
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -80,6 +80,7 @@
import org.apache.hadoop.hdds.scm.ScmInfo;
import org.apache.hadoop.hdds.scm.client.HddsClientUtils;
import org.apache.hadoop.hdds.server.OzoneAdmins;
+import org.apache.hadoop.hdds.utils.BackgroundService;
Review Comment:
Remove this import.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java:
##########
@@ -34,6 +34,7 @@
import org.apache.hadoop.hdds.utils.db.managed.ManagedTransactionLogIterator;
import org.apache.hadoop.hdds.utils.db.managed.ManagedWriteBatch;
import org.apache.hadoop.hdds.utils.db.managed.ManagedWriteOptions;
+import org.apache.ozone.rocksdb.util.RdbUtil;
Review Comment:
Please remove these unused imports.
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. SST Filtering service should be stopped while before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1261745811
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3632,7 +3633,7 @@ TermIndex installCheckpoint(String leaderId, Path checkpointLocation,
keyManager.stop();
stopSecretManager();
stopTrashEmptier();
-
+ omSnapshotManager.getSnapshotCache().invalidateAll();
Review Comment:
Had a offline discussion and problem is that in case of reloading OmMetadataManager, previous [snapshotCache](https://github.com/apache/ozone/blob/5a6f08ce48eb7053894596a7bb2d3993072f8c8a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java#L116) object may/may not get GC'ed and new snapshotCache object get created. If previous snapshotCache is not GC'ed and has opened RocksDB instance, it will keep deleting file because of RocksDB async delete (delete command was executed by previous SSTFilteringService run). New cache will try to open RocksDB instance based on new manifest and will fail because file is missing.
Based on logs: On followed node,
1. File 000077.sst was deleted by SSTFilteringService at hemant_sst_filtering.log line 20972.
1. File 000077.sst was copied from candidate dir to snapshot db dir at hemant_sst_filtering.log line 30871
1. Background services including SSTFilteringService get started at hemant_sst_filtering.log line 31062
1. Later SSTFilteringService run fails because file doesn't exist. at hemant_sst_filtering.log line 31111-31112.
Which was the overall `testInstallIncrementalSnapshot`.
Attached the log file for details: [testInstallIncrementalSnapshot.log](https://github.com/apache/ozone/files/12033012/hemant_sst_filtering.log)
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5035: HDDS-8987. [Snapshot] Make RocksDB delete sstFile api synchronous. Invalidate Snapshot cache before copying incremental snapshot
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5035:
URL: https://github.com/apache/ozone/pull/5035#discussion_r1267498449
##########
hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksDB.java:
##########
@@ -78,4 +86,20 @@ public static ManagedRocksDB open(
RocksDB.open(path, columnFamilyDescriptors, columnFamilyHandles)
);
}
+
+ /**
+ * Delete liveMetaDataFile from rocks db using RocksDB#deleteFile Api.
+ * This function makes the RocksDB#deleteFile Api synchronized by waiting
+ * for the deletes to happen.
+ * @param fileToBeDeleted File to be deleted.
+ * @throws RocksDBException In the underlying db throws an exception.
+ * @throws IOException In the case file is not deleted.
+ */
+ public void deleteFile(LiveFileMetaData fileToBeDeleted)
+ throws RocksDBException, IOException {
+ String sstFileName = fileToBeDeleted.fileName();
+ this.get().deleteFile(sstFileName);
+ File file = new File(fileToBeDeleted.path(), fileToBeDeleted.fileName());
+ ManagedRocksObjectUtils.waitForFileDelete(file, Duration.ofSeconds(60));
Review Comment:
Based on the current usage of this function, only SST filtering service will fail for the snapshot.
https://github.com/apache/ozone/blob/7a19afa71401e356cd86a99648e94278ea2850c8/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java#L193
--
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