Reduce default max # of subTasks to 1 for native parallel task by jihoonson · Pull Request #7181 · apache/druid

jihoonson · 2019-03-02T07:46:58Z

The unlimited maxNumSubTasks is easy to spawn a huge number of sub tasks. This PR is to reduce its default value to 1, so that people would think about how many tasks they want. When it's 1, the supervisor task prints a warning and does ingestion job on its own instead of spawning sub tasks.

I also added a section of 'Capacity Planning' to give a brief idea about how to set maxNumSubTasks properly.

fjy · 2019-03-02T19:22:17Z

 |pushTimeout|Milliseconds to wait for pushing segments. It must be >= 0, where 0 means to wait forever.|0|no|
 |segmentWriteOutMediumFactory|Segment write-out medium to use when creating segments. See [SegmentWriteOutMediumFactory](#segmentWriteOutMediumFactory).|Not specified, the value from `druid.peon.defaultSegmentWriteOutMediumFactory.type` is used|no|
-|maxNumSubTasks|Maximum number of tasks which can be run at the same time.|Integer.MAX_VALUE|no|
+|maxNumSubTasks|Maximum number of tasks which can be run at the same time. The supervisor task would spawn sub tasks up to `maxNumSubTasks` regardless of the reamining task slots. If this value is set to too large, too many sub tasks can be created which might block other ingestion.|2|no|


Thanks. I'll fix this and add a new section to emphasize the importance of setting this value properly.

Added Capacity Planning section.

Since the value is so critical and tough to set automatically, how about setting it to 1 by default and logging a warning in the task log when it's not raised higher? That way users are more likely to notice it if they didn't read the documentation.

I thought about it, but set to 2 because I’m thinking to make the supervisor task do the ingestion job on its own instead of spawning sub tasks if this is 1.

What do you think about more aggressive way like making this option mandatory and throwing error if it’s missing?

I thought about it, but set to 2 because I’m thinking to make the supervisor task do the ingestion job on its own instead of spawning sub tasks if this is 1.

If this is the case, I think 1 is an even better default.

What do you think about more aggressive way like making this option mandatory and throwing error if it’s missing?

I think there should be some kind of default (we want to minimize the # of mandatory parameters) and at least "1" + a warning is something where it's fairly clear what's going on if you forgot to set it.

Default of 1 with a warning message that tells the user to set "maxNumSubTasks" if they want parallelism sounds good to me.

Changed default to 1 and added a warning. I also changed the behavior of default to the sequential mode to emphasize the importance.

clintropolis

Also will need to update PR title and description if default is now 1

clintropolis · 2019-03-05T02:43:33Z

 |pushTimeout|Milliseconds to wait for pushing segments. It must be >= 0, where 0 means to wait forever.|0|no|
 |segmentWriteOutMediumFactory|Segment write-out medium to use when creating segments. See [SegmentWriteOutMediumFactory](#segmentWriteOutMediumFactory).|Not specified, the value from `druid.peon.defaultSegmentWriteOutMediumFactory.type` is used|no|
-|maxNumSubTasks|Maximum number of tasks which can be run at the same time.|Integer.MAX_VALUE|no|
+|maxNumSubTasks|Maximum number of tasks which can be run at the same time. The supervisor task would spawn worker tasks up to `maxNumSubTasks` regardless of the available task slots. If this value is set to too large, too many worker tasks can be created which might block other ingestion. Check [Capacity Planning](#capacity-planning) for more details.|2|no|


Is default 1 now?

Ah sorry. Fixed.

jihoonson · 2019-03-05T21:21:49Z

I fixed a wrong if clause in ParallelIndexSupervisorTask.run() and added a unit test.

jihoonson · 2019-03-05T23:16:26Z

Fixed the failing IT test.

…e#7181) * Reduce # of max subTasks to 2 * fix typo and add more doc * add more doc and link * change default and add warning * fix doc * add test * fix it test

#7200) * Reduce # of max subTasks to 2 * fix typo and add more doc * add more doc and link * change default and add warning * fix doc * add test * fix it test

glasser · 2019-03-07T19:45:43Z

The concern here is that batch ingestion tasks will get in the way of other ingestion tasks. But isn't that what the task priority system solves?

jihoonson · 2019-03-07T22:11:26Z

Would you please elaborate a bit more about your concern? Currently the task priority is actually just a lock priority. I think we should improve it to be able to run higher priority tasks faster.

glasser · 2019-03-07T22:18:20Z

Ah, my assumption about how priority works was that if there are more tasks scheduled for the cluster than available capacity, higher priority tasks would take precedence (and maybe even kill existing lower-prioritytasks?). Sounds like I'm wrong?

gianm · 2019-03-08T00:34:01Z

Right now, priority doesn't affect scheduling, only ability of tasks that are already-scheduled to acquire locks.

glasser · 2019-03-08T00:42:42Z

That's good to know, and this change makes lots of sense given that!

glasser · 2019-03-08T01:27:18Z

@jihoonson I just rebased #7048 on top of this and noticed that ITParallelIndexTest no longer actually runs in parallel. I assume that is not intended; I'll send off a PR.

jihoonson · 2019-03-08T01:29:19Z

@glasser OMG, it's my bad. That test should be run in parallel. Would you please fix it by adding maxNumSubTasks to the ingestion spec if possible?

glasser · 2019-03-08T01:31:13Z

Yep, #7211.

…7211) * integration-tests: make ITParallelIndexTest still work in parallel Follow-up to #7181, which made the default behavior for index_parallel tasks non-parallel. * Validate that parallel index subtasks were run

Reduce # of max subTasks to 2

9776b48

jihoonson added the Area - Batch Ingestion label Mar 2, 2019

jihoonson added this to the 0.14.0 milestone Mar 2, 2019

fjy reviewed Mar 2, 2019

View reviewed changes

jihoonson added 2 commits March 3, 2019 21:26

fix typo and add more doc

65258f8

add more doc and link

d7e4182

jihoonson changed the title ~~Reduce # of max subTasks to 2~~ Reduce default max # of subTasks to 2 for native parallel task Mar 4, 2019

jon-wei added the Release Notes label Mar 5, 2019

change default and add warning

409a9d1

clintropolis requested changes Mar 5, 2019

View reviewed changes

fix doc

b264a43

clintropolis approved these changes Mar 5, 2019

View reviewed changes

jihoonson changed the title ~~Reduce default max # of subTasks to 2 for native parallel task~~ Reduce default max # of subTasks to 1 for native parallel task Mar 5, 2019

add test

ae4c1d0

fix it test

a051f7e

fjy merged commit e48a9c1 into apache:master Mar 6, 2019

jon-wei mentioned this pull request Mar 6, 2019

[Backport] Reduce default max # of subTasks to 1 for native parallel task (#7181) #7200

Merged

glasser mentioned this pull request Mar 8, 2019

integration-tests: make ITParallelIndexTest still work in parallel #7211

Merged

Conversation

jihoonson commented Mar 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jihoonson Mar 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintropolis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jihoonson commented Mar 5, 2019

Uh oh!

jihoonson commented Mar 5, 2019

Uh oh!

glasser commented Mar 7, 2019

Uh oh!

jihoonson commented Mar 7, 2019

Uh oh!

glasser commented Mar 7, 2019

Uh oh!

gianm commented Mar 8, 2019

Uh oh!

glasser commented Mar 8, 2019

Uh oh!

glasser commented Mar 8, 2019

Uh oh!

jihoonson commented Mar 8, 2019

Uh oh!

glasser commented Mar 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jihoonson commented Mar 2, 2019 •

edited

Loading

jihoonson Mar 4, 2019 •

edited

Loading

clintropolis left a comment •

edited

Loading