Skip to content

[HUDI-2925] Fix duplicate cleaning of same files when unfinished clean operations are present. #4212

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
Merged
nsivabalan merged 6 commits into from
Feb 22, 2022
Merged

[HUDI-2925] Fix duplicate cleaning of same files when unfinished clean operations are present. #4212

nsivabalan merged 6 commits into from
Feb 22, 2022

Conversation

@prashantwason
Copy link
Member

@prashantwason prashantwason commented Dec 4, 2021

What is the purpose of the pull request

Fixed duplicate cleans which lead to issues with metadata table.

Brief change log

Perform unfinished clean operations before attempting to generate a new clean plan.
Refresh fsview after each unfinished clean operation.

Verify this pull request

Added unit test TestHoodieBackedMetadata::testMultiClean

Committer checklist

  • Has a corresponding JIRA in PR title & commit

  • Commit message is descriptive of the change

  • CI is green

  • Necessary doc changes done or have another open PR

  • For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

@prashantwason prashantwason requested review from bvaradar, nsivabalan and vinothchandar and removed request for bvaradar December 4, 2021 00:58
@prashantwason prashantwason force-pushed the pw_fix_multi_clean_OSS branch from 951d3b4 to c75df82 Compare December 4, 2021 01:00
@nsivabalan nsivabalan self-assigned this Dec 22, 2021
@nsivabalan nsivabalan added the priority:critical Production degraded; pipelines stalled label Jan 4, 2022
@nsivabalan
Copy link
Contributor

nsivabalan commented Jan 4, 2022

Hey @prashantwason : Can you respond to our comments above. Looking to get his into 0.10.1. would appreciate if you can respond. we can see how to proceed and get this in.

@prashantwason
Copy link
Member Author

prashantwason commented Jan 5, 2022

Responded to the comment above. Are there specific objections to the way this patch is implemented or the issue itself? I am open to any way to fix this issue.

@nsivabalan nsivabalan added priority:high Significant impact; potential bugs and removed priority:critical Production degraded; pipelines stalled labels Jan 10, 2022
@nsivabalan
Copy link
Contributor

nsivabalan commented Jan 10, 2022

Since we don't have a clear/concrete plan yet, I am removing it from 0.10.1. but lets brainstorm and try to get a closure.

@nsivabalan
Copy link
Contributor

nsivabalan commented Feb 7, 2022

@prashantwason : We are looking to get this in for 0.11. Would appreciate if you can follow up on this and let us know once its ready to review again.

@nsivabalan nsivabalan added the priority:critical Production degraded; pipelines stalled label Feb 8, 2022
@prashantwason
Copy link
Member Author

prashantwason commented Feb 8, 2022

@nsivabalan Your plan to not schedule clean when another is in progress works. Can you please make that change and I can close this PR? Happy to review the other PR too.

@nsivabalan nsivabalan force-pushed the pw_fix_multi_clean_OSS branch 2 times, most recently from a14bc80 to 98c3c09 Compare February 10, 2022 01:33
prashantwason
prashantwason commented Feb 10, 2022
View reviewed changes
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java Outdated
}
LOG.info("Cleaner started");
final Timer.Context timerContext = metrics.getCleanCtx();
LOG.info("Cleaned failed attempts if any");
Copy link
Member Author

@prashantwason prashantwason Feb 10, 2022

Choose a reason for hiding this comment

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

This log is not very useful and prints each time. Probably move it to within rollbackFailedWrites() if any writes are actually found to be cleaned.

Copy link
Contributor

@nsivabalan nsivabalan Feb 10, 2022

Choose a reason for hiding this comment

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

I have moved it to CleanerUtils.rollbackFailedWrites().
rollbackFailedwrites() is called in regular rollbacks as well (single writer) and not in the context of cleaner

prashantwason
prashantwason commented Feb 10, 2022
View reviewed changes
...i-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieBackedMetadata.java Outdated
* Tests no more than 1 clean is scheduled/executed if HoodieCompactionConfig.allowMultipleCleanSchedule config is disabled.
*/
@Test
public void testMultiClean() throws Exception {
Copy link
Member Author

@prashantwason prashantwason Feb 10, 2022

Choose a reason for hiding this comment

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

Is there any file which has cleaner specific tests? This does not seem metadata table related test.

Copy link
Contributor

@nsivabalan nsivabalan Feb 10, 2022

Choose a reason for hiding this comment

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

yeah. I just followed up from where you left :) will try to move it to TestCleaner.

prashantwason
prashantwason commented Feb 10, 2022
View reviewed changes
Copy link
Member Author

@prashantwason prashantwason left a comment

Choose a reason for hiding this comment

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

Looks good. A few minor comments.

@nsivabalan
Copy link
Contributor

nsivabalan commented Feb 10, 2022

@prashantwason : addressed all feedback.

prashantwason
prashantwason commented Feb 10, 2022
View reviewed changes
Copy link
Member Author

@prashantwason prashantwason left a comment

Choose a reason for hiding this comment

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

LGTM

@nsivabalan
Copy link
Contributor

nsivabalan commented Feb 11, 2022

@yihua : do you want to review. Prashant has given a ship it.

yihua
yihua reviewed Feb 14, 2022
View reviewed changes
nsivabalan
nsivabalan reviewed Feb 15, 2022
View reviewed changes
Copy link
Contributor

@nsivabalan nsivabalan left a comment

Choose a reason for hiding this comment

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

addressed all comments

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java Outdated
}
LOG.info("Cleaner started");
final Timer.Context timerContext = metrics.getCleanCtx();
LOG.info("Cleaned failed attempts if any");
Copy link
Contributor

@nsivabalan nsivabalan Feb 10, 2022

Choose a reason for hiding this comment

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

I have moved it to CleanerUtils.rollbackFailedWrites().
rollbackFailedwrites() is called in regular rollbacks as well (single writer) and not in the context of cleaner

...i-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieBackedMetadata.java Outdated
* Tests no more than 1 clean is scheduled/executed if HoodieCompactionConfig.allowMultipleCleanSchedule config is disabled.
*/
@Test
public void testMultiClean() throws Exception {
Copy link
Contributor

@nsivabalan nsivabalan Feb 10, 2022

Choose a reason for hiding this comment

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

yeah. I just followed up from where you left :) will try to move it to TestCleaner.

@nsivabalan nsivabalan force-pushed the pw_fix_multi_clean_OSS branch from 3ab4015 to 8c8eeed Compare February 15, 2022 23:30
@nsivabalan
Copy link
Contributor

nsivabalan commented Feb 17, 2022

@yihua : if you are good, can I go ahead and merge this in.

@apache apache deleted a comment from hudi-bot Feb 21, 2022
yihua
yihua approved these changes Feb 21, 2022
View reviewed changes
Copy link
Contributor

@yihua yihua left a comment

Choose a reason for hiding this comment

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

LGTM. For the config naming, let's have a consensus.

@hudi-bot
Copy link
Collaborator

hudi-bot commented Feb 22, 2022

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

@nsivabalan nsivabalan merged commit 0dee8ed into apache:master Feb 22, 2022
@nsivabalan nsivabalan added the status:triaged Issue has been reviewed and categorized label Feb 27, 2022
vingov pushed a commit to vingov/hudi that referenced this pull request Apr 3, 2022
@prashantwason @nsivabalan
[HUDI-2925] Fix duplicate cleaning of same files when unfinished clea…
8b2dc17 
…n operations are present using a config. (apache#4212)


Co-authored-by: sivabalan <n.siva.b@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nsivabalan nsivabalan nsivabalan left review comments

@danny0405 danny0405 danny0405 left review comments

@yihua yihua yihua approved these changes

@vinothchandar vinothchandar Awaiting requested review from vinothchandar

Assignees

@nsivabalan nsivabalan

@yihua yihua

Labels

priority:critical Production degraded; pipelines stalled priority:high Significant impact; potential bugs status:triaged Issue has been reviewed and categorized

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@prashantwason @nsivabalan @hudi-bot @yihua @danny0405