From f82cee901c2383fe1789984e903fb9cdfcccdee6 Mon Sep 17 00:00:00 2001 From: Ivan Andika Date: Wed, 17 Jan 2024 11:45:15 +0800 Subject: [PATCH 1/5] HDDS-10138. NPE for SstFilteringService in OMDBCheckpointServlet.Lock --- .../org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java index 2a7771fe60a3..5064d782bc7c 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java @@ -664,7 +664,9 @@ public BootstrapStateHandler.Lock lock() throws InterruptedException { // First lock all the handlers. keyDeletingService.getBootstrapStateLock().lock(); - sstFilteringService.getBootstrapStateLock().lock(); + if (sstFilteringService != null) { + sstFilteringService.getBootstrapStateLock().lock(); + } rocksDbCheckpointDiffer.getBootstrapStateLock().lock(); snapshotDeletingService.getBootstrapStateLock().lock(); @@ -677,7 +679,9 @@ public BootstrapStateHandler.Lock lock() public void unlock() { snapshotDeletingService.getBootstrapStateLock().unlock(); rocksDbCheckpointDiffer.getBootstrapStateLock().unlock(); - sstFilteringService.getBootstrapStateLock().unlock(); + if (sstFilteringService != null) { + sstFilteringService.getBootstrapStateLock().unlock(); + } keyDeletingService.getBootstrapStateLock().unlock(); } } From 0eeedc7a5cc81c1a438d921266291dc5925e17ca Mon Sep 17 00:00:00 2001 From: Ivan Andika Date: Wed, 17 Jan 2024 14:23:41 +0800 Subject: [PATCH 2/5] Also do nullity check for related services --- .../hadoop/ozone/om/OMDBCheckpointServlet.java | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java index 5064d782bc7c..8bc371503227 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java @@ -667,8 +667,12 @@ public BootstrapStateHandler.Lock lock() if (sstFilteringService != null) { sstFilteringService.getBootstrapStateLock().lock(); } - rocksDbCheckpointDiffer.getBootstrapStateLock().lock(); - snapshotDeletingService.getBootstrapStateLock().lock(); + if (rocksDbCheckpointDiffer != null) { + rocksDbCheckpointDiffer.getBootstrapStateLock().lock(); + } + if (snapshotDeletingService != null) { + snapshotDeletingService.getBootstrapStateLock().lock(); + } // Then wait for the double buffer to be flushed. om.awaitDoubleBufferFlush(); @@ -677,8 +681,12 @@ public BootstrapStateHandler.Lock lock() @Override public void unlock() { - snapshotDeletingService.getBootstrapStateLock().unlock(); - rocksDbCheckpointDiffer.getBootstrapStateLock().unlock(); + if (snapshotDeletingService != null) { + snapshotDeletingService.getBootstrapStateLock().unlock(); + } + if (rocksDbCheckpointDiffer != null) { + rocksDbCheckpointDiffer.getBootstrapStateLock().unlock(); + } if (sstFilteringService != null) { sstFilteringService.getBootstrapStateLock().unlock(); } From 41d18a13d2f82f5f094722bb7cd1c3bb13e40b67 Mon Sep 17 00:00:00 2001 From: Ivan Andika Date: Wed, 17 Jan 2024 16:41:14 +0800 Subject: [PATCH 3/5] Add preconditions and use optional --- .../apache/hadoop/hdds/utils/db/RDBStore.java | 1 + .../hadoop/ozone/om/KeyManagerImpl.java | 2 + .../ozone/om/OMDBCheckpointServlet.java | 56 ++++++++++--------- 3 files changed, 33 insertions(+), 26 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBStore.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBStore.java index 71cd3716e566..defe804344eb 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBStore.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBStore.java @@ -198,6 +198,7 @@ public String getSnapshotsParentDir() { return snapshotsParentDir; } + @Override public RocksDBCheckpointDiffer getRocksDBCheckpointDiffer() { return rocksDBCheckpointDiffer; } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java index 37b1c129af43..ad8a1a8887de 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java @@ -697,10 +697,12 @@ public BackgroundService getMultipartUploadCleanupService() { return multipartUploadCleanupService; } + @Override public SstFilteringService getSnapshotSstFilteringService() { return snapshotSstFilteringService; } + @Override public SnapshotDeletingService getSnapshotDeletingService() { return snapshotDeletingService; } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java index 8bc371503227..4a583d08ed48 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java @@ -36,6 +36,7 @@ import org.apache.hadoop.security.UserGroupInformation; import org.apache.ozone.rocksdiff.RocksDBCheckpointDiffer; +import org.apache.ratis.thirdparty.com.google.common.base.Preconditions; import org.jetbrains.annotations.NotNull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -55,6 +56,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.concurrent.atomic.AtomicLong; import java.util.stream.Collectors; @@ -644,34 +646,41 @@ public BootstrapStateHandler.Lock getBootstrapStateLock() { } static class Lock extends BootstrapStateHandler.Lock { - private final BootstrapStateHandler keyDeletingService; - private final BootstrapStateHandler sstFilteringService; - private final BootstrapStateHandler rocksDbCheckpointDiffer; - private final BootstrapStateHandler snapshotDeletingService; + private final Optional keyDeletingService; + private final Optional sstFilteringService; + private final Optional rocksDbCheckpointDiffer; + private final Optional snapshotDeletingService; private final OzoneManager om; Lock(OzoneManager om) { + Preconditions.checkNotNull(om); + Preconditions.checkNotNull(om.getKeyManager()); + Preconditions.checkNotNull(om.getMetadataManager()); + Preconditions.checkNotNull(om.getMetadataManager().getStore()); + this.om = om; - keyDeletingService = om.getKeyManager().getDeletingService(); - sstFilteringService = om.getKeyManager().getSnapshotSstFilteringService(); - rocksDbCheckpointDiffer = om.getMetadataManager().getStore() - .getRocksDBCheckpointDiffer(); - snapshotDeletingService = om.getKeyManager().getSnapshotDeletingService(); + keyDeletingService = Optional.ofNullable(om.getKeyManager().getDeletingService()); + sstFilteringService = Optional.ofNullable(om.getKeyManager().getSnapshotSstFilteringService()); + rocksDbCheckpointDiffer = Optional.ofNullable(om.getMetadataManager().getStore() + .getRocksDBCheckpointDiffer()); + snapshotDeletingService = Optional.ofNullable(om.getKeyManager().getSnapshotDeletingService()); } @Override public BootstrapStateHandler.Lock lock() throws InterruptedException { // First lock all the handlers. - keyDeletingService.getBootstrapStateLock().lock(); - if (sstFilteringService != null) { - sstFilteringService.getBootstrapStateLock().lock(); + if (keyDeletingService.isPresent()) { + keyDeletingService.get().getBootstrapStateLock().lock(); + } + if (sstFilteringService.isPresent()) { + sstFilteringService.get().getBootstrapStateLock().lock(); } - if (rocksDbCheckpointDiffer != null) { - rocksDbCheckpointDiffer.getBootstrapStateLock().lock(); + if (rocksDbCheckpointDiffer.isPresent()) { + rocksDbCheckpointDiffer.get().getBootstrapStateLock().lock(); } - if (snapshotDeletingService != null) { - snapshotDeletingService.getBootstrapStateLock().lock(); + if (snapshotDeletingService.isPresent()) { + snapshotDeletingService.get().getBootstrapStateLock().lock(); } // Then wait for the double buffer to be flushed. @@ -681,16 +690,11 @@ public BootstrapStateHandler.Lock lock() @Override public void unlock() { - if (snapshotDeletingService != null) { - snapshotDeletingService.getBootstrapStateLock().unlock(); - } - if (rocksDbCheckpointDiffer != null) { - rocksDbCheckpointDiffer.getBootstrapStateLock().unlock(); - } - if (sstFilteringService != null) { - sstFilteringService.getBootstrapStateLock().unlock(); - } - keyDeletingService.getBootstrapStateLock().unlock(); + snapshotDeletingService.ifPresent(deletingService -> deletingService.getBootstrapStateLock().unlock()); + rocksDbCheckpointDiffer.ifPresent( + rocksDBCheckpointDiffer -> rocksDBCheckpointDiffer.getBootstrapStateLock().unlock()); + sstFilteringService.ifPresent(filteringService -> filteringService.getBootstrapStateLock().unlock()); + keyDeletingService.ifPresent(deletingService -> deletingService.getBootstrapStateLock().unlock()); } } } From 707098bdba1aad7f647a7d54e863a8d0f2485d48 Mon Sep 17 00:00:00 2001 From: Ivan Andika Date: Wed, 17 Jan 2024 17:23:23 +0800 Subject: [PATCH 4/5] Use Guava preconditions --- .../java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java index 4a583d08ed48..c87ec1b214f5 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java @@ -36,7 +36,7 @@ import org.apache.hadoop.security.UserGroupInformation; import org.apache.ozone.rocksdiff.RocksDBCheckpointDiffer; -import org.apache.ratis.thirdparty.com.google.common.base.Preconditions; +import com.google.common.base.Preconditions; import org.jetbrains.annotations.NotNull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; From 6834b5823436e8ac46f22c468ae8a1162983f7a0 Mon Sep 17 00:00:00 2001 From: Ivan Andika Date: Thu, 18 Jan 2024 09:18:00 +0800 Subject: [PATCH 5/5] Simplify logic based on review --- .../ozone/om/OMDBCheckpointServlet.java | 40 +++++++------------ 1 file changed, 14 insertions(+), 26 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java index c87ec1b214f5..3e6d70626728 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java @@ -56,7 +56,6 @@ import java.util.List; import java.util.Map; import java.util.Objects; -import java.util.Optional; import java.util.Set; import java.util.concurrent.atomic.AtomicLong; import java.util.stream.Collectors; @@ -646,10 +645,7 @@ public BootstrapStateHandler.Lock getBootstrapStateLock() { } static class Lock extends BootstrapStateHandler.Lock { - private final Optional keyDeletingService; - private final Optional sstFilteringService; - private final Optional rocksDbCheckpointDiffer; - private final Optional snapshotDeletingService; + private final List locks; private final OzoneManager om; Lock(OzoneManager om) { @@ -659,28 +655,24 @@ static class Lock extends BootstrapStateHandler.Lock { Preconditions.checkNotNull(om.getMetadataManager().getStore()); this.om = om; - keyDeletingService = Optional.ofNullable(om.getKeyManager().getDeletingService()); - sstFilteringService = Optional.ofNullable(om.getKeyManager().getSnapshotSstFilteringService()); - rocksDbCheckpointDiffer = Optional.ofNullable(om.getMetadataManager().getStore() - .getRocksDBCheckpointDiffer()); - snapshotDeletingService = Optional.ofNullable(om.getKeyManager().getSnapshotDeletingService()); + + locks = Stream.of( + om.getKeyManager().getDeletingService(), + om.getKeyManager().getSnapshotSstFilteringService(), + om.getMetadataManager().getStore().getRocksDBCheckpointDiffer(), + om.getKeyManager().getSnapshotDeletingService() + ) + .filter(Objects::nonNull) + .map(BootstrapStateHandler::getBootstrapStateLock) + .collect(Collectors.toList()); } @Override public BootstrapStateHandler.Lock lock() throws InterruptedException { // First lock all the handlers. - if (keyDeletingService.isPresent()) { - keyDeletingService.get().getBootstrapStateLock().lock(); - } - if (sstFilteringService.isPresent()) { - sstFilteringService.get().getBootstrapStateLock().lock(); - } - if (rocksDbCheckpointDiffer.isPresent()) { - rocksDbCheckpointDiffer.get().getBootstrapStateLock().lock(); - } - if (snapshotDeletingService.isPresent()) { - snapshotDeletingService.get().getBootstrapStateLock().lock(); + for (BootstrapStateHandler.Lock lock : locks) { + lock.lock(); } // Then wait for the double buffer to be flushed. @@ -690,11 +682,7 @@ public BootstrapStateHandler.Lock lock() @Override public void unlock() { - snapshotDeletingService.ifPresent(deletingService -> deletingService.getBootstrapStateLock().unlock()); - rocksDbCheckpointDiffer.ifPresent( - rocksDBCheckpointDiffer -> rocksDBCheckpointDiffer.getBootstrapStateLock().unlock()); - sstFilteringService.ifPresent(filteringService -> filteringService.getBootstrapStateLock().unlock()); - keyDeletingService.ifPresent(deletingService -> deletingService.getBootstrapStateLock().unlock()); + locks.forEach(BootstrapStateHandler.Lock::unlock); } } }