More fine-grained DI for management node types. Don't allocate processing resources on Router by leventov · Pull Request #4429 · apache/druid

leventov · 2017-06-20T22:00:10Z

Remove DruidProcessingModule, QueryableModule and QueryRunnerFactoryModule from DI for coordinator, overlord, middle-manager.
Add RouterDruidProcessing not to allocate processing resources on Router.

The same goal as in #4410.

…odule from DI for coordinator, overlord, middle-manager. Add RouterDruidProcessing not to allocate processing resources on router

leventov · 2017-06-22T05:05:13Z

@gianm could you please review this pr?

gianm

General approach looks reasonable. Didn't review all the X-Pool refactors yet.

gianm · 2017-06-22T08:44:02Z

examples/conf-quickstart/druid/middleManager/runtime.properties


-# Processing threads and buffers
-druid.processing.buffer.sizeBytes=256000000
-druid.processing.numThreads=2


These are still useful since they're passed down to tasks. It would be better if the MM ignored them for its own processing pool (since it doesn't need one!) but still "remembered" them to pass down to each task.

Isn't it configured as druid.indexer.fork.property.druid.processing.numThreads=2

Currently both druid.indexer.fork.property.druid.processing.numThreads and druid.processing.numThreads would work (all druid.* properties are passed down automatically). A number of deployments depend on the latter form so it should keep working. fwiw, I also think it's prettier than the former, so I wouldn't want to deprecate it.

Reverted those example configs but with druid.indexer.fork.property. prefixes because IMO it's less confusing, and leaves less room for mistakes (e. g. when configs for different node types are shared, or copied when people base configs for one node type on config for another node type, etc.)

gianm · 2017-06-22T08:49:49Z

java-util/src/main/java/io/druid/java/util/common/concurrent/ExecutorServiceConfig.java

+    if (numThreadsConfigured != DEFAULT_NUM_THREADS) {
+      return numThreadsConfigured;
+    } else {
+      return Runtime.getRuntime().availableProcessors() - 1;


I suspect this code is unused (or maybe lightly used) or else someone would have complained by now that it doesn't work when you only have one processor. It should be max(1, Runtime.getRuntime().availableProcessors() - 1).

gianm · 2017-06-22T08:50:30Z

processing/src/main/java/io/druid/query/DruidProcessingConfig.java

+    } else {
+      // default to leaving one core for background tasks
+      final int processors = Runtime.getRuntime().availableProcessors();
+      return processors > 1 ? processors - 1 : processors;


This overrides getNumThreads from ExecutorServiceConfig, which maybe explains why nobody has complained about the bug there yet.

Removed this method, because it seems to be identical to the overridden method.

gianm · 2017-06-22T08:52:19Z

server/src/main/java/io/druid/guice/RouterProcessingModule.java

+  public BlockingPool<ByteBuffer> getMergeBufferPool(DruidProcessingConfig config)
+  {
+    if (config.getNumMergeBuffersConfigured() != DruidProcessingConfig.DEFAULT_NUM_MERGE_BUFFERS) {
+      log.error(


warn is probably more appropriate as nothing will really break here -- it's just a misguided configuration.

gianm · 2017-06-22T08:58:04Z

server/src/main/java/io/druid/guice/RouterProcessingModule.java

+  public ExecutorService getProcessingExecutorService(DruidProcessingConfig config)
+  {
+    if (config.getNumThreadsConfigured() != ExecutorServiceConfig.DEFAULT_NUM_THREADS) {
+      log.error("numThreads[%d] configured, that is ingnored on Router", config.getNumThreadsConfigured());


"ignored" (spelling). Also, as below, warn is probably more appropriate.

Fixed spelling.

Made it error deliberately, because in the future DruidProcessingConfig shouldn't be injected on Router.

Ok, error sounds good then.

gianm · 2017-06-22T08:59:06Z

services/src/main/java/io/druid/cli/DumpSegment.java

  protected List<? extends Module> getModules()
  {
    return ImmutableList.of(
+        new DruidProcessingModule(),


DumpSegment needs this stuff?

Yes, DumpSegment makes a segmentMetadata query!

Ah, cool :)

gianm · 2017-06-22T08:59:13Z

services/src/main/java/io/druid/cli/InsertSegment.java

  protected List<? extends Module> getModules()
  {
    return ImmutableList.<Module>of(
+        new DruidProcessingModule(),


InsertSegment needs this stuff?

I don't know, but since CreateTables and DumpSegment weirdly depend on this stuff, and we don't test InsertSegment at all (neither from unit tests nor integration tests), I decided to include everything, not to break it occasionally. The same for ResetCluster, ValidateSegments and DruidJsonValidator.

Sounds good. Perhaps include a comment that we're not sure if these are actually needed or not.

Added comments

gianm · 2017-06-22T08:59:20Z

services/src/main/java/io/druid/cli/ResetCluster.java

  protected List<? extends Module> getModules()
  {
    return ImmutableList.<Module>of(
+        new DruidProcessingModule(),


ResetCluster needs this stuff?

gianm · 2017-06-22T08:59:26Z

services/src/main/java/io/druid/cli/ValidateSegments.java

  protected List<? extends Module> getModules()
  {
    return ImmutableList.of(
+        new DruidProcessingModule(),


ValidateSegments needs this stuff?

gianm · 2017-06-22T08:59:35Z

services/src/main/java/io/druid/cli/validate/DruidJsonValidator.java

  protected List<? extends com.google.inject.Module> getModules()
  {
    return ImmutableList.<com.google.inject.Module>of(
+        new DruidProcessingModule(),


DruidJsonValidator needs this stuff?

…odule

dgolitsyn · 2017-06-23T17:03:05Z

processing/src/main/java/io/druid/segment/CompressedPools.java


  public static final int BUFFER_SIZE = 0x10000;
-  private static final StupidPool<BufferRecycler> bufferRecyclerPool = new StupidPool<BufferRecycler>(
+  private static final NonBlockingPool<com.ning.compress.BufferRecycler> bufferRecyclerPool = new StupidPool<BufferRecycler>(


could you please remove com.ning.compress

dgolitsyn · 2017-06-23T17:13:57Z

common/src/main/java/io/druid/collections/NonBlockingPool.java

+
+public interface NonBlockingPool<T>
+{
+  ResourceHolder<T> take();


Should NonBlockingPool and BlockingPool extend some common interface like ResourcePool?

I don't see the point. BlockingPool could extend NonBlockingPool but it would look surprising. Also it may create problems with DI

drcrallen

Looks cool! Changing around the module injection order can change the lifecycle ordering through. As such this probably needs good testing internally to make sure the different instance types can all run with this change.

gianm · 2017-06-26T21:17:14Z

Looks cool! Changing around the module injection order can change the lifecycle ordering through. As such this probably needs good testing internally to make sure the different instance types can all run with this change.

The integration tests, at least, check it for the major types.

gianm · 2017-06-27T17:32:05Z

Moved into 0.10.1 as discussed in the dev sync

gianm

LGTM

…sing resources on Router (apache#4429) * Remove DruidProcessingModule, QueryableModule and QueryRunnerFactoryModule from DI for coordinator, overlord, middle-manager. Add RouterDruidProcessing not to allocate processing resources on router * Fix examples * Fixes * Revert Peon configs and add comments * Remove qualifier

…sing resources on Router (#4429) (#4478) * Remove DruidProcessingModule, QueryableModule and QueryRunnerFactoryModule from DI for coordinator, overlord, middle-manager. Add RouterDruidProcessing not to allocate processing resources on router * Fix examples * Fixes * Revert Peon configs and add comments * Remove qualifier

Remove DruidProcessingModule, QueryableModule and QueryRunnerFactoryM…

3322943

…odule from DI for coordinator, overlord, middle-manager. Add RouterDruidProcessing not to allocate processing resources on router

leventov added the Improvement label Jun 20, 2017

leventov requested a review from gianm June 20, 2017 22:00

Fix examples

83f78b9

gianm reviewed Jun 22, 2017

View reviewed changes

leventov added 3 commits June 22, 2017 12:35

Merge remote-tracking branch 'upstream/master' into RouterProcessingM…

6f698d4

…odule

Fixes

8ffc521

Revert Peon configs and add comments

11dcf18

dgolitsyn reviewed Jun 23, 2017

View reviewed changes

Remove qualifier

2085a54

drcrallen reviewed Jun 23, 2017

View reviewed changes

leventov mentioned this pull request Jun 23, 2017

Lifecycle ordering depends on DI Module injection ordering #4462

Closed

jihoonson mentioned this pull request Jun 27, 2017

Transient failures of integration tests #4359

Closed

gianm added this to the 0.10.1 milestone Jun 27, 2017

gianm approved these changes Jun 28, 2017

View reviewed changes

gianm merged commit 2fa4b10 into apache:master Jun 28, 2017

gianm mentioned this pull request Jun 28, 2017

[Backport] More fine-grained DI for management node types. Don't allocate processing resources on Router #4478

Merged

leventov deleted the RouterProcessingModule branch June 28, 2017 21:02

leventov mentioned this pull request Sep 26, 2017

Error in coordinator/overlord when loading scan-query #4835

Closed

Comments

Conversation

leventov commented Jun 20, 2017

Uh oh!

leventov commented Jun 22, 2017

Uh oh!

gianm left a comment

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!

leventov Jun 23, 2017 • 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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drcrallen left a comment

Choose a reason for hiding this comment

Uh oh!

gianm commented Jun 26, 2017

Uh oh!

gianm commented Jun 27, 2017

Uh oh!

gianm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

leventov Jun 23, 2017 •

edited

Loading