Skip to content

HDDS-6287. BlockDeletingService add inDeleting Container condition#3058

Closed
guohao-rosicky wants to merge 3 commits intoapache:masterfrom
guohao-rosicky:guohao-HDDS-6287-dev
Closed

HDDS-6287. BlockDeletingService add inDeleting Container condition#3058
guohao-rosicky wants to merge 3 commits intoapache:masterfrom
guohao-rosicky:guohao-HDDS-6287-dev

Conversation

@guohao-rosicky
Copy link
Contributor

@guohao-rosicky guohao-rosicky commented Feb 9, 2022

What changes were proposed in this pull request?

BlockDeletingService add inDeleting Container condition

The container that is being deleted does not participate in container selection, preventing worker threads from holding locks for a long time

What is the link to the Apache JIRA

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

@swagle swagle requested a review from errose28 March 7, 2022 17:06
@errose28
Copy link
Contributor

Thanks for working on this @guohao-rosicky. My understanding is that the current background service implementation could allow more container delete tasks to be kicked off for an overlapping container set before the current ones have finished (if the current ones take a very long time), and that is why code was added to track containers currently under deletion.

To me it would seem more intuitive to have BackgroundService block until its current set of tasks have finished or timed out before adding new ones, since I don't think any of Ozone's background services can effectively handle overlapping executions. However, this behavior was explicitly changed in HDDS-4231, so maybe @lokeshj1703 can provide some insight as to why the background service was changed in this way.

The current BackgroundService implementation seems a bit dangerous to me as nothing is enforcing the timeout and overlapping runs on the same data will break child classes. FWIW Java's ScheduledFuture#scheduleWithFixedDelay defines the interval as being between the end of one task and the start of another, so the BackgroundService is using a somewhat non-standard definition for interval here I would say.

@lokeshj1703
Copy link
Contributor

@guohao-rosicky Thanks for working on this! Can you please provide more info on the problem and how inDeleting containers are involved?

@adoroszlai
Copy link
Contributor

/pending please provide more info on the problem

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Marking this issue as un-mergeable as requested.

Please use /ready comment when it's resolved.

Please note that the PR will be closed after 21 days of inactivity from now. (But can be re-opened anytime later...)

please provide more info on the problem

@github-actions
Copy link

Thank you very much for the patch. I am closing this PR temporarily as there was no activity recently and it is waiting for response from its author.

It doesn't mean that this PR is not important or ignored: feel free to reopen the PR at any time.

It only means that attention of committers is not required. We prefer to keep the review queue clean. This ensures PRs in need of review are more visible, which results in faster feedback for all PRs.

If you need ANY help to finish this PR, please contact the community on the mailing list or the slack channel."

@github-actions github-actions bot closed this Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments