Skip to content

Use daemon thread pool for AsyncHttpClient in emitters#5057

Merged
gianm merged 5 commits into
apache:masterfrom
metamx:daemon-threaded-http-client
Nov 10, 2017
Merged

Use daemon thread pool for AsyncHttpClient in emitters#5057
gianm merged 5 commits into
apache:masterfrom
metamx:daemon-threaded-http-client

Conversation

@xanec

@xanec xanec commented Nov 8, 2017

Copy link
Copy Markdown
Contributor

This PR is to configure AsyncHttpClients to use daemon thread pool: #4973 (comment)

leventov
leventov reviewed Nov 8, 2017
View reviewed changes
Comment thread server/src/main/java/io/druid/server/emitter/ParametrizedUriEmitterModule.java Outdated
@@ -63,6 +64,7 @@ public Emitter getEmitter(
if (sslContext != null) {

@leventov leventov Nov 8, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this common logic be shared between in HttpEmitterModule and ParametrizedUriEmitterModule?

@xanec xanec Nov 8, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think so. But I am having a hard time to come up with a proper place to put the common code. Previously, part of the logic was in java-util's HttpClientInit.createClient; maybe we should have a similar factory method for AsyncHttpClient in java-util?

@leventov leventov Nov 8, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it may be a static method in HttpEmitterModule, and ParametrizedUriEmitterModule uses it

@xanec xanec Nov 8, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I missed the other static method in HttpEmitterModule. Thanks 👍

@himanshug

himanshug commented Nov 8, 2017

Copy link
Copy Markdown
Contributor

👍 @xanec were you able to reproduce the issue and verified this fix ?

@himanshug

himanshug commented Nov 8, 2017

Copy link
Copy Markdown
Contributor

also, does this need to be backported to 0.11.0 ?

drcrallen
drcrallen requested changes Nov 8, 2017
View reviewed changes
Comment thread server/src/main/java/io/druid/server/emitter/HttpEmitterModule.java Outdated
if (sslContext != null) {
builder.setSslContext(new JdkSslContext(sslContext, true, ClientAuth.NONE));
}
builder.setThreadFactory(new DefaultThreadFactory("emitter-http-client", true));

@drcrallen drcrallen Nov 8, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

io.druid.java.util.common.concurrent.Execs#makeThreadFactory(java.lang.String)

Comment thread server/src/main/java/io/druid/server/emitter/ParametrizedUriEmitterModule.java Outdated
if (sslContext != null) {
builder.setSslContext(new JdkSslContext(sslContext, true, ClientAuth.NONE));
}
builder.setThreadFactory(new DefaultThreadFactory("parametrized-emitter-http-client", true));

@drcrallen drcrallen Nov 8, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

io.druid.java.util.common.concurrent.Execs#makeThreadFactory(java.lang.String) should be used

@xanec

xanec commented Nov 8, 2017

Copy link
Copy Markdown
Contributor Author

@himanshug yeah, I believe we are facing similar issue on our side. I'll need to get back to you on the verification.

@leventov leventov added this to the 0.11.0 milestone Nov 8, 2017
@himanshug

himanshug commented Nov 8, 2017

Copy link
Copy Markdown
Contributor

ok, i think we can merge it once the build passes as we definitely want that pool to be non-daemon in any case.

@leventov

leventov commented Nov 9, 2017

Copy link
Copy Markdown
Member

This build fails with some strange error:

[ERROR] COMPILATION ERROR : 
[ERROR] error reading /home/travis/.m2/repository/org/apache/hadoop/hadoop-hdfs/2.7.3/hadoop-hdfs-2.7.3-tests.jar; error in opening zip file
[ERROR] error reading /home/travis/.m2/repository/org/apache/hadoop/hadoop-hdfs/2.7.3/hadoop-hdfs-2.7.3-tests.jar; cannot read zip file
[ERROR] /home/travis/build/druid-io/druid/indexing-hadoop/src/test/java/io/druid/indexer/HdfsClasspathSetupTest.java:[35,30] cannot find symbol
  symbol:   class MiniDFSCluster
  location: package org.apache.hadoop.hdfs
[ERROR] /home/travis/build/druid-io/druid/indexing-hadoop/src/test/java/io/druid/indexer/HdfsClasspathSetupTest.java:[62,18] cannot find symbol
  symbol:   class MiniDFSCluster
  location: class io.druid.indexer.HdfsClasspathSetupTest
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.2:testCompile (default-testCompile) on project druid-indexing-hadoop: Compilation failure: Compilation failure: 
[ERROR] error reading /home/travis/.m2/repository/org/apache/hadoop/hadoop-hdfs/2.7.3/hadoop-hdfs-2.7.3-tests.jar; error in opening zip file
[ERROR] error reading /home/travis/.m2/repository/org/apache/hadoop/hadoop-hdfs/2.7.3/hadoop-hdfs-2.7.3-tests.jar; cannot read zip file
[ERROR] /home/travis/build/druid-io/druid/indexing-hadoop/src/test/java/io/druid/indexer/HdfsClasspathSetupTest.java:[35,30] cannot find symbol
[ERROR]   symbol:   class MiniDFSCluster
[ERROR]   location: package org.apache.hadoop.hdfs
[ERROR] /home/travis/build/druid-io/druid/indexing-hadoop/src/test/java/io/druid/indexer/HdfsClasspathSetupTest.java:[62,18] cannot find symbol
[ERROR]   symbol:   class MiniDFSCluster
[ERROR]   location: class io.druid.indexer.HdfsClasspathSetupTest
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
[ERROR] 
[ERROR] After correcting the problems, you can resume the build with the command
[ERROR]   mvn <goals> -rf :druid-indexing-hadoop

Making an experiment with updating Hadoop 2.7.3 -> 2.7.4.

FYI @nishantmonu51 @b-slim

@gianm

gianm commented Nov 9, 2017

Copy link
Copy Markdown
Contributor

@leventov seems like the weird error is due to this part:

error reading /home/travis/.m2/repository/org/apache/hadoop/hadoop-hdfs/2.7.3/hadoop-hdfs-2.7.3-tests.jar; error in opening zip file

The hadoop-hdfs-2.7.3-tests.jar could be corrupt. The corruption could persist across builds due to the cache, enabled in our .travis.yml here:

cache:
  directories:
    - $HOME/.m2

Clearing the cache could help (https://docs.travis-ci.com/user/caching/).

@leventov leventov force-pushed the daemon-threaded-http-client branch from 0d03c23 to 848663a Compare November 9, 2017 21:41
@gianm gianm merged commit 81f2498 into apache:master Nov 10, 2017
gianm pushed a commit to gianm/druid that referenced this pull request Nov 10, 2017
@xanec @gianm
Use daemon thread pool for AsyncHttpClient in emitters (apache#5057)
667e752 
* use daemon thread pool for AsyncHttpClient in emitters

* changed to use existing helper methods

* refactored creation of AsyncHttpClient
@gianm gianm mentioned this pull request Nov 10, 2017
Merged
@leventov leventov deleted the daemon-threaded-http-client branch November 10, 2017 03:53
leventov pushed a commit that referenced this pull request Nov 10, 2017
@gianm @leventov
[Backport] Use daemon thread pool for AsyncHttpClient in emitters (#5069
4b31d10 
)

* Use daemon thread pool for AsyncHttpClient in emitters (#5057)

* use daemon thread pool for AsyncHttpClient in emitters

* changed to use existing helper methods

* refactored creation of AsyncHttpClient

* Avoid Execs, it's not available in this branch.
@himanshug himanshug mentioned this pull request Nov 10, 2017
Merged
leventov pushed a commit to metamx/druid that referenced this pull request Nov 13, 2017
@gianm @leventov
[Backport] Use daemon thread pool for AsyncHttpClient in emitters (ap…
baa9d44 
…ache#5069)

* Use daemon thread pool for AsyncHttpClient in emitters (apache#5057)

* use daemon thread pool for AsyncHttpClient in emitters

* changed to use existing helper methods

* refactored creation of AsyncHttpClient

* Avoid Execs, it's not available in this branch.
riovic918data pushed a commit to riovic918data/druid that referenced this pull request Jun 12, 2026
@xanec @gianm
Use daemon thread pool for AsyncHttpClient in emitters (apache#5057)
5c08145 
* use daemon thread pool for AsyncHttpClient in emitters

* changed to use existing helper methods

* refactored creation of AsyncHttpClient
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leventov leventov leventov approved these changes
@drcrallen drcrallen drcrallen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.11.0

Development

Successfully merging this pull request may close these issues.

5 participants

@xanec @himanshug @leventov @gianm @drcrallen