Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HDDS-7974. [Snapshot] KeyDeletingService to be aware of Ozone snapshots #4486

Merged
merged 7 commits into from
Apr 4, 2023
Merged

HDDS-7974. [Snapshot] KeyDeletingService to be aware of Ozone snapshots #4486

merged 7 commits into from
Apr 4, 2023

Conversation

aswinshakil
Copy link
Member

@aswinshakil aswinshakil commented Mar 28, 2023 •
edited

What changes were proposed in this pull request?

Refactored KeyDeletingService to accommodate deleting keys from snapshot DB's deletedTable.

  • Add support for snapshots in OMKeyPurgeRequest and OMKeyPurgeResponse
  • Abstracted common code for both SnapshotDeletingService and KeyDeletingService into AbstractKeyDeletingService
  • Added getBlocksForKeyDelete() which gets the BlockGroup for a single key from OmKeyLocationInfoGroup

What is the link to the Apache JIRA

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

How was this patch tested?

Unit test and Existing test.

@aswinshakil aswinshakil added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Mar 28, 2023
@aswinshakil aswinshakil self-assigned this Mar 28, 2023
smengcl
smengcl approved these changes Mar 28, 2023
View reviewed changes
Copy link
Contributor

@smengcl smengcl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @aswinshakil . I figure I would post the first part of my review while I continue reviewing the KDT refactoring.

hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/OMMetadataManager.java Show resolved Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java Show resolved Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java Outdated Show resolved Hide resolved
...ne/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyPurgeRequest.java Outdated Show resolved Hide resolved
...ne/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyPurgeRequest.java Show resolved Hide resolved
.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyPurgeResponse.java Outdated Show resolved Hide resolved
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto Outdated Show resolved Hide resolved
smengcl
smengcl requested changes Mar 28, 2023
View reviewed changes
...one-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java Outdated Show resolved Hide resolved
...one-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java Outdated Show resolved Hide resolved
...one-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java Outdated Show resolved Hide resolved
...one-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java Outdated
Comment on lines 81 to 84
// TODO: Once HA and non-HA paths are merged, we should have
// only one code path here. Purge keys should go through an
// OMRequest model.
delCount = deleteAllKeys(results, manager);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not even sure OM can still boot without Ratis anymore. So this code path might never be used.

Need to double check.

...one-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java Outdated Show resolved Hide resolved
...one-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java Outdated Show resolved Hide resolved
...ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyDeletingService.java Outdated Show resolved Hide resolved
.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java Show resolved Hide resolved
.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java Show resolved Hide resolved
.../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java Outdated Show resolved Hide resolved
smengcl
smengcl reviewed Mar 30, 2023
View reviewed changes
...ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyDeletingService.java
@@ -130,7 +129,7 @@ public BackgroundTaskResult call() throws Exception {
.getPendingDeletionKeys(keyLimitPerTask);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can add a separate timer inside getPendingDeletionKeys() later. Just a thought.

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java Outdated Show resolved Hide resolved
smengcl
smengcl approved these changes Mar 31, 2023
View reviewed changes
Copy link
Contributor

@smengcl smengcl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall lgtm

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java Outdated Show resolved Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java Outdated Show resolved Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java Outdated Show resolved Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java Outdated Show resolved Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java Show resolved Hide resolved
@aswinshakil
Copy link
Member Author

Thanks @smengcl for the review.

@aswinshakil aswinshakil merged commit 3adae01 into apache:master Apr 4, 2023
errose28 added a commit to errose28/ozone that referenced this pull request Apr 6, 2023
@errose28
Merge branch 'master' into HDDS-8115
6207a08 
* master: (155 commits)
  update readme (apache#4535)
  HDDS-8374. Disable flaky unit test: TestContainerStateCounts
  HDDS-8016. updated the ozone doc for linked bucket and deletion async limitation (apache#4526)
  HDDS-8237. [Snapshot] loadDb() used by SstFiltering service creates extraneous directories. (apache#4446)
  HDDS-8035. Intermittent timeout in TestOzoneManagerHAWithData.testOMHAMetrics (apache#4362)
  HDDS-8039. Allow container inspector to run from ozone debug. (apache#4337)
  HDDS-8304. [Snapshot] Reduce flakiness in testSkipTrackingWithZeroSnapshot (apache#4487)
  HDDS-7974. [Snapshot] KeyDeletingService to be aware of Ozone snapshots (apache#4486)
  HDDS-8368. ReplicationManager: Create ContainerReplicaOp with correct target Datanode (apache#4532)
  HDDS-8358. Fix the space usage comparator in ContainerBalancerSelectionCriteria (apache#4527)
  HDDS-8359. ReplicationManager: Fix getContainerReplicationHealth() so that it builds ContainerCheckRequest correctly (apache#4528)
  HDDS-8361. Useless object in TestOzoneBlockTokenIdentifier (apache#4517)
  HDDS-8325. Consolidate and refine RocksDB metrics of services (apache#4506)
  HDDS-8135. Incorrect synchronization during certificate renewal in DefaultCertificateClient. (apache#4381)
  HDDS-8127. Exclude deleted containers from Recon container count (apache#4440)
  HDDS-8364. ReadReplicas may give wrong results with topology-aware read enabled (apache#4522)
  HDDS-8354. Avoid WARNING about ObjectEndpoint#get (apache#4515)
  HDDS-8324. DN data cache gets removed randomly asking for data from disk (apache#4499)
  HDDS-8291. Upgrade to Hadoop 3.3.5 (apache#4484)
  HDDS-8355. Mark TestOMRatisSnapshots#testInstallSnapshot as flaky
  ...
@smengcl smengcl mentioned this pull request Apr 14, 2023
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smengcl smengcl smengcl approved these changes

@prashantpogde prashantpogde Awaiting requested review from prashantpogde

Assignees

@aswinshakil aswinshakil

Labels
snapshot https://issues.apache.org/jira/browse/HDDS-6517
Projects
None yet
Milestone
No milestone
2 participants
@aswinshakil @smengcl