-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Refactor alter job process #1613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| } | ||
|
|
||
| protected void runPendingJob() { | ||
| throw new NotImplementedException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making the AlterJobV2 to a abstract class is more reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will change it after all test are done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
| // send all tasks and wait them finished | ||
| AgentTaskQueue.addBatchTask(batchTask); | ||
| AgentTaskExecutor.submit(batchTask); | ||
| // max timeout is 30 seconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
30 seconds is certainly enough? if the table has 1000 partitions and each partition has 100 buckets and only 10 BE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should change it to a configuration. And default value is 30 sec.
Because I don't want to stuck in here for a long time if error happens. But for
the extreme case like you said, user can always modify the config to satisfy their case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. configuration is OK, like create table logic.
ec102bd to
609b0fa
Compare
572e44d to
cf70586
Compare
ISSUE: #1429