You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "zhengchenyu (via GitHub)" <gi...@apache.org> on 2024/04/06 11:02:15 UTC
[PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
zhengchenyu opened a new pull request, #4899:
URL: https://github.com/apache/hive/pull/4899
### What changes were proposed in this pull request?
Try canCommit after process.
### Why are the changes needed?
We should avoid speculative tasks being committed at the same time. Detailed information see: https://issues.apache.org/jira/browse/HIVE-27899
### Does this PR introduce _any_ user-facing change?
No
### Is the change a dependency upgrade?
No
### How was this patch tested?
test on our cluster
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2321658402
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
[TaskImpl::canCommit](https://github.com/apache/tez/blob/6683866ea5e9a7b900ccbcd617e49588974019ae/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java#L694) is already wrapped with `writeLock` - OK
however, if commitAttempt != taskAttemptID (speculative attempt was already committed), how are we planning to leave the loop?
````
LOG.debug("{} is current committer. Commit waiting for: {}", commitAttempt, taskAttemptID);
return false;
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324538957
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
we can also change the multiplier, say 1.2
S(13)=50⋅(1.2^14 -1)/0.2 = 3 sec until we run over 500 (a13=50⋅1.2^13≈553.9 ms)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324772328
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -185,22 +184,21 @@ public void initialize() throws IOException {
Configuration conf = TezUtils.createConfFromUserPayload(getContext().getUserPayload());
this.jobConf = new JobConf(conf);
this.jobConf.getCredentials().mergeAll(UserGroupInformation.getCurrentUser().getCredentials());
- this.processorContext = getContext();
initTezAttributes();
- ExecutionContext execCtx = processorContext.getExecutionContext();
+ ExecutionContext execCtx = getContext().getExecutionContext();
if (execCtx instanceof Hook) {
((Hook)execCtx).initializeHook(this);
}
- setupMRLegacyConfigs(processorContext);
+ setupMRLegacyConfigs(getContext());
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_INITIALIZE_PROCESSOR);
}
private void initTezAttributes() {
Review Comment:
4/8 bytes to store the object ref? it's nothing
````
Use a method parameter for data that is needed for a single, specific operation. This is transient data that defines what a particular method does, not what the object is
````
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -185,22 +184,21 @@ public void initialize() throws IOException {
Configuration conf = TezUtils.createConfFromUserPayload(getContext().getUserPayload());
this.jobConf = new JobConf(conf);
this.jobConf.getCredentials().mergeAll(UserGroupInformation.getCurrentUser().getCredentials());
- this.processorContext = getContext();
initTezAttributes();
- ExecutionContext execCtx = processorContext.getExecutionContext();
+ ExecutionContext execCtx = getContext().getExecutionContext();
if (execCtx instanceof Hook) {
((Hook)execCtx).initializeHook(this);
}
- setupMRLegacyConfigs(processorContext);
+ setupMRLegacyConfigs(getContext());
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_INITIALIZE_PROCESSOR);
}
private void initTezAttributes() {
Review Comment:
4/8 bytes to store the object ref? it's nothing
````
Use a method parameter for data that is needed for a single, specific operation.
This is transient data that defines what a particular method does, not what the object is
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324772328
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -185,22 +184,21 @@ public void initialize() throws IOException {
Configuration conf = TezUtils.createConfFromUserPayload(getContext().getUserPayload());
this.jobConf = new JobConf(conf);
this.jobConf.getCredentials().mergeAll(UserGroupInformation.getCurrentUser().getCredentials());
- this.processorContext = getContext();
initTezAttributes();
- ExecutionContext execCtx = processorContext.getExecutionContext();
+ ExecutionContext execCtx = getContext().getExecutionContext();
if (execCtx instanceof Hook) {
((Hook)execCtx).initializeHook(this);
}
- setupMRLegacyConfigs(processorContext);
+ setupMRLegacyConfigs(getContext());
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_INITIALIZE_PROCESSOR);
}
private void initTezAttributes() {
Review Comment:
4/8 bytes to store the object ref? it's nothing
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4899:
URL: https://github.com/apache/hive/pull/4899#issuecomment-3257505608
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4899) **Quality Gate passed**
Issues
 [3624 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4899&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4899&issueStatuses=ACCEPTED)
Measures
 [34 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4899&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4899&metric=new_coverage&view=list)
 [1.4% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4899&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4899)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2321658402
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
1)
[TaskImpl::canCommit](https://github.com/apache/tez/blob/6683866ea5e9a7b900ccbcd617e49588974019ae/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java#L694) is already wrapped with `writeLock` - OK
however, if `commitAttempt != taskAttemptID` (speculative attempt was already committed), how are we planning to leave the loop `while (!getContext().canCommit()) {Thread.sleep(100);}`?
````
LOG.debug("{} is current committer. Commit waiting for: {}", commitAttempt, taskAttemptID);
return false;
````
I've found additional explanation in a JIRA
````
The problem is that both speculatively executed tasks commit the file. This will not happen in the Tez
examples because they will try canCommit, which can guarantee that one and only one task attempt commit
successfully. If one task attempt executes canCommit successfully, the other one will be stuck by canCommit
until it receives a kill signal.
````
so, is it intentional that 2nd attempt would be in busy-waiting until it receives a kill signal? is it a jvm process kill?
2)
> In fact, tez example use this way to avoid commit simultaneously. I just copy from here。
I think, usage pattern there is a bit different (no speculative execution):
````
// This will loop till the AM asks for the task to be killed. As
// against, the AM sending a signal to the task to kill itself
// gracefully. The AM waits for the current committer to successfully
// complete and then kills us. Until then we wait in case the
// current committer fails and we get chosen to commit.
while (!getContext().canCommit()) {
Thread.sleep(100);
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "zhengchenyu (via GitHub)" <gi...@apache.org>.
zhengchenyu commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2323928608
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
1 about backoff?
I think we should not use backoff sleep. Because in tez source code, `TaskImpl::commitAttempt` may be reset to null due to failure, etc. Sleeping for too long will affect the overall running time. And sleep is very light.
2. For me, I think no difference. In fact, we already enable speculative execution for tez engine.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #4899: HIVE-27899: Killed speculative execution task attempt should not comm…
URL: https://github.com/apache/hive/pull/4899
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "zhengchenyu (via GitHub)" <gi...@apache.org>.
zhengchenyu closed pull request #4899: HIVE-27899: Killed speculative execution task attempt should not comm…
URL: https://github.com/apache/hive/pull/4899
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4899:
URL: https://github.com/apache/hive/pull/4899#issuecomment-1880488424
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4899) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[3 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4899&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4899&resolved=false&inNewCodePeriod=true)
No data about Coverage
No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4899)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2321658402
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
1) [TaskImpl::canCommit](https://github.com/apache/tez/blob/6683866ea5e9a7b900ccbcd617e49588974019ae/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java#L694) is already wrapped with `writeLock` - OK
however, if commitAttempt != taskAttemptID (speculative attempt was already committed), how are we planning to leave the loop `while (!this.processorContext.canCommit()) {Thread.sleep(100);}`?
````
LOG.debug("{} is current committer. Commit waiting for: {}", commitAttempt, taskAttemptID);
return false;
````
2)
> In fact, tez example use this way to avoid commit simultaneously. I just copy from here。
I think usage pattern there is a bit different (no speculative execution):
````
// This will loop till the AM asks for the task to be killed. As
// against, the AM sending a signal to the task to kill itself
// gracefully. The AM waits for the current committer to successfully
// complete and then kills us. Until then we wait in case the
// current committer fails and we get chosen to commit.
while (!getContext().canCommit()) {
Thread.sleep(100);
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2321658402
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
1)
[TaskImpl::canCommit](https://github.com/apache/tez/blob/6683866ea5e9a7b900ccbcd617e49588974019ae/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java#L694) is already wrapped with `writeLock` - OK
however, if commitAttempt != taskAttemptID (speculative attempt was already committed), how are we planning to leave the loop `while (!this.processorContext.canCommit()) {Thread.sleep(100);}`?
````
LOG.debug("{} is current committer. Commit waiting for: {}", commitAttempt, taskAttemptID);
return false;
````
2)
> In fact, tez example use this way to avoid commit simultaneously. I just copy from here。
I think usage pattern there is a bit different (no speculative execution):
````
// This will loop till the AM asks for the task to be killed. As
// against, the AM sending a signal to the task to kill itself
// gracefully. The AM waits for the current committer to successfully
// complete and then kills us. Until then we wait in case the
// current committer fails and we get chosen to commit.
while (!getContext().canCommit()) {
Thread.sleep(100);
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324126394
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
@zhengchenyu, sleep is light. what about `canCommit()` invocation every 100 msec? @abstractdog was proposing a capped back-off not to sleep too long
````
static final long MAX_SLEEP = 500;
while (!processorContext.canCommit()) {
backoff(50);
}
private void backoff(long nextSleep) {
try {
Thread.sleep(nextSleep);
} catch (InterruptedException e) {
}
if (nextSleep < MAX_SLEEP) {
nextSleep = Math.min(MAX_SLEEP, nextSleep * 2);
}
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2321658402
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
1)
[TaskImpl::canCommit](https://github.com/apache/tez/blob/6683866ea5e9a7b900ccbcd617e49588974019ae/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java#L694) is already wrapped with `writeLock` - OK
however, if `commitAttempt != taskAttemptID` (speculative attempt was already committed), how are we planning to leave the loop `while (!getContext().canCommit()) {Thread.sleep(100);}`?
````
LOG.debug("{} is current committer. Commit waiting for: {}", commitAttempt, taskAttemptID);
return false;
````
I've found additional explanation in a JIRA
````
The problem is that both speculatively executed tasks commit the file. This will not happen in the Tez
examples because they will try canCommit, which can guarantee that one and only one task attempt commit
successfully. If one task attempt executes canCommit successfully, the other one will be stuck by canCommit
until it receives a kill signal.
````
so, it is intentional that 2nd attempt would be in busy-waiting until it receives a kill signal? is it a jvm process kill?
why not use backoff?
````
private void backoff() {
nextSleep = (nextSleep > Long.MAX_VALUE / 2) ?
Long.MAX_VALUE : nextSleep * 2;
try {
Thread.sleep(nextSleep);
} catch (InterruptedException e) {
}
}
````
2)
> In fact, tez example use this way to avoid commit simultaneously. I just copy from here。
I think, usage pattern there is a bit different, but I might be wrong (no speculative execution):
````
// This will loop till the AM asks for the task to be killed. As
// against, the AM sending a signal to the task to kill itself
// gracefully. The AM waits for the current committer to successfully
// complete and then kills us. Until then we wait in case the
// current committer fails and we get chosen to commit.
while (!getContext().canCommit()) {
Thread.sleep(100);
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324126394
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
@zhengchenyu, sleep is light. what about `canCommit()` invocation every 100 msec? @abstractdog was proposing a capped back-off not to sleep too long
````
static final long MAX_SLEEP = 500;
long nextSleep = 50
while (!processorContext.canCommit()) {
nextSleep = backoff(nextSleep);
}
private void backoff(long nextSleep) {
try {
Thread.sleep(nextSleep);
} catch (InterruptedException e) {
}
if (nextSleep < MAX_SLEEP) {
nextSleep = Math.min(MAX_SLEEP, nextSleep * 2);
}
return nextSleep;
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "zhengchenyu (via GitHub)" <gi...@apache.org>.
zhengchenyu commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2323939452
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
It seems that it doesn't matter. If this is the case, I also removed the processorContext parameter passing to avoid unnecessary use of the stack.
However, the large amount of code modified in this PR has nothing to do with this PR. I don’t know if this is a good idea?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2321658402
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
1)
[TaskImpl::canCommit](https://github.com/apache/tez/blob/6683866ea5e9a7b900ccbcd617e49588974019ae/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java#L694) is already wrapped with `writeLock` - OK
however, if `commitAttempt != taskAttemptID` (speculative attempt was already committed), how are we planning to leave the loop `while (!getContext().canCommit()) {Thread.sleep(100);}`?
````
LOG.debug("{} is current committer. Commit waiting for: {}", commitAttempt, taskAttemptID);
return false;
````
I've found additional explanation in a JIRA
````
The problem is that both speculatively executed tasks commit the file. This will not happen in the Tez
examples because they will try canCommit, which can guarantee that one and only one task attempt commit
successfully. If one task attempt executes canCommit successfully, the other one will be stuck by canCommit
until it receives a kill signal.
````
so, it is intentional that 2nd attempt would be in busy-waiting until it receives a kill signal? is it a jvm process kill?
why not use backoff?
````
private void backoff() {
nextSleep = Long.min(nextSleep * 2, Long.MAX_VALUE);
try {
Thread.sleep(nextSleep);
} catch (InterruptedException e) {
}
}
````
2)
> In fact, tez example use this way to avoid commit simultaneously. I just copy from here。
I think, usage pattern there is a bit different (no speculative execution):
````
// This will loop till the AM asks for the task to be killed. As
// against, the AM sending a signal to the task to kill itself
// gracefully. The AM waits for the current committer to successfully
// complete and then kills us. Until then we wait in case the
// current committer fails and we get chosen to commit.
while (!getContext().canCommit()) {
Thread.sleep(100);
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324538957
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
we can also change the multiplier to square root and it will take 100 iterations to reach 500 ms (33.3 sec)
````
nextSleep = (long) Math.min(MAX_SLEEP, Math.sqrt(nextSleep * nextSleep + 50 * 50)));
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "zhengchenyu (via GitHub)" <gi...@apache.org>.
zhengchenyu commented on PR #4899:
URL: https://github.com/apache/hive/pull/4899#issuecomment-2212857850
@deniskuzZ @ayushtkn @abstractdog @glapark
Here we have the following problem about repetition in our production problem:
* (1) When speculative execution is enabled, I found that the cluster occasionally has file duplication problem.
When speculative execution is turned on, the slower task attempt will be killed. I found that the killed task may commit unfinished files. So I submit HIVE-25561 to solve this problem.
> Note: In fact, kill task attempt will cause this problem, Enabling speculative execution just increases the probability of killing task attempts.
* (2) When I apply HIVE-25561, I found that duplicated file still happened, but the probability was very very low.
I found that the duplication of files was caused by two different attempts of the same task being committed at the same time. So I submit HIVE-27899 to solve this problem.
* (3) After the task attempt is committed and before the appMaster receives the task success message, if an exception occurs, file duplication may occur. See for information in HIVE-27985.
We can use task-based but not attempt-based file names. Even if there are duplicate files, since the names are the same, the file committed later will delete the file committed earlier. And because HIVE-25561 is applied, we can make sure that all commit file is generated by successfull task attempt . So I submit HIVE-27986.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2321658402
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
1)
[TaskImpl::canCommit](https://github.com/apache/tez/blob/6683866ea5e9a7b900ccbcd617e49588974019ae/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java#L694) is already wrapped with `writeLock` - OK
however, if `commitAttempt != taskAttemptID` (speculative attempt was already committed), how are we planning to leave the loop `while (!getContext().canCommit()) {Thread.sleep(100);}`?
````
LOG.debug("{} is current committer. Commit waiting for: {}", commitAttempt, taskAttemptID);
return false;
````
2)
> In fact, tez example use this way to avoid commit simultaneously. I just copy from here。
I think, usage pattern there is a bit different (no speculative execution):
````
// This will loop till the AM asks for the task to be killed. As
// against, the AM sending a signal to the task to kill itself
// gracefully. The AM waits for the current committer to successfully
// complete and then kills us. Until then we wait in case the
// current committer fails and we get chosen to commit.
while (!getContext().canCommit()) {
Thread.sleep(100);
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2321658402
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
1)
[TaskImpl::canCommit](https://github.com/apache/tez/blob/6683866ea5e9a7b900ccbcd617e49588974019ae/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java#L694) is already wrapped with `writeLock` - OK
however, if `commitAttempt != taskAttemptID` (speculative attempt was already committed), how are we planning to leave the loop `while (!this.processorContext.canCommit()) {Thread.sleep(100);}`?
````
LOG.debug("{} is current committer. Commit waiting for: {}", commitAttempt, taskAttemptID);
return false;
````
2)
> In fact, tez example use this way to avoid commit simultaneously. I just copy from here。
I think, usage pattern there is a bit different (no speculative execution):
````
// This will loop till the AM asks for the task to be killed. As
// against, the AM sending a signal to the task to kill itself
// gracefully. The AM waits for the current committer to successfully
// complete and then kills us. Until then we wait in case the
// current committer fails and we get chosen to commit.
while (!getContext().canCommit()) {
Thread.sleep(100);
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2321658402
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
1)
[TaskImpl::canCommit](https://github.com/apache/tez/blob/6683866ea5e9a7b900ccbcd617e49588974019ae/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java#L694) is already wrapped with `writeLock` - OK
however, if `commitAttempt != taskAttemptID` (speculative attempt was already committed), how are we planning to leave the loop `while (!getContext().canCommit()) {Thread.sleep(100);}`?
````
LOG.debug("{} is current committer. Commit waiting for: {}", commitAttempt, taskAttemptID);
return false;
````
I've found additional explanation in a JIRA
````
The problem is that both speculatively executed tasks commit the file. This will not happen in the Tez
examples because they will try canCommit, which can guarantee that one and only one task attempt commit
successfully. If one task attempt executes canCommit successfully, the other one will be stuck by canCommit
until it receives a kill signal.
````
so, it is intentional that 2nd attempt would be in busy-waiting until it receives a kill signal? is it a jvm process kill?
why not use backoff?
````
private void backoff() {
nextSleep = (nextSleep > Long.MAX_VALUE / 2) ?
Long.MAX_VALUE : nextSleep * 2;
try {
Thread.sleep(nextSleep);
} catch (InterruptedException e) {
}
}
````
2)
> In fact, tez example use this way to avoid commit simultaneously. I just copy from here。
I think, usage pattern there is a bit different (no speculative execution):
````
// This will loop till the AM asks for the task to be killed. As
// against, the AM sending a signal to the task to kill itself
// gracefully. The AM waits for the current committer to successfully
// complete and then kills us. Until then we wait in case the
// current committer fails and we get chosen to commit.
while (!getContext().canCommit()) {
Thread.sleep(100);
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324745181
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -185,22 +184,21 @@ public void initialize() throws IOException {
Configuration conf = TezUtils.createConfFromUserPayload(getContext().getUserPayload());
this.jobConf = new JobConf(conf);
this.jobConf.getCredentials().mergeAll(UserGroupInformation.getCurrentUser().getCredentials());
- this.processorContext = getContext();
initTezAttributes();
- ExecutionContext execCtx = processorContext.getExecutionContext();
+ ExecutionContext execCtx = getContext().getExecutionContext();
if (execCtx instanceof Hook) {
((Hook)execCtx).initializeHook(this);
}
- setupMRLegacyConfigs(processorContext);
+ setupMRLegacyConfigs(getContext());
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_INITIALIZE_PROCESSOR);
}
private void initTezAttributes() {
Review Comment:
maybe parametrize with `ProcessorContext processorContext` similar to `setupMRLegacyConfigs`
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324740013
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -185,22 +184,21 @@ public void initialize() throws IOException {
Configuration conf = TezUtils.createConfFromUserPayload(getContext().getUserPayload());
this.jobConf = new JobConf(conf);
this.jobConf.getCredentials().mergeAll(UserGroupInformation.getCurrentUser().getCredentials());
- this.processorContext = getContext();
initTezAttributes();
- ExecutionContext execCtx = processorContext.getExecutionContext();
+ ExecutionContext execCtx = getContext().getExecutionContext();
if (execCtx instanceof Hook) {
((Hook)execCtx).initializeHook(this);
}
- setupMRLegacyConfigs(processorContext);
+ setupMRLegacyConfigs(getContext());
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_INITIALIZE_PROCESSOR);
}
private void initTezAttributes() {
Review Comment:
maybe add `ProcessorContext processorContext` param similar to `setupMRLegacyConfigs`
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324538957
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
we can also change the multiplier, say 1.2
S(14)=50⋅(1.2^14 -1)/0.2 = 3 sec until we run over 500 (a13=50⋅1.2^13≈553.9 ms)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324927938
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -185,22 +184,21 @@ public void initialize() throws IOException {
Configuration conf = TezUtils.createConfFromUserPayload(getContext().getUserPayload());
this.jobConf = new JobConf(conf);
this.jobConf.getCredentials().mergeAll(UserGroupInformation.getCurrentUser().getCredentials());
- this.processorContext = getContext();
initTezAttributes();
- ExecutionContext execCtx = processorContext.getExecutionContext();
+ ExecutionContext execCtx = getContext().getExecutionContext();
if (execCtx instanceof Hook) {
((Hook)execCtx).initializeHook(this);
}
- setupMRLegacyConfigs(processorContext);
+ setupMRLegacyConfigs(getContext());
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_INITIALIZE_PROCESSOR);
}
private void initTezAttributes() {
Review Comment:
````
ProcessorContext processorContext = getContext();
initTezAttributes(processorContext);
...
setupMRLegacyConfigs(processorContext);
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2293805627
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
There is no time interval that could potentially lead to starvation. Also would be nice to use some callback instead of Thread.sleep()
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2293805627
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
There is no time interval that could lead to starvation. Also would be nice to use some callback instead of Thread.sleep()
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ merged PR #4899:
URL: https://github.com/apache/hive/pull/4899
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324126394
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
@zhengchenyu, sleep is light. what about `canCommit()` invocation every 100 msec? @abstractdog was proposing a capped back-off not to sleep too long
````
static final long MAX_SLEEP = 500;
while (!processorContext.canCommit()) {
backoff();
}
private void backoff() {
long nextSleep = 50;
try {
Thread.sleep(nextSleep);
} catch (InterruptedException e) {
}
if (nextSleep < MAX_SLEEP) {
nextSleep = Math.min(MAX_SLEEP, nextSleep * 2);
}
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "zhengchenyu (via GitHub)" <gi...@apache.org>.
zhengchenyu commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324265871
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
If initsleep 50ms, max sleep =500ms. And after 50+100+200+400=0.75s, we will always use 0.5s to sleep. It doesn't seem to make much sense. How about configuring this variable? And set it to a value not less than a certain value, such as 100ms.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324538957
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
we can also change the multiplier, say 1.2
S(12)=50⋅(1.2^13 -1)/0.2 = 2,5 sec until we run over 500
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324732623
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
thanks @zhengchenyu for the explanation, it really makes sense, I agree that it's a rare edge-case that another task starts to commit, then it fails, but if it happens, the current task should react on time, so for this purpose, max. 500ms looks good, also the exponential backoff might be an overcomplicated approach, as if canCommit == false, we can expect this task to be killed anyway, and 500ms won't overload the AM, so I'm fine with Thread.sleep(500) with an extra comment above sleep call that contains your explanation
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "zhengchenyu (via GitHub)" <gi...@apache.org>.
zhengchenyu commented on PR #4899:
URL: https://github.com/apache/hive/pull/4899#issuecomment-2095079310
@deniskuzZ Hi, can you please review the [HIVE-27985](https://issues.apache.org/jira/browse/HIVE-27985)?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324126394
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
@zhengchenyu, sleep is light. what about `canCommit()` invocation every 100 msec? @abstractdog was proposing a capped back-off not to sleep too long
````
static final long MAX_SLEEP = 500;
while (!processorContext.canCommit()) {
backoff(50);
}
private void backoff(long nextSleep) {
try {
Thread.sleep(nextSleep);
} catch (InterruptedException e) {
}
if (nextSleep < MAX_SLEEP) {
nextSleep = Math.min(MAX_SLEEP, nextSleep * 2);
}
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324126394
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
@zhengchenyu, sleep is light. what about `canCommit()` invocation every 100 msec? @abstractdog was proposing a capped back-off not to sleep too long
````
long sleepMillis = 50;
final long maxSleepMillis = 500;
while (!processorContext.canCommit()) {
Thread.sleep(sleepMillis);
if (sleepMillis < maxSleepMillis) {
sleepMillis = Math.min(maxSleepMillis, sleepMillis * 2);
}
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324538957
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
we can also change the multiplier, say 1.2
S(12)=50⋅(1.2^13 -1)/0.2 = 2,5 sec
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324538957
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
fixed 0.5s looks reasonable to me
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324733562
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
constant 500 ms is fine by me
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2321658402
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
[TaskImpl::canCommit](https://github.com/apache/tez/blob/6683866ea5e9a7b900ccbcd617e49588974019ae/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java#L694) is already wrapped with `writeLock` - OK
however, if commitAttempt != taskAttemptID (speculative attempt was already committed), how are we planning to leave the loop `while (!this.processorContext.canCommit()) {Thread.sleep(100);}`?
````
LOG.debug("{} is current committer. Commit waiting for: {}", commitAttempt, taskAttemptID);
return false;
````
> In fact, tez example use this way to avoid commit simultaneously. I just copy from here。
I think usage pattern there is a bit different (no speculative execution):
````
// This will loop till the AM asks for the task to be killed. As
// against, the AM sending a signal to the task to kill itself
// gracefully. The AM waits for the current committer to successfully
// complete and then kills us. Until then we wait in case the
// current committer fails and we get chosen to commit.
while (!getContext().canCommit()) {
Thread.sleep(100);
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2321658402
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
1)
[TaskImpl::canCommit](https://github.com/apache/tez/blob/6683866ea5e9a7b900ccbcd617e49588974019ae/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java#L694) is already wrapped with `writeLock` - OK
however, if `commitAttempt != taskAttemptID` (speculative attempt was already committed), how are we planning to leave the loop `while (!getContext().canCommit()) {Thread.sleep(100);}`?
````
LOG.debug("{} is current committer. Commit waiting for: {}", commitAttempt, taskAttemptID);
return false;
````
I've found additional explanation in a JIRA
````
The problem is that both speculatively executed tasks commit the file. This will not happen in the Tez
examples because they will try canCommit, which can guarantee that one and only one task attempt commit
successfully. If one task attempt executes canCommit successfully, the other one will be stuck by canCommit
until it receives a kill signal.
````
so, it is intentional that 2nd attempt would be in busy-waiting until it receives a kill signal? is it a jvm process kill?
why not use backoff?
````
private void backoff() {
nextSleep *= 2;
if (nextSleep > MAX_SLEEP) nextSleep = MAX_SLEEP;
try {
Thread.sleep(nextSleep);
} catch (InterruptedException e) {
}
}
````
2)
> In fact, tez example use this way to avoid commit simultaneously. I just copy from here。
I think, usage pattern there is a bit different (no speculative execution):
````
// This will loop till the AM asks for the task to be killed. As
// against, the AM sending a signal to the task to kill itself
// gracefully. The AM waits for the current committer to successfully
// complete and then kills us. Until then we wait in case the
// current committer fails and we get chosen to commit.
while (!getContext().canCommit()) {
Thread.sleep(100);
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2321658402
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
1)
[TaskImpl::canCommit](https://github.com/apache/tez/blob/6683866ea5e9a7b900ccbcd617e49588974019ae/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java#L694) is already wrapped with `writeLock` - OK
however, if `commitAttempt != taskAttemptID` (speculative attempt was already committed), how are we planning to leave the loop `while (!getContext().canCommit()) {Thread.sleep(100);}`?
````
LOG.debug("{} is current committer. Commit waiting for: {}", commitAttempt, taskAttemptID);
return false;
````
I've found additional explanation in a JIRA
````
The problem is that both speculatively executed tasks commit the file. This will not happen in the Tez examples because they will try canCommit, which can guarantee that one and only one task attempt commit successfully. If one task attempt executes canCommit successfully, the other one will be stuck by canCommit until it receives a kill signal.
````
so, is it intentional that 2nd attempt would be in busy-waiting until it receives a kill signal? is it a jvm process kill?
2)
> In fact, tez example use this way to avoid commit simultaneously. I just copy from here。
I think, usage pattern there is a bit different (no speculative execution):
````
// This will loop till the AM asks for the task to be killed. As
// against, the AM sending a signal to the task to kill itself
// gracefully. The AM waits for the current committer to successfully
// complete and then kills us. Until then we wait in case the
// current committer fails and we get chosen to commit.
while (!getContext().canCommit()) {
Thread.sleep(100);
}
````
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
1)
[TaskImpl::canCommit](https://github.com/apache/tez/blob/6683866ea5e9a7b900ccbcd617e49588974019ae/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java#L694) is already wrapped with `writeLock` - OK
however, if `commitAttempt != taskAttemptID` (speculative attempt was already committed), how are we planning to leave the loop `while (!getContext().canCommit()) {Thread.sleep(100);}`?
````
LOG.debug("{} is current committer. Commit waiting for: {}", commitAttempt, taskAttemptID);
return false;
````
I've found additional explanation in a JIRA
````
The problem is that both speculatively executed tasks commit the file. This will not happen in the Tez examples because they will try canCommit, which can guarantee that one and only one task attempt commit successfully. If one task attempt executes canCommit successfully, the other one will be stuck by canCommit until it receives a kill signal.
````
so, is it intentional that 2nd attempt would be in busy-waiting until it receives a kill signal? is it a jvm process kill?
2)
> In fact, tez example use this way to avoid commit simultaneously. I just copy from here。
I think, usage pattern there is a bit different (no speculative execution):
````
// This will loop till the AM asks for the task to be killed. As
// against, the AM sending a signal to the task to kill itself
// gracefully. The AM waits for the current committer to successfully
// complete and then kills us. Until then we wait in case the
// current committer fails and we get chosen to commit.
while (!getContext().canCommit()) {
Thread.sleep(100);
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4899:
URL: https://github.com/apache/hive/pull/4899#issuecomment-2461073389
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4899:
URL: https://github.com/apache/hive/pull/4899#issuecomment-2585989436
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324538957
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
0.5s looks reasonable to me
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324772328
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -185,22 +184,21 @@ public void initialize() throws IOException {
Configuration conf = TezUtils.createConfFromUserPayload(getContext().getUserPayload());
this.jobConf = new JobConf(conf);
this.jobConf.getCredentials().mergeAll(UserGroupInformation.getCurrentUser().getCredentials());
- this.processorContext = getContext();
initTezAttributes();
- ExecutionContext execCtx = processorContext.getExecutionContext();
+ ExecutionContext execCtx = getContext().getExecutionContext();
if (execCtx instanceof Hook) {
((Hook)execCtx).initializeHook(this);
}
- setupMRLegacyConfigs(processorContext);
+ setupMRLegacyConfigs(getContext());
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_INITIALIZE_PROCESSOR);
}
private void initTezAttributes() {
Review Comment:
4/8 bytes to store the object ref? it's nothing
````
Use a method parameter over class property for data that is needed for a single, specific operation.
This is transient data that defines what a particular method does, not what the object is
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324772328
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -185,22 +184,21 @@ public void initialize() throws IOException {
Configuration conf = TezUtils.createConfFromUserPayload(getContext().getUserPayload());
this.jobConf = new JobConf(conf);
this.jobConf.getCredentials().mergeAll(UserGroupInformation.getCurrentUser().getCredentials());
- this.processorContext = getContext();
initTezAttributes();
- ExecutionContext execCtx = processorContext.getExecutionContext();
+ ExecutionContext execCtx = getContext().getExecutionContext();
if (execCtx instanceof Hook) {
((Hook)execCtx).initializeHook(this);
}
- setupMRLegacyConfigs(processorContext);
+ setupMRLegacyConfigs(getContext());
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_INITIALIZE_PROCESSOR);
}
private void initTezAttributes() {
Review Comment:
4/8 bytes to store the object ref?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324772328
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -185,22 +184,21 @@ public void initialize() throws IOException {
Configuration conf = TezUtils.createConfFromUserPayload(getContext().getUserPayload());
this.jobConf = new JobConf(conf);
this.jobConf.getCredentials().mergeAll(UserGroupInformation.getCurrentUser().getCredentials());
- this.processorContext = getContext();
initTezAttributes();
- ExecutionContext execCtx = processorContext.getExecutionContext();
+ ExecutionContext execCtx = getContext().getExecutionContext();
if (execCtx instanceof Hook) {
((Hook)execCtx).initializeHook(this);
}
- setupMRLegacyConfigs(processorContext);
+ setupMRLegacyConfigs(getContext());
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_INITIALIZE_PROCESSOR);
}
private void initTezAttributes() {
Review Comment:
4/8 bytes to store the object ref? it's nothing
````
Use a method parameter over class property for data that is needed for a single, specific operation.
This is transient data that defines what a particular method does, not what the object is
````
but i don't insist on that change
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "zhengchenyu (via GitHub)" <gi...@apache.org>.
zhengchenyu commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324794761
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -185,22 +184,21 @@ public void initialize() throws IOException {
Configuration conf = TezUtils.createConfFromUserPayload(getContext().getUserPayload());
this.jobConf = new JobConf(conf);
this.jobConf.getCredentials().mergeAll(UserGroupInformation.getCurrentUser().getCredentials());
- this.processorContext = getContext();
initTezAttributes();
- ExecutionContext execCtx = processorContext.getExecutionContext();
+ ExecutionContext execCtx = getContext().getExecutionContext();
if (execCtx instanceof Hook) {
((Hook)execCtx).initializeHook(this);
}
- setupMRLegacyConfigs(processorContext);
+ setupMRLegacyConfigs(getContext());
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_INITIALIZE_PROCESSOR);
}
private void initTezAttributes() {
Review Comment:
Do you means fix like this?
```
ProcessorContext processorContext = getContext();
setupMRLegacyConfigs(processorContext);
```
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4899:
URL: https://github.com/apache/hive/pull/4899#issuecomment-3258326972
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4899) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4899&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4899&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4899&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4899&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4899&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4899)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324126394
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
@zhengchenyu, sleep is light. what about `canCommit()` invocation every 100 msec? @abstractdog was proposing a capped back-off not to sleep too long
````
static final long MAX_SLEEP = 500;
while (!processorContext.canCommit()) {
backoff();
}
private void backoff() {
long nextSleep = 50;
try {
Thread.sleep(nextSleep);
} catch (InterruptedException e) {
}
if (nextSleep < MAX_SLEEP) {
nextSleep = Math.min(MAX_SLEEP, nextSleep * 2);
}
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324126394
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
@zhengchenyu, sleep is light. what about `canCommit()` invocation every 100 msec? @abstractdog was proposing a capped back-off not to sleep too long
````
static final long MAX_SLEEP = 500;
while (!processorContext.canCommit()) {
backoff();
}
private void backoff() {
long nextSleep = 50;
try {
Thread.sleep(nextSleep);
} catch (InterruptedException e) {
}
if (nextSleep < MAX_SLEEP) {
nextSleep = Math.min(MAX_SLEEP, nextSleep * 2);
}
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #4899: HIVE-27899: Killed speculative execution task attempt should not comm…
URL: https://github.com/apache/hive/pull/4899
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324150554
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -185,46 +184,45 @@ public void initialize() throws IOException {
Configuration conf = TezUtils.createConfFromUserPayload(getContext().getUserPayload());
this.jobConf = new JobConf(conf);
this.jobConf.getCredentials().mergeAll(UserGroupInformation.getCurrentUser().getCredentials());
- this.processorContext = getContext();
initTezAttributes();
- ExecutionContext execCtx = processorContext.getExecutionContext();
+ ExecutionContext execCtx = getContext().getExecutionContext();
if (execCtx instanceof Hook) {
((Hook)execCtx).initializeHook(this);
}
- setupMRLegacyConfigs(processorContext);
+ setupMRLegacyConfigs();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_INITIALIZE_PROCESSOR);
}
private void initTezAttributes() {
- jobConf.set(HIVE_TEZ_VERTEX_NAME, processorContext.getTaskVertexName());
- jobConf.setInt(HIVE_TEZ_VERTEX_INDEX, processorContext.getTaskVertexIndex());
- jobConf.setInt(HIVE_TEZ_TASK_INDEX, processorContext.getTaskIndex());
- jobConf.setInt(HIVE_TEZ_TASK_ATTEMPT_NUMBER, processorContext.getTaskAttemptNumber());
+ jobConf.set(HIVE_TEZ_VERTEX_NAME, getContext().getTaskVertexName());
+ jobConf.setInt(HIVE_TEZ_VERTEX_INDEX, getContext().getTaskVertexIndex());
+ jobConf.setInt(HIVE_TEZ_TASK_INDEX, getContext().getTaskIndex());
+ jobConf.setInt(HIVE_TEZ_TASK_ATTEMPT_NUMBER, getContext().getTaskAttemptNumber());
}
- private void setupMRLegacyConfigs(ProcessorContext processorContext) {
+ private void setupMRLegacyConfigs() {
Review Comment:
please keep the original param
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2321658402
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
[TaskImpl::canCommit](https://github.com/apache/tez/blob/6683866ea5e9a7b900ccbcd617e49588974019ae/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java#L694) is already wrapped with `writeLock` - OK
however, if commitAttempt != taskAttemptID (speculative attempt was already committed), how are we planning to leave the loop (`while (!this.processorContext.canCommit()) {Thread.sleep(100);}`)?
````
LOG.debug("{} is current committer. Commit waiting for: {}", commitAttempt, taskAttemptID);
return false;
````
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
[TaskImpl::canCommit](https://github.com/apache/tez/blob/6683866ea5e9a7b900ccbcd617e49588974019ae/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java#L694) is already wrapped with `writeLock` - OK
however, if commitAttempt != taskAttemptID (speculative attempt was already committed), how are we planning to leave the loop `while (!this.processorContext.canCommit()) {Thread.sleep(100);}`?
````
LOG.debug("{} is current committer. Commit waiting for: {}", commitAttempt, taskAttemptID);
return false;
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2321695315
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
can we drop this `processorContext` global var and use `getContext()`as it seems unnecessary and confusing?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324126394
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
@zhengchenyu, sleep is light. what about `canCommit()` invocation every 100 msec? @abstractdog was proposing a capped back-off not to sleep too long
````
static final long MAX_SLEEP = 500;
long nextSleep = 50
while (!processorContext.canCommit()) {
nextSleep = backoff(nextSleep);
}
private void backoff(Long nextSleep) {
try {
Thread.sleep(nextSleep);
} catch (InterruptedException e) {
}
if (nextSleep < MAX_SLEEP) {
nextSleep = Math.min(MAX_SLEEP, nextSleep * 2);
}
return nextSleep;
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324538957
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
we can also change the multiplier to square root and it will take 100 iterations to reach 500 ms (33.3 sec)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324538957
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
we can also change the multiplier to square root and it will take 100 iterations to reach 500 ms
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324740013
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -185,22 +184,21 @@ public void initialize() throws IOException {
Configuration conf = TezUtils.createConfFromUserPayload(getContext().getUserPayload());
this.jobConf = new JobConf(conf);
this.jobConf.getCredentials().mergeAll(UserGroupInformation.getCurrentUser().getCredentials());
- this.processorContext = getContext();
initTezAttributes();
- ExecutionContext execCtx = processorContext.getExecutionContext();
+ ExecutionContext execCtx = getContext().getExecutionContext();
if (execCtx instanceof Hook) {
((Hook)execCtx).initializeHook(this);
}
- setupMRLegacyConfigs(processorContext);
+ setupMRLegacyConfigs(getContext());
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_INITIALIZE_PROCESSOR);
}
private void initTezAttributes() {
Review Comment:
maybe add `ProcessorContext processorContext` param similar to `setupMRLegacyConfigs`
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324538957
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
we can also change the multiplier to square root and it will take 600+ iterations to reach 500 ms (208 sec)
````
long nextSleep = 50
...
nextSleep = (long) Math.min(MAX_SLEEP, Math.sqrt(nextSleep * nextSleep + 20 * 20)));
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "zhengchenyu (via GitHub)" <gi...@apache.org>.
zhengchenyu commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2327193310
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -185,22 +184,21 @@ public void initialize() throws IOException {
Configuration conf = TezUtils.createConfFromUserPayload(getContext().getUserPayload());
this.jobConf = new JobConf(conf);
this.jobConf.getCredentials().mergeAll(UserGroupInformation.getCurrentUser().getCredentials());
- this.processorContext = getContext();
initTezAttributes();
- ExecutionContext execCtx = processorContext.getExecutionContext();
+ ExecutionContext execCtx = getContext().getExecutionContext();
if (execCtx instanceof Hook) {
((Hook)execCtx).initializeHook(this);
}
- setupMRLegacyConfigs(processorContext);
+ setupMRLegacyConfigs(getContext());
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_INITIALIZE_PROCESSOR);
}
private void initTezAttributes() {
Review Comment:
OK, I got it and I have fix.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "zhengchenyu (via GitHub)" <gi...@apache.org>.
zhengchenyu commented on PR #4899:
URL: https://github.com/apache/hive/pull/4899#issuecomment-1827191490
@abstractdog Can you please review this PR? It is reasonable to use AM’s synchronization mechanism for commit file.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4899:
URL: https://github.com/apache/hive/pull/4899#issuecomment-1825282689
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4899)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4899&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4899&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4899&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4899&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4899&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4899&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4899&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4899&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4899&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4899&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4899&resolved=false&types=CODE_SMELL) [3 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4899&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4899&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4899&metric=duplicated_lines_density&view=list) No Duplication information
 The version of Java (11.0.8) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more [here](https://docs.sonarsource.com/sonarcloud/appendices/scanner-environment/)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4899:
URL: https://github.com/apache/hive/pull/4899#issuecomment-1986592204
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "zhengchenyu (via GitHub)" <gi...@apache.org>.
zhengchenyu commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2320617588
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
In tez source code [TaskImpl::canCommit](https://github.com/apache/tez/blob/6683866ea5e9a7b900ccbcd617e49588974019ae/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java#L694), we can see only one task attempt can commit successfully. If one attempt is committed, canCommit for other attempt will return false, here I sleep until the speculative attempt was killed.
In fact, tez example use this way to avoid commit simultaneously. I just copy from [here](https://github.com/apache/tez/blob/6683866ea5e9a7b900ccbcd617e49588974019ae/tez-mapreduce/src/main/java/org/apache/tez/mapreduce/processor/SimpleMRProcessor.java#L71)。
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "zhengchenyu (via GitHub)" <gi...@apache.org>.
zhengchenyu closed pull request #4899: HIVE-27899: Killed speculative execution task attempt should not comm…
URL: https://github.com/apache/hive/pull/4899
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2321658402
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
1)
[TaskImpl::canCommit](https://github.com/apache/tez/blob/6683866ea5e9a7b900ccbcd617e49588974019ae/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java#L694) is already wrapped with `writeLock` - OK
however, if commitAttempt != taskAttemptID (speculative attempt was already committed), how are we planning to leave the loop `while (!this.processorContext.canCommit()) {Thread.sleep(100);}`?
````
LOG.debug("{} is current committer. Commit waiting for: {}", commitAttempt, taskAttemptID);
return false;
````
2)
> In fact, tez example use this way to avoid commit simultaneously. I just copy from here。
I think, usage pattern there is a bit different (no speculative execution):
````
// This will loop till the AM asks for the task to be killed. As
// against, the AM sending a signal to the task to kill itself
// gracefully. The AM waits for the current committer to successfully
// complete and then kills us. Until then we wait in case the
// current committer fails and we get chosen to commit.
while (!getContext().canCommit()) {
Thread.sleep(100);
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4899:
URL: https://github.com/apache/hive/pull/4899#issuecomment-3262144747
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4899) **Quality Gate passed**
Issues
 [460 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4899&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4899&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4899&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4899&metric=new_coverage&view=list)
 [0.5% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4899&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4899)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324538957
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
we can also change the multiplier, say 1.5
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324538957
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
we can also change the multiplier, say 1.2
S(13)=50⋅(1.2^14 -1)/0.2 = 3 sec until we run over 500 (a13=50⋅1.2^13≈553.9 ms)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "zhengchenyu (via GitHub)" <gi...@apache.org>.
zhengchenyu commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324786472
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -295,7 +295,12 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
// If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
// or the committed task fails.
while (!getContext().canCommit()) {
- Thread.sleep(100);
+ // If canCommit returns false and fall into this loop, it means another task attempt has committed.
+ // And this task attempt is only needs to sleep for a relatively long time to wait for being killed.
+ // However, we need to avoid low-probability events: the rare case where a task attempt fails after
+ // committed, so we can't set an excessively long delay so that this task attempt could react on time.
+ // 500ms is a trade-off value.
Review Comment:
Thanks, update it.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "zhengchenyu (via GitHub)" <gi...@apache.org>.
zhengchenyu commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324689270
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
If canCommit returns false, it means another task attempt has committed. And this task attempt is likely to be killed. Therefore, we can delay it for a relatively long time for example, 500ms. In my opinion, backoff is not necessary, because in most case, this task attempt will be killed!
However, we need to avoid low-probability events: the rare case where a task attempt fails after committed, so we can't set an excessively long delay.
Thus, choosing the sleep time is crucial. It should account for low-probability events. For example, if the time from task attempt commit to task attempt failure is `t1`. Here the sleep time should be related to `t1`, `0.5 * t1` seems reasonable. Perhaps a simple sleep time of 500ms is a good choice.
This is just my personal opinion. please correct me if I'm wrong. @abstractdog @deniskuzZ
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324927938
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -185,22 +184,21 @@ public void initialize() throws IOException {
Configuration conf = TezUtils.createConfFromUserPayload(getContext().getUserPayload());
this.jobConf = new JobConf(conf);
this.jobConf.getCredentials().mergeAll(UserGroupInformation.getCurrentUser().getCredentials());
- this.processorContext = getContext();
initTezAttributes();
- ExecutionContext execCtx = processorContext.getExecutionContext();
+ ExecutionContext execCtx = getContext().getExecutionContext();
if (execCtx instanceof Hook) {
((Hook)execCtx).initializeHook(this);
}
- setupMRLegacyConfigs(processorContext);
+ setupMRLegacyConfigs(getContext());
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_INITIALIZE_PROCESSOR);
}
private void initTezAttributes() {
Review Comment:
````
ProcessorContext processorContext = getContext();
initTezAttributes(processorContext);
ExecutionContext execCtx = processorContext.getExecutionContext();
...
setupMRLegacyConfigs(processorContext);
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "zhengchenyu (via GitHub)" <gi...@apache.org>.
zhengchenyu commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2297765793
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
@deniskuzZ Sorry, I don't understand you. Do you means replace sleep with callback and do not block?
I try to enable speculative execution. The purpose of this is to avoid simultaneous commits. If a commit occurs simultaneously, the slower attempt will sleep, and then when the faster attempt completes, the slower sleep attempt will be killed, thus exiting the sleep loop. If the slower attempt does not block here, the slower attempt and the faster attempt may commit the file in the close method at the same time.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324126394
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
@zhengchenyu, sleep is light. what about `canCommit()` invocation every 100 msec? @abstractdog was proposing a capped back-off not to sleep too long
````
static final long MAX_SLEEP = 500;
long nextSleep = 50
while (!processorContext.canCommit()) {
nextSleep = backoff(nextSleep);
}
private void backoff(Long nextSleep) {
try {
Thread.sleep(nextSleep);
} catch (InterruptedException e) {
}
if (nextSleep < MAX_SLEEP) {
nextSleep = Math.min(MAX_SLEEP, nextSleep * 2);
}
return nextSleep;
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324152887
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
since you use the context in your code, I don't see why it's a bad idea to drop the redundant global var.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324538957
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
we can also change the multiplier, say 1.2
S(12)=50⋅(1.2^13 -1)/0.2 = 2,5 sec
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324538957
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
we can also change the multiplier to square root and it will take 600+ iterations to reach 500 ms (208 sec)
````
nextSleep = (long) Math.min(MAX_SLEEP, Math.sqrt(nextSleep * nextSleep + 20 * 20)));
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2318872476
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
> the purpose of this is to avoid simultaneous commits.
is it possible to order the commit requests (use queue or synchronize the execution block)? also i don't really follow this sleep retry logic used here.
we are trying to call `canCommit` until it returns 'true'. what if it never reports true (stuck process)?
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
> the purpose of this is to avoid simultaneous commits.
is it possible to order the commit requests (use queue or synchronize the execution block)? also i don't really follow this sleep retry logic used here.
we are trying to call `canCommit` until it returns 'true'. what if it never reports true (stuck process)?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324275693
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
configuring such details seems a configuration overkill to me in the initial implementation, let's just agree in reasonable values and set it, what do you think?
my idea about 50-500 was a tradeoff about small initial and then not too large final sleep, if you have more reasonable values, I'm open to ideas
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324538957
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
we can also change the multiplier to square root and it will take 100 iterations to reach 500 ms (33.3 sec)
````
nextSleep = (long) Math.min(MAX_SLEEP, Math.sqrt(nextSleep) * Math.sqrt(MAX_SLEEP))
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "zhengchenyu (via GitHub)" <gi...@apache.org>.
zhengchenyu commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324754367
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -185,22 +184,21 @@ public void initialize() throws IOException {
Configuration conf = TezUtils.createConfFromUserPayload(getContext().getUserPayload());
this.jobConf = new JobConf(conf);
this.jobConf.getCredentials().mergeAll(UserGroupInformation.getCurrentUser().getCredentials());
- this.processorContext = getContext();
initTezAttributes();
- ExecutionContext execCtx = processorContext.getExecutionContext();
+ ExecutionContext execCtx = getContext().getExecutionContext();
if (execCtx instanceof Hook) {
((Hook)execCtx).initializeHook(this);
}
- setupMRLegacyConfigs(processorContext);
+ setupMRLegacyConfigs(getContext());
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_INITIALIZE_PROCESSOR);
}
private void initTezAttributes() {
Review Comment:
hmm... The same context with setupMRLegacyConfigs. I think it is unnecessary to pass this parameter, and it wastes stack space...
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "abstractdog (via GitHub)" <gi...@apache.org>.
abstractdog commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2324772867
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -295,7 +295,12 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
// If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
// or the committed task fails.
while (!getContext().canCommit()) {
- Thread.sleep(100);
+ // If canCommit returns false and fall into this loop, it means another task attempt has committed.
+ // And this task attempt is only needs to sleep for a relatively long time to wait for being killed.
+ // However, we need to avoid low-probability events: the rare case where a task attempt fails after
+ // committed, so we can't set an excessively long delay so that this task attempt could react on time.
+ // 500ms is a trade-off value.
Review Comment:
Nit, with minor fixes:
```
// If canCommit returns false and we enter this loop, it means another task attempt has already committed.
// This task attempt only needs to sleep for a relatively long time while waiting to be killed.
// However, we must avoid a low-probability scenario: a rare case where a task attempt fails after committing.
// In that situation, the delay must not be excessively long so this attempt can still react in time.
// 500ms is chosen as a trade-off value.
```
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2321695315
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
can we drop this `processorContext` global var as it seems unnecessary and confusing?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #4899:
URL: https://github.com/apache/hive/pull/4899#discussion_r2321658402
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezProcessor.java:
##########
@@ -293,6 +293,13 @@ protected void initializeAndRunProcessor(Map<String, LogicalInput> inputs,
rproc.run();
perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.TEZ_RUN_PROCESSOR);
+
+ // Try to call canCommit to AM. If there is no other speculative attempt execute canCommit, then continue.
+ // If there are other speculative attempt execute canCommit first, then wait until the attempt is killed
+ // or the committed task fails.
+ while (!this.processorContext.canCommit()) {
Review Comment:
1)
[TaskImpl::canCommit](https://github.com/apache/tez/blob/6683866ea5e9a7b900ccbcd617e49588974019ae/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java#L694) is already wrapped with `writeLock` - OK
however, if `commitAttempt != taskAttemptID` (speculative attempt was already committed), how are we planning to leave the loop `while (!getContext().canCommit()) {Thread.sleep(100);}`?
````
LOG.debug("{} is current committer. Commit waiting for: {}", commitAttempt, taskAttemptID);
return false;
````
I've found additional explanation in a JIRA
````
The problem is that both speculatively executed tasks commit the file. This will not happen in the Tez
examples because they will try canCommit, which can guarantee that one and only one task attempt commit
successfully. If one task attempt executes canCommit successfully, the other one will be stuck by canCommit
until it receives a kill signal.
````
so, it is intentional that 2nd attempt would be in busy-waiting until it receives a kill signal? is it a jvm process kill?
why not use backoff?
````
private void backoff() {
nextSleep = LongMath.saturatedMultiply(nextSleep, 2);
try {
Thread.sleep(nextSleep);
} catch (InterruptedException e) {
}
}
````
2)
> In fact, tez example use this way to avoid commit simultaneously. I just copy from here。
I think, usage pattern there is a bit different (no speculative execution):
````
// This will loop till the AM asks for the task to be killed. As
// against, the AM sending a signal to the task to kill itself
// gracefully. The AM waits for the current committer to successfully
// complete and then kills us. Until then we wait in case the
// current committer fails and we get chosen to commit.
while (!getContext().canCommit()) {
Thread.sleep(100);
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-27899: Killed speculative execution task attempt should not comm… [hive]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4899:
URL: https://github.com/apache/hive/pull/4899#issuecomment-2334963653
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org