Skip to content

Conversation

@sadanand48
Copy link
Contributor

@sadanand48 sadanand48 commented Nov 6, 2020

What changes were proposed in this pull request?

This change makes the Trash Emptier multithreaded. i.e the main emptier thread starts a thread pool executor and executes tasks (checkpointing and deletion) present in the task queue. the parallelism is obtained at the bucket level as fs.trashRoots for a root OFS URI returns a list of bucket level trashroots.

What is the link to the Apache JIRA

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

How was this patch tested?

Added unit tests and tested manually

@linyiqun
Copy link
Contributor

linyiqun commented Nov 6, 2020

@sadanand48 , current unit test cases only cover the basic functionality of trash delete behavior. But here we introduce many new setting, like trash interval, emptier interval. Can we add more test case to cover this?

In additional, will Ozone trash feature also support S3 semantic path later?
Like we accidentally delete one key /vol/bucket/key (not a ozfs path), then we can recover this key from a trash path.

I see currently we implement this only for FS semantic path.

@sadanand48
Copy link
Contributor Author

@sadanand48 , current unit test cases only cover the basic functionality of trash delete behavior. But here we introduce many new setting, like trash interval, emptier interval. Can we add more test case to cover this?

In additional, will Ozone trash feature also support S3 semantic path later?
Like we accidentally delete one key /vol/bucket/key (not a ozfs path), then we can recover this key from a trash path.

I see currently we implement this only for FS semantic path.

Thanks for the comment @linyiqun . Yes I will add more unit tests to this. About the trash support for the object store I don't think we'll be supporting atleast for now since s3 too doesn't support trash.

@linyiqun
Copy link
Contributor

linyiqun commented Nov 7, 2020

About the trash support for the object store I don't think we'll be supporting atleast for now since s3 too doesn't support trash.

Okay, get it,

Copy link
Member

@elek elek left a comment

Choose a reason for hiding this comment

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

Thanks for the work @sadanand48

I have a minor suggestion about config handling.

I also feel that there is no clear agreement about the followed approach (at least I didn't find the sign in the public records of the community, but I have seen a question similar to mine: why do we need FileSystem in OM side.).

I suggested in HDDS-3367 to answer to the thread on the mailing list with clarifying the chosen direction, but didn't get any answer yet. It can be useful to have everybody informed and up-to-date.

@mukul1987
Copy link
Contributor

Thanks for the work @sadanand48

I have a minor suggestion about config handling.

I also feel that there is no clear agreement about the followed approach (at least I didn't find the sign in the public records of the community, but I have seen a question similar to mine: why do we need FileSystem in OM side.).

I suggested in HDDS-3367 to answer to the thread on the mailing list with clarifying the chosen direction, but didn't get any answer yet. It can be useful to have everybody informed and up-to-date.

The question has been answered on HDDS-3367. The current patch looks good to me. Resolving this comment for now.

@mukul1987
Copy link
Contributor

@sadanand48 , the test failures seems related, TestOzoneFileSystem#testTrash is failing in the latest run.

@mukul1987
Copy link
Contributor

All the checks are green. merging this change.

@mukul1987 mukul1987 merged commit e671211 into apache:master Jan 4, 2021
@elek
Copy link
Member

elek commented Jan 6, 2021

I suggested in HDDS-3367 to answer to the thread on the mailing list with clarifying the chosen direction, but didn't get any answer yet. It can be useful to have everybody informed and up-to-date.

The question has been answered on HDDS-3367. The current patch looks good to me. Resolving this comment for now.

I might miss but couldn't find the update on the mailing list.

Would you be so kind to update this thread:

https://lists.apache.org/thread.html/r9f6c11546a7a4792aadf6ea4c1b1bb4b7680eeb5406162e155d3c58b%40%3Cdev.ozone.apache.org%3E

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.

7 participants