Enhance task schedule api for single type/table support #6352
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6352 +/- ##
===========================================
- Coverage 66.44% 45.55% -20.90%
===========================================
Files 1075 1291 +216
Lines 54773 62186 +7413
Branches 8168 9027 +859
===========================================
- Hits 36396 28331 -8065
- Misses 15700 31523 +15823
+ Partials 2677 2332 -345
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| import static org.testng.Assert.assertFalse; | ||
| import static org.testng.Assert.assertNotNull; | ||
| import static org.testng.Assert.assertTrue; | ||
| import static org.testng.Assert.*; |
There was a problem hiding this comment.
Per the code style, if there are >= 5 imports, they will be combined into *
| setUpTask(); | ||
|
|
||
| return tasksScheduled; | ||
| return scheduleTasks(_pinotHelixResourceManager.getAllTables(), false); |
There was a problem hiding this comment.
why this is always false?
There was a problem hiding this comment.
This method is connected to the rest endpoint, so the caller might not be the leader controller
There was a problem hiding this comment.
ic, so we actually always assume this call is from a non-leader.
There was a problem hiding this comment.
Yes, if it is not always the leader, we need to assume it is not the leader
| if (taskTypes.contains(enabledTaskType)) { | ||
| enabledTableConfigMap.computeIfAbsent(enabledTaskType, k -> new ArrayList<>()).add(tableConfig); | ||
| } else { | ||
| LOGGER.warn("Task type: {} is not registered, cannot enable it for table: {}", enabledTaskType, |
There was a problem hiding this comment.
Is it possible that we return this info to the client-side? I feel this information is useful for users to test and debug.
There was a problem hiding this comment.
Good point. We cannot directly return the message without backward-incompatible change on the rest endpoint (currently returns the map from task type to task scheduled). I made the change so that the returned map contains all the task types within the table configs, and if the task type is not registered, there will be no task scheduled (value is null for the task type), and then user can look into the log and find the reason.
There was a problem hiding this comment.
true, maybe we add a new array field like "errors" later to hold these information.
Jackie-Jiang
force-pushed
the
minion_task_schedule_api
branch
from
1a3f264
to
6189870
Compare
Jackie-Jiang
force-pushed
the
minion_task_schedule_api
branch
from
6189870
to
8fc87a5
Compare
Description
Added 2 optional query parameters to the task schedule API to schedule tasks for the given task type/table.
Examples: