[AMORO-4040] Change default value of optimizer.task-execute-timeout t… #4042
Conversation
…o Long.MAX_VALUE ns
github-actions
bot
added
type:docs
klion26
left a comment
klion26
left a comment
There was a problem hiding this comment.
Thanks for the contribution, left some comments, please let me know what do you think about them.
| .defaultValue(Duration.ofSeconds(30)) | ||
| .withDescription("Timeout duration for task acknowledgment."); | ||
|
|
||
| // defaultValue must be in nanoseconds; otherwise, TimeUtils.formatWithHighestUnit will fail with |
There was a problem hiding this 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
There was a problem hiding this comment.
Could we use the TimeUnit::Second? (may be with Int.MAX_VALUE), Fom my perspective, seconds are used more often than nanoseconds.
There was a problem hiding this comment.
That makes sense.
| } | ||
| } | ||
|
|
||
| private boolean isTaskExecTimeout(TaskRuntime<?> task) { |
There was a problem hiding this comment.
Thanks for extracting the common logic
davedwwang
force-pushed
the
amoro-4040
branch
from
e35dc2e to
18890da
Compare
|
The change LGTM, could you please resolve the conflict, so that we can merge it in. thanks |
2 participants
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