-
Notifications
You must be signed in to change notification settings - Fork 2.4k
[HUDI-7507] Fixing Rollbacks and Cleaning to acquire locks as needed #13064
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.
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
[HUDI-7507] Fixing Rollbacks and Cleaning to acquire locks as needed #13064
Conversation
…s before adding new rollback requested to timeline
...mmon/src/main/java/org/apache/hudi/table/action/rollback/BaseRollbackPlanActionExecutor.java
Show resolved
Hide resolved
...-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanActionExecutor.java
Outdated
Show resolved
Hide resolved
...-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanActionExecutor.java
Outdated
Show resolved
Hide resolved
...mmon/src/main/java/org/apache/hudi/table/action/rollback/BaseRollbackPlanActionExecutor.java
Outdated
Show resolved
Hide resolved
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java
Outdated
Show resolved
Hide resolved
...nt/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java
Show resolved
Hide resolved
@@ -153,6 +156,7 @@ protected HoodieTable(HoodieWriteConfig config, HoodieEngineContext context, Hoo | |||
this.index = getIndex(config, context); | |||
this.storageLayout = getStorageLayout(config); | |||
this.taskContextSupplier = context.getTaskContextSupplier(); | |||
this.txnManager = new TransactionManager(config, metaClient.getStorage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we introduce txnManager into hoodie table? It looks like it is only used for rollback, can we move it out?
...mmon/src/main/java/org/apache/hudi/table/action/rollback/BaseRollbackPlanActionExecutor.java
Outdated
Show resolved
Hide resolved
hey @danny0405 :
May main concern is the change in public apis like HoodieTable.compact(), HoodieTable.logCompact(), HoodieTable.rollback*() APIs. Attaching pic on the APIs we have in HoodieTable : For Flink specifically, I might need some help as well (whether CompactionCommitSink is the right place to introduce the TransactionManager or not). Also, I was looking at places where we instantiate TransactionManager, looks like we don't have it clean either. We have many usages in ActionExecutors which are expected to be called from Hoodie*Table. So, not sure if its worth cleaning it up in this patch where we are looking to fix rollback scheduling. This could be much larger scope if we wanted to clean up the instantiations of TransactionManager to standardize it and keep it at either WriteClient or TableServiceClient. Let me know what do you think. |
The scheduling of clean and rollback should be already separeted from other write APIs I think, maybe just add the fresh new txn manager for these 3 APIs should be good already. |
@danny0405 : ready for review based on our sync up yesterday. |
Change Logs
This is targetted against 0.x branch.
Impact
Robust rollback planning even in the event of concurrent writers/planners.
Risk level (write none, low medium or high below)
low
Documentation Update
Describe any necessary documentation update if there is any new feature, config, or user-facing change. If not, put "none".
ticket number here and follow the instruction to make
changes to the website.
Contributor's checklist