HDDS-10138. NPE for SstFilteringService in OMDBCheckpointServlet.Lock #6015

ivandika3 · 2024-01-17T03:51:50Z

What changes were proposed in this pull request?

Fix NPE on OMDBCheckpointServlet caused by possible null SstFilteringService. The null SstFilteringService is due to configuration ozone.filesystem.snapshot.enabled=false or ozone.snapshot.filtering.service.interval=-1.

When NPE is thrown in the call of OMDBCheckpointServlet.Lock, this cause the subsequent call to OMDBCheckpointServlet.Lock.lock to block indefinitely since the previous call that triggered the NPE did not release the lock held on keyDeletingService.

The current solution is a simple nullity check. There might be better solution to prevent the NPE than just a nullity check.

What is the link to the Apache JIRA

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

How was this patch tested?

Manual test.

Clean CI: https://github.com/ivandika3/ozone/actions/runs/7550769000

ivandika3 · 2024-01-17T06:16:57Z

@GeorgeJahad @ChenSammi @symious Could you help to review this?

ivandika3 · 2024-01-17T06:27:37Z

We might also need to synchronize the lock and unlock to prevent possible deadlock.

symious · 2024-01-17T07:52:45Z

If it's for null check, can we change to use Optional?

ivandika3 · 2024-01-17T08:45:10Z

@symious Updated. PTAL.

symious · 2024-01-17T08:59:10Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java

-      sstFilteringService.getBootstrapStateLock().lock();
-      rocksDbCheckpointDiffer.getBootstrapStateLock().lock();
-      snapshotDeletingService.getBootstrapStateLock().lock();
+      if (keyDeletingService.isPresent()) {


Can changed to ifPresent.

Since lock throws InterruptedException (unlike unlock), it might not be possible to use ifPresent.

adoroszlai

Thanks @ivandika3 for the patch. I'd like to suggest some changes to simplify the logic.

adoroszlai · 2024-01-17T11:09:11Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java

+    private final Optional<BootstrapStateHandler> keyDeletingService;
+    private final Optional<BootstrapStateHandler> sstFilteringService;
+    private final Optional<BootstrapStateHandler> rocksDbCheckpointDiffer;
+    private final Optional<BootstrapStateHandler> snapshotDeletingService;


Suggested change

private final Optional<BootstrapStateHandler> keyDeletingService;

private final Optional<BootstrapStateHandler> sstFilteringService;

private final Optional<BootstrapStateHandler> rocksDbCheckpointDiffer;

private final Optional<BootstrapStateHandler> snapshotDeletingService;

private final List<BootstrapStateHandler.Lock> locks;

Thanks for the suggestion. Updated.

adoroszlai · 2024-01-17T11:10:40Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java

+      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());


Suggested change

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());

Updated. PTAL.

adoroszlai · 2024-01-17T11:11:06Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java

+      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();
+      }


Suggested change

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();

}

Updated. PTAL.

adoroszlai · 2024-01-17T11:11:37Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java

+      snapshotDeletingService.ifPresent(deletingService -> deletingService.getBootstrapStateLock().unlock());
+      rocksDbCheckpointDiffer.ifPresent(
+          rocksDBCheckpointDiffer -> rocksDBCheckpointDiffer.getBootstrapStateLock().unlock());
+      sstFilteringService.ifPresent(filteringService -> filteringService.getBootstrapStateLock().unlock());
+      keyDeletingService.ifPresent(deletingService -> deletingService.getBootstrapStateLock().unlock());


Suggested change

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);

Note: this will release locks in the same order as they were acquired, instead of in reverse order. I think both are fine as long as the order is fixed.

Updated PTAL.

Note: this will release locks in the same order as they were acquired, instead of in reverse order. I think both are fine as long as the order is fixed.

I think it should be fine.

However, seems like lock and unlock call are not synchronized properly on the Lock object. This might cause possible deadlocks. Shall we synchronize on the lock object (synhronized(this)) inside the lock and unlock call?

Do you mean the lock/unlock methods of OMDBCheckpointServlet.Lock?

Yes, I was thinking whether it's necessary to add synchronized block inside the lock and unlock. However, after I think about it again, it should be fine as it is.

adoroszlai · 2024-01-17T11:16:08Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java

 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Optional;


If suggestion to use list is accepted, this becomes unused.

Suggested change

import java.util.Optional;

Updated. PTAL.

adoroszlai

Thanks @ivandika3 for updating the patch, LGTM.

symious

LGTM.

hemantk-12

Thanks @ivandika3 for the patch.

LGTM.

adoroszlai · 2024-01-19T12:05:57Z

Thanks @ivandika3 for the patch, @hemantk-12, @symious for the review.

ivandika3 · 2024-01-19T12:15:02Z

Thank you for the reviews @adoroszlai @symious @hemantk-12

…apache#6015)

…apache#6015) (cherry picked from commit abc3e1f)

…Servlet.Lock (apache#6015) (apache#48) (cherry picked from commit abc3e1f)

HDDS-10138. NPE for SstFilteringService in OMDBCheckpointServlet.Lock

f82cee9

ivandika3 marked this pull request as ready for review January 17, 2024 06:16

Also do nullity check for related services

0eeedc7

Add preconditions and use optional

41d18a1

symious reviewed Jan 17, 2024

View reviewed changes

Use Guava preconditions

707098b

adoroszlai reviewed Jan 17, 2024

View reviewed changes

Simplify logic based on review

6834b58

adoroszlai reviewed Jan 18, 2024

View reviewed changes

adoroszlai requested review from ChenSammi, GeorgeJahad, smengcl and symious January 18, 2024 09:45

symious approved these changes Jan 18, 2024

View reviewed changes

adoroszlai requested a review from hemantk-12 January 18, 2024 19:29

hemantk-12 approved these changes Jan 18, 2024

View reviewed changes

adoroszlai merged commit abc3e1f into apache:master Jan 19, 2024

Tejaskriya pushed a commit to Tejaskriya/ozone that referenced this pull request Jan 24, 2024

HDDS-10138. NPE for SstFilteringService in OMDBCheckpointServlet.Lock (…

245e4dd

…apache#6015)

ivandika3 deleted the HDDS-10138 branch January 25, 2024 04:24

adoroszlai pushed a commit to adoroszlai/ozone that referenced this pull request Jan 25, 2024

HDDS-10138. NPE for SstFilteringService in OMDBCheckpointServlet.Lock (…

eae7061

…apache#6015) (cherry picked from commit abc3e1f)

adoroszlai mentioned this pull request Jan 25, 2024

[DO NOT MERGE] Backport some fixes from master to ozone-1.4 #6096

Merged

ivandika3 self-assigned this Apr 23, 2024

ptlrs pushed a commit to ptlrs/ozone that referenced this pull request Mar 8, 2025

CDPD-65559. HDDS-10138. NPE for SstFilteringService in OMDBCheckpoint…

0173442

…Servlet.Lock (apache#6015) (apache#48) (cherry picked from commit abc3e1f)

-      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());

HDDS-10138. NPE for SstFilteringService in OMDBCheckpointServlet.Lock #6015

HDDS-10138. NPE for SstFilteringService in OMDBCheckpointServlet.Lock #6015

Uh oh!

Conversation

ivandika3 commented Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

ivandika3 commented Jan 17, 2024

Uh oh!

ivandika3 commented Jan 17, 2024

Uh oh!

symious commented Jan 17, 2024

Uh oh!

ivandika3 commented Jan 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

symious left a comment

Choose a reason for hiding this comment

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

adoroszlai commented Jan 19, 2024

Uh oh!

ivandika3 commented Jan 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ivandika3 commented Jan 17, 2024 •

edited

Loading