Move resource managemnt to be the responsibility of the TaskRunner#1953
Conversation
|
Net reduction in lines... SCORE |
|
I'll rebase after preliminary review. |
There was a problem hiding this comment.
similar to getWorkers() , getScalingStats() is also tied to some specific TaskRunners and ScalingStat looks like related much more to autoscaling . not sure if we are introducing same kind of coupling again via this.
There was a problem hiding this comment.
My issue here is how to handle io.druid.indexing.overlord.http.OverlordResource#getScalingState
This is intended to be a baby step
|
looks OK given that there is a need for this. I am assuming you need this to implement "Tiered" TaskRunner . |
|
looks good to me, will check again when rebased. |
drcrallen
force-pushed
the
taskRunnerResourceManagement
branch
from
aa6860e to
5713ae9
Compare
|
@nishantmonu51 / @himanshug Fixed merge conflicts |
There was a problem hiding this comment.
will revert this. It modifies the pending task exec from non-daemon to daemon
drcrallen
force-pushed
the
taskRunnerResourceManagement
branch
from
5713ae9 to
4a6b4f1
Compare
|
@nishantmonu51 / @himanshug Fixed merge conflicts any other comments? |
|
I would like to look this over before this gets merged. We choose to move the resource management out of the TaskRunner for several reasons and this PR is moving it back. |
|
@fjy but this PR has been up for 2 months, and I have mentioned it multiple times in dev syncs. If you are going to review it please do it promptly. |
There was a problem hiding this comment.
isn't above code already done in INITIALIZED and is duplicate ? possibly introduced while fixing conflicts ?
There was a problem hiding this comment.
probably, yes. fixing
|
looks pretty good to me, only few minor nitpicks and #1953 (comment) |
drcrallen
force-pushed
the
taskRunnerResourceManagement
branch
from
4a6b4f1 to
976d4c9
Compare
|
@nishantmonu51 fixed comments |
|
I think moving resource mgmt back to the taskrunner is fine. At first we were thinking that we might replace remote task runner with something else, but if task tiering is blocking on this, I am fine with the changes. @gianm any thoughts? |
|
👍 |
|
This looks good to me, I think it makes sense for the autoscaling to be folded into the task runner, since in practice they ended up being really tightly coupled anyway. Removing getWorkers from the interface makes sense to me too since that was something that only made sense for the RTR. 👍 |
Move resource managemnt to be the responsibility of the TaskRunner
6 participants
As per https://groups.google.com/d/msg/druid-development/G36t3TvbuhQ/jFB_pe-XFAAJ