You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@amoro.apache.org by "davedwwang (via GitHub)" <gi...@apache.org> on 2026/01/15 07:12:47 UTC
[PR] [AMORO-4040] Change default value of optimizer.task-execute-timeout t… [amoro]
davedwwang opened a new pull request, #4042:
URL: https://github.com/apache/amoro/pull/4042
## Why are the changes needed?
Close #4040.
## Brief change log
Update the default value of OPTIMIZER_TASK_EXECUTE_TIMEOUT from Duration.ofHours(1) to Duration.ofMillis(Long.MAX_VALUE) ns
-
## How was this patch tested?
- [ x] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [x ] Run test locally before making a pull request
## Documentation
- Does this pull request introduce a new feature? (no)
- If yes, how is the feature documented? (docs)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4040] Change default value of optimizer.task-execute-timeout t… [amoro]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 merged PR #4042:
URL: https://github.com/apache/amoro/pull/4042
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4040] Change default value of optimizer.task-execute-timeout t… [amoro]
Posted by "davedwwang (via GitHub)" <gi...@apache.org>.
davedwwang commented on PR #4042:
URL: https://github.com/apache/amoro/pull/4042#issuecomment-3753317560
hello @klion26 @Jzjsnow Please take a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4040] Change default value of optimizer.task-execute-timeout t… [amoro]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on PR #4042:
URL: https://github.com/apache/amoro/pull/4042#issuecomment-3787853811
The change LGTM, could you please resolve the conflict, so that we can merge it in. thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4040] Change default value of optimizer.task-execute-timeout t… [amoro]
Posted by "davedwwang (via GitHub)" <gi...@apache.org>.
davedwwang commented on code in PR #4042:
URL: https://github.com/apache/amoro/pull/4042#discussion_r2703910097
##########
amoro-ams/src/main/java/org/apache/amoro/server/AmoroManagementConf.java:
##########
@@ -474,13 +474,14 @@ public class AmoroManagementConf {
.durationType()
.defaultValue(Duration.ofSeconds(30))
.withDescription("Timeout duration for task acknowledgment.");
-
+ // defaultValue must be in nanoseconds; otherwise, TimeUtils.formatWithHighestUnit will fail with
Review Comment:
That makes sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4040] Change default value of optimizer.task-execute-timeout t… [amoro]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on PR #4042:
URL: https://github.com/apache/amoro/pull/4042#issuecomment-3809157900
Merging, thanks @davedwwang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4040] Change default value of optimizer.task-execute-timeout t… [amoro]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #4042:
URL: https://github.com/apache/amoro/pull/4042#discussion_r2697320987
##########
amoro-ams/src/main/java/org/apache/amoro/server/DefaultOptimizingService.java:
##########
@@ -543,11 +542,16 @@ private Predicate<TaskRuntime<?>> buildSuspendingPredication(Set<String> activeT
&& task.getStatus() != TaskRuntime.Status.SUCCESS
|| task.getStatus() == TaskRuntime.Status.SCHEDULED
&& task.getStartTime() + taskAckTimeout < System.currentTimeMillis()
- || task.getStatus() == TaskRuntime.Status.ACKED
- && task.getStartTime() + taskExecuteTimeout < System.currentTimeMillis();
+ || isTaskExecTimeout(task);
}
}
+ private boolean isTaskExecTimeout(TaskRuntime<?> task) {
Review Comment:
Thanks for extracting the common logic
##########
amoro-ams/src/main/java/org/apache/amoro/server/AmoroManagementConf.java:
##########
@@ -474,13 +474,14 @@ public class AmoroManagementConf {
.durationType()
.defaultValue(Duration.ofSeconds(30))
.withDescription("Timeout duration for task acknowledgment.");
-
+ // defaultValue must be in nanoseconds; otherwise, TimeUtils.formatWithHighestUnit will fail with
Review Comment:
IIUC we use `Duration.ofNanos(Long.MAX_VALUE)` instead of `Duration.ofHours(xx)`(or others) because they will overflow, if this is true, maybe we don't need this comment, or we can move it to the L482
##########
amoro-ams/src/main/java/org/apache/amoro/server/AmoroManagementConf.java:
##########
@@ -474,13 +474,14 @@ public class AmoroManagementConf {
.durationType()
.defaultValue(Duration.ofSeconds(30))
.withDescription("Timeout duration for task acknowledgment.");
-
+ // defaultValue must be in nanoseconds; otherwise, TimeUtils.formatWithHighestUnit will fail with
Review Comment:
Could we use the TimeUnit::Second? (may be with Int.MAX_VALUE), Fom my perspective, seconds are used more often than nanoseconds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4040] Change default value of optimizer.task-execute-timeout t… [amoro]
Posted by "davedwwang (via GitHub)" <gi...@apache.org>.
davedwwang commented on PR #4042:
URL: https://github.com/apache/amoro/pull/4042#issuecomment-3805010296
> The change LGTM, could you please resolve the conflict, so that we can merge it in. thanks
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org