You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "nsivabalan (via GitHub)" <gi...@apache.org> on 2023/01/23 21:39:54 UTC
[GitHub] [hudi] nsivabalan opened a new pull request, #7739: [HUDI-5593] Fixing deadlocks due to async cleaner interplay w/ main thread
nsivabalan opened a new pull request, #7739:
URL: https://github.com/apache/hudi/pull/7739
### Change Logs
In post commit in WriteClient, main thread awaits for async cleaner if async clean is enabled. This code block is already within a lock. So, if multi-writer locks are enabled, and if main thread has already acquired the lock, async cleaner may not be able to acquire the lock and it will keep timing out. This was the main reason for our CI tests to keep timing our frequently. Especially we also auto enable lock provider configs for deltastreamer in continuous mode w/ async table services, some of these tests are impacted.
### Impact
No deadlocks due to locks when async cleaner is invoked.
### Risk level (write none, low medium or high below)
medium.
### Documentation Update
_Describe any necessary documentation update if there is any new feature, config, or user-facing change_
N/A
### Contributor's checklist
- [ ] Read through [contributor's guide](https://hudi.apache.org/contribute/how-to-contribute)
- [ ] Change Logs and Impact were stated clearly
- [ ] Adequate tests were added if applicable
- [ ] CI passed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] codope merged pull request #7739: [HUDI-5593] Fixing deadlocks due to async cleaner interplay w/ main thread
Posted by "codope (via GitHub)" <gi...@apache.org>.
codope merged PR #7739:
URL: https://github.com/apache/hudi/pull/7739
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] hudi-bot commented on pull request #7739: [HUDI-5593] Fixing deadlocks due to async cleaner interplay w/ main thread
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #7739:
URL: https://github.com/apache/hudi/pull/7739#issuecomment-1401112366
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "afb4305f23ca9b2c35a4d614a7bbc4badf9d26c0",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14570",
"triggerID" : "afb4305f23ca9b2c35a4d614a7bbc4badf9d26c0",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* afb4305f23ca9b2c35a4d614a7bbc4badf9d26c0 Azure: [PENDING](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14570)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] nsivabalan commented on a diff in pull request #7739: [HUDI-5593] Fixing deadlocks due to async cleaner interplay w/ main thread
Posted by "nsivabalan (via GitHub)" <gi...@apache.org>.
nsivabalan commented on code in PR #7739:
URL: https://github.com/apache/hudi/pull/7739#discussion_r1084939951
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -514,21 +516,26 @@ public void preWrite(String instantTime, WriteOperationType writeOperationType,
* @param metadata Commit Metadata corresponding to committed instant
* @param instantTime Instant Time
* @param extraMetadata Additional Metadata passed by user
- * @param acquireLockForArchival true if lock has to be acquired for archival. false otherwise.
*/
- protected void postCommit(HoodieTable table, HoodieCommitMetadata metadata, String instantTime, Option<Map<String, String>> extraMetadata,
- boolean acquireLockForArchival) {
+ protected void postCommit(HoodieTable table, HoodieCommitMetadata metadata, String instantTime, Option<Map<String, String>> extraMetadata) {
try {
// Delete the marker directory for the instant.
WriteMarkersFactory.get(config.getMarkersType(), table, instantTime)
.quietDeleteMarkerDir(context, config.getMarkersDeleteParallelism());
Review Comment:
I need to think thru this. lets land this patch for now to unblock CI. by tmrw will get back to this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] hudi-bot commented on pull request #7739: [HUDI-5593] Fixing deadlocks due to async cleaner interplay w/ main thread
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #7739:
URL: https://github.com/apache/hudi/pull/7739#issuecomment-1401106603
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "afb4305f23ca9b2c35a4d614a7bbc4badf9d26c0",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "afb4305f23ca9b2c35a4d614a7bbc4badf9d26c0",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* afb4305f23ca9b2c35a4d614a7bbc4badf9d26c0 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] nsivabalan commented on pull request #7739: [HUDI-5593] Fixing deadlocks due to async cleaner interplay w/ main thread
Posted by "nsivabalan (via GitHub)" <gi...@apache.org>.
nsivabalan commented on PR #7739:
URL: https://github.com/apache/hudi/pull/7739#issuecomment-1401544654
CI is green
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] nsivabalan commented on a diff in pull request #7739: [HUDI-5593] Fixing deadlocks due to async cleaner interplay w/ main thread
Posted by "nsivabalan (via GitHub)" <gi...@apache.org>.
nsivabalan commented on code in PR #7739:
URL: https://github.com/apache/hudi/pull/7739#discussion_r1084597880
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java:
##########
@@ -308,26 +308,6 @@ public boolean scheduleClusteringAtInstant(String instantTime, Option<Map<String
return scheduleTableService(instantTime, extraMetadata, TableServiceType.CLUSTER).isPresent();
}
- /**
- * Schedules a new cleaning instant.
Review Comment:
NTR (not to reviewer): these methods are not used. removing dead code.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java:
##########
@@ -527,16 +507,31 @@ protected void writeTableMetadata(HoodieTable table, String instantTime, String
*
* @param cleanInstantTime instant time for clean.
* @param scheduleInline true if needs to be scheduled inline. false otherwise.
- * @param skipLocking if this is triggered by another parent transaction, locking can be skipped.
*/
@Nullable
+ @Deprecated
Review Comment:
NTR: have deprecated the methods which was taking in "skipLocking" as an argument as we don't need it anymore.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java:
##########
@@ -670,23 +664,18 @@ protected Map<String, Option<HoodiePendingRollbackInfo>> getPendingRollbackInfos
* @return true if rollback was triggered. false otherwise.
*/
protected Boolean rollbackFailedWrites() {
- return rollbackFailedWrites(false);
- }
-
- /**
- * Rollback all failed writes.
- * @param skipLocking if this is triggered by another parent transaction, locking can be skipped.
- * @return true if rollback was triggered. false otherwise.
- */
- protected Boolean rollbackFailedWrites(boolean skipLocking) {
HoodieTable table = createTable(config, hadoopConf);
List<String> instantsToRollback = getInstantsToRollback(table.getMetaClient(), config.getFailedWritesCleanPolicy(), Option.empty());
Map<String, Option<HoodiePendingRollbackInfo>> pendingRollbacks = getPendingRollbackInfos(table.getMetaClient());
instantsToRollback.forEach(entry -> pendingRollbacks.putIfAbsent(entry, Option.empty()));
- rollbackFailedWrites(pendingRollbacks, skipLocking);
+ rollbackFailedWrites(pendingRollbacks);
return !pendingRollbacks.isEmpty();
}
+ protected void rollbackFailedWrites(Map<String, Option<HoodiePendingRollbackInfo>> instantsToRollback) {
+ rollbackFailedWrites(instantsToRollback, false);
+ }
+
Review Comment:
NTR:
rollbackFailedWrites in L679, is being invoked from upgrade code path which is within lock. so, could not deprecate this method to avoid "skipLocking"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] hudi-bot commented on pull request #7739: [HUDI-5593] Fixing deadlocks due to async cleaner interplay w/ main thread
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #7739:
URL: https://github.com/apache/hudi/pull/7739#issuecomment-1401287459
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "afb4305f23ca9b2c35a4d614a7bbc4badf9d26c0",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14570",
"triggerID" : "afb4305f23ca9b2c35a4d614a7bbc4badf9d26c0",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* afb4305f23ca9b2c35a4d614a7bbc4badf9d26c0 Azure: [FAILURE](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14570)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] codope commented on a diff in pull request #7739: [HUDI-5593] Fixing deadlocks due to async cleaner interplay w/ main thread
Posted by "codope (via GitHub)" <gi...@apache.org>.
codope commented on code in PR #7739:
URL: https://github.com/apache/hudi/pull/7739#discussion_r1084809906
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -514,21 +516,26 @@ public void preWrite(String instantTime, WriteOperationType writeOperationType,
* @param metadata Commit Metadata corresponding to committed instant
* @param instantTime Instant Time
* @param extraMetadata Additional Metadata passed by user
- * @param acquireLockForArchival true if lock has to be acquired for archival. false otherwise.
*/
- protected void postCommit(HoodieTable table, HoodieCommitMetadata metadata, String instantTime, Option<Map<String, String>> extraMetadata,
- boolean acquireLockForArchival) {
+ protected void postCommit(HoodieTable table, HoodieCommitMetadata metadata, String instantTime, Option<Map<String, String>> extraMetadata) {
try {
// Delete the marker directory for the instant.
WriteMarkersFactory.get(config.getMarkersType(), table, instantTime)
.quietDeleteMarkerDir(context, config.getMarkersDeleteParallelism());
Review Comment:
Why should this be done inside the lock if commit has already happened?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org