[HUDI-4062] Only rollback the failed writes pre upgrade under optimis… #5535

danny0405 · 2022-05-09T08:04:16Z

…tic concurrency

Tips

Thank you very much for contributing to Apache Hudi.
Please review https://hudi.apache.org/contribute/how-to-contribute before opening a pull request.

What is the purpose of the pull request

(For example: This pull request adds quick-start document.)

Brief change log

(for example:)

Modify AnnotationLocation checkstyle rule in checkstyle.xml

Verify this pull request

(Please pick either of the following options)

This pull request is a trivial rework / code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end.
Added HoodieClientWriteTest to verify the change.
Manually verified the change by running a job locally.

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.

danny0405 · 2022-05-09T08:08:24Z

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java

-      Map<String, Option<HoodiePendingRollbackInfo>> pendingRollbacks = getPendingRollbackInfos(metaClient);
-      instantsToRollback.forEach(entry -> pendingRollbacks.putIfAbsent(entry, Option.empty()));
+        Map<String, Option<HoodiePendingRollbackInfo>> pendingRollbacks = getPendingRollbackInfos(metaClient);
+        instantsToRollback.forEach(entry -> pendingRollbacks.putIfAbsent(entry, Option.empty()));


The pre-upgrade rollback was introduced in #2374, with a pre-condition that when the write concurrency mode is based on optimistic lock.

While in #4114, the pre-condition was removed, which i think is a regression because we already do the rollback when starting new instant/commit before write. In single writer mode, there is no need to do redundant rollback again before upgrade.

@n3nash : change is ok with me. Do you see any issues as such.
CC @alexeykudinkin

One case that I can think of is when the CLEAN_FAILED_FILES policy is set to LAZY and writer config is set to a single writer - I believe there can be older inflight files lying around. In this case if the rollback is done only for Optimistic Concurrency, some older inflight metadata files may be lying around.

some older inflight metadata files may be lying around

That's true, but what is the problem here in single writer upgrade ? It seems that the problem is not about upgrade but about the metadata table rollback, but shouldn't the metadata table also has MVCC/SI transaction semantics here ? The metadata table reader can not see these inflight meta logs right ?

@danny0405 I'm not fully following your comment. From this PR, what I gather is that we want to special case rollback for Optimistic Concurrency only since for Single Writer this will be redundant given rollback is supposed to happen at the start of the commit. I'm merely saying that's not necessarily the case due to the LAZY cleaning policy, does that make sense ?

You mean for LAZY cleaning strategy, we do not rollback failed commits at the start of commits. I totally agree with that.

I'm actually confused here whether there are any issues here for metadata table rollback for single-writer mode, if there is no problem, then this patch is an improvement ?

hey @danny0405 : I am confused as to whats the regression here. I thought this is more of an optimization. From your first comment "While in #4114, the pre-condition was removed, which i think is a regression because we already do the rollback when starting new instant/commit before write. In single writer mode, there is no need to do redundant rollback again before upgrade."

can you help me understand why would be do redundant rollback. once we rolledback a given commit, next time if we check if something needs to be rolled back, the same partially failed commit should not appear right. I mean, if the rollback was complete the first time, it should be be part of the list of commits to rollback the 2nd time right. or am I missing something here.

Hi @nsivabalan , i agree with your point, but at least, there is no need for redundant metadata checking if the rollback strategy is EAGER.

And still, no one answers the question why we need the rollback (for data set files and metadata files) before upgrade ? What is exactly the purpose here ?

@n3nash say that it is because we need to rollback the metadata table if it is enabled, but we can not see any clues that the rollback is only for metadata. And even if we do not rollback the metadata, the content read from the metadata should still be correct based on that the metadata table itself is a Hudi table and has SI visibility ?

Seems it is clear that the rollback here is not for data set files, correct me if i'm wrong.

Hello @nsivabalan @n3nash , i have refactored the code a little to

move the failed writes cleaning all together to when starting the new commit

the cleaning strategy for commit actions as tweaked: force to clean if metadata table is enabled in case we commit metadata table successfully but data set table failed

Please review again if you have time, thanks so much in advance ~

I analyze the code a little and find that we would do the metadata table sync when it is initialized, so for the case "commit metadata successfully but data set table failed", it should be fine.

Also i didn't find any upgrade/downgrade code that needs the data set/metadata files, from my perspective, the rollback is totally unnecessary, so i just remove it here.

danny0405 · 2022-05-09T08:09:12Z

@manojpec @n3nash Can you please confirm this change ?

danny0405 · 2022-05-09T08:11:09Z

Also cc @alexeykudinkin , can you review if you have time :)

nsivabalan

lets wait to hear from nishith

hudi-bot · 2022-05-19T14:23:28Z

CI report:

35eb107 Azure: FAILURE

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

alexeykudinkin · 2022-05-19T21:22:54Z

@danny0405 can you please update PR description to capture the intent of this change. Adding context to the description helps tremendously in the review and beyond to understand the original intent.

alexeykudinkin · 2022-05-19T21:28:31Z

@danny0405 let's take a step back here: can you please outline what's the problem we're trying to solve for here?

Do you see any issues introduced by #4114 or is this purely targeting an optimization?
If it's an optimization, can you please outline what's the end goal of this optimization?

danny0405 · 2022-05-20T01:52:09Z

It is a optimization, but i think i made the logic more complicated, so i will close this PR.

danny0405 commented May 9, 2022

View reviewed changes

danny0405 mentioned this pull request May 10, 2022

[HUDI-3365] Make sure Metadata Table records are updated appropriately on HDFS #4739

Merged

5 tasks

nsivabalan self-assigned this May 10, 2022

nsivabalan reviewed May 11, 2022

View reviewed changes

nsivabalan added the priority:critical Production degraded; pipelines stalled label May 11, 2022

danny0405 force-pushed the HUDI-4062 branch 2 times, most recently from 30ac93b to 35a1fed Compare May 19, 2022 07:52

[HUDI-4062] Remove the failed writes rollback pre upgrade

35eb107

danny0405 force-pushed the HUDI-4062 branch from 35a1fed to 35eb107 Compare May 19, 2022 12:39

danny0405 closed this May 20, 2022

[HUDI-4062] Only rollback the failed writes pre upgrade under optimis… #5535

[HUDI-4062] Only rollback the failed writes pre upgrade under optimis… #5535

Uh oh!

Conversation

danny0405 commented May 9, 2022

Tips

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danny0405 commented May 9, 2022

Uh oh!

danny0405 commented May 9, 2022

Uh oh!

nsivabalan left a comment

Choose a reason for hiding this comment

Uh oh!

hudi-bot commented May 19, 2022

CI report:

Uh oh!

alexeykudinkin commented May 19, 2022

Uh oh!

alexeykudinkin commented May 19, 2022

Uh oh!

danny0405 commented May 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants