Skip to content

HDDS-7328. Improve Deletion of FSO Paths#3844

Merged
JacksonYao287 merged 4 commits intoapache:masterfrom
Xushaohong:HDDS-7328
Oct 27, 2022
Merged

HDDS-7328. Improve Deletion of FSO Paths#3844
JacksonYao287 merged 4 commits intoapache:masterfrom
Xushaohong:HDDS-7328

Conversation

@Xushaohong
Copy link
Contributor

@Xushaohong Xushaohong commented Oct 17, 2022

What changes were proposed in this pull request?

Currently, the deleting speed of paths in the FSO bucket is too slow.

Every time the DirectoryDeletingService executes(every 1m by default),  it chooses only one path from DeletedDirectroyTable and tries adding its sub-files and sub-dirs into the PurgeDirectories Request. The real deletion happens in OMKeyDeleteResponseWithFSO, which moved all sub-dirs into DeletedDirectroyTable.  Then these sub-dirs will be chosen by the future execution of DirectoryDeletingService.

In the real production environment, such deletion speed is not applicable. There could be lots of dirs deleted without sub-dirs or sub-files. The dir deletion speed would lag behind the dir creation speed.

In this PR, I propose to optimize the logic of the DirectoryDeletingService, we could consume all the quota (ozone.path.deleting.limit.per.task) to delete paths as much as possible. The good news is the PB is already designed to do so, we could add more PurgePathRequest in one PurgeDirectoriesRequest.

Originally: 1 min. At most 1 path from deletedDirectoryTable.
Now: 1min. At most 10000 paths from deletedDirectoryTable.

message PurgeDirectoriesRequest {
  repeated PurgePathRequest deletedPath = 1;
}

What is the link to the Apache JIRA

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

How was this patch tested?

UT and real env

@kaijchen kaijchen requested a review from rakeshadr October 17, 2022 07:14
@Xushaohong
Copy link
Contributor Author

@linyiqun @rakeshadr PTAL.

This should be compatible with HDDS-5048.

This aims to improve the efficiency of the deletion if not enough sub-files or sub-dirs are chosen.

Copy link
Contributor

@JacksonYao287 JacksonYao287 left a comment

Choose a reason for hiding this comment

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

thank @Xushaohong for the work! LGTM +1

deletedDirsCount.addAndGet(dirNum);
movedDirsCount.addAndGet(subDirNum);
movedFilesCount.addAndGet(subFileNum);
LOG.info("Number of dirs deleted: {}, Number of sub-files moved:" +
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: maybe debug here is better than info

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense! I have updated.

Copy link
Contributor Author

@Xushaohong Xushaohong Oct 26, 2022

Choose a reason for hiding this comment

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

Hi, Jackson! After some thought, and according to practice, such conclusion info should be logged instead of debugged, such msg will not fill too much, but useful!

@JacksonYao287 JacksonYao287 merged commit f6cad7c into apache:master Oct 27, 2022
@Xushaohong
Copy link
Contributor Author

Thx @JacksonYao287 for the review!

@adoroszlai
Copy link
Contributor

@JacksonYao287 please don't forget to close the Jira issue and set the fix version based on where it was committed.

@kaijchen
Copy link
Member

Thanks @Xushaohong for the improvement and @JacksonYao287 for the review. I have backported this fix into 1.3 branch.


Table.KeyValue<String, OmKeyInfo> pendingDeletedDirInfo;
try {
TableIterator<String, ? extends KeyValue<String, OmKeyInfo>>
Copy link
Contributor

Choose a reason for hiding this comment

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

This introduces a TableIterator leak, which is fixed by this PR. #3922
Please help review it. @JacksonYao287

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks @duongkame for the fix, @Xushaohong could you also take a look?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @duongkame for the fix, @Xushaohong could you also take a look?

@duongkame Thx for the fix!
@JacksonYao287 That fix is merged. LTGM

jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Feb 21, 2023
(cherry picked from commit f6cad7c)
Change-Id: I1c93e0a705b49e198b1332c7ce39321b2a77966e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Comments