Allowing users to pass minionInstanceTag as a param in /tasks/schedule API #12786
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #12786 +/- ##
=============================================
- Coverage 61.75% 46.64% -15.11%
- Complexity 207 953 +746
=============================================
Files 2436 1903 -533
Lines 133233 99964 -33269
Branches 20636 16126 -4510
=============================================
- Hits 82274 46629 -35645
- Misses 44911 49940 +5029
+ Partials 6048 3395 -2653
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
vvivekiyer
left a comment
vvivekiyer
left a comment
There was a problem hiding this comment.
LGTM overall. Minor comments.
There was a problem hiding this comment.
Add @Nullable annotation here and in other places.
There was a problem hiding this comment.
Can we not add minionInstanceTag to the public method above? It's only invoked from PinotTaskRestletResource and a bunch of tests.
IMO, there are already too many scheduleTask methods making this code difficult to read.
There was a problem hiding this comment.
Yeah I started with that but I see there were 35+ places in tests where we were calling that method so I ended up with adding a new function for simplicity. Lmk if we want to remove this.
There was a problem hiding this comment.
Would prefer that to make the code readable. Feel free to leave a todo if you want to pick it up later.
|
@tibrewalpratik17 - can you please rebase ? |
tibrewalpratik17
force-pushed
the
overwrite_instance_tag_schedule_job
branch
from
f2ea514 to
2b26373
Compare
|
@siddharthteotia @vvivekiyer rebased with master. Sorry for the delay as I was on PTO. |
|
Hey @tibrewalpratik17 the changes that went into this PR may create unexpected damage as the behaviour for the method |
|
Hey @shounakmk219 thanks for pointing this out! Let me raise a fix shortly. |
|
@shounakmk219 please refer #12964 |
Jackie-Jiang
added
the
incompatible
6 participants
label:
APIfeatureThis patch accepts a
minionInstanceTagstring in/tasks/scheduleAPI to allow users to schedule the tasks on that particular instance-tag nodes. This overwrites the values provided in the table configs.We can use
/tasks/executetoo as that honors this tag in the config but using an API param seems more intuitive and uses pretty much all the configs of the already-defined table-tasks anyways.Usecase:
cc @vvivekiyer