rest-client-sniffer: configurable threadfactory #26897

retoo · 2017-10-05T17:02:51Z

Make the sniffers threadfactory configurable.

I.e. useful if you want to make the sniffer's thread daemonized.

elasticmachine · 2017-10-05T17:02:53Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

javanna · 2017-10-05T18:23:44Z

hi @retoo thanks for your PR. Is the only usecase for this making the sniffer thread daemonized. If so maybe we should do it by default instead rather than making the thread factory configurable. What do you think?

s1monw · 2017-10-05T19:09:07Z

I think we should not make it pluggable and all thread should be daemonized. It should in-fact do the same thing we do in core here

retoo · 2017-10-05T19:11:20Z

I also give the thing a reasonable threadname :) .

I was able to configure the threadfactory for the regular http client (using setHttpClientConfigCallback()), but not for the sniffer, so I thought you wanted to have it configured simillary. See the following snippet:

	return RestClient.builder(host)
				.setHttpClientConfigCallback(c -> c.setThreadFactory(threadFactorWithPrefix("es-rest-client")))
				.build();

btw. I've completed the CLA in July 2016, I don't know why your check says otherwise, I contacted @karmi by mail regarding that topic.

@s1monw I'm not sure, i think it's good style to give some control about the threads when spinning up new thread executors. As said above, thats what the apache http client is doing.

s1monw · 2017-10-05T19:33:01Z

@retoo would you mind changeing the PR to add the a dedicated thread factory like this feel free to copy the code. The thread-name should also be fixed IMO.

karmi · 2017-10-07T09:35:25Z

(..) I contacted @karmi by mail regarding that topic

Correct!, and when looking into the issue, @retoo only signed the "A company wanting its employees to contribute" agreement, but every individual contributor under that agreement also needs to sign the "An individual contributing under an existing company contributor agreement" agreement. I've asked @retoo to do that and comment here again, that should trigger the check.

s1monw

Left one comment LGTM otherwise

s1monw · 2017-10-07T12:08:46Z

client/sniffer/src/main/java/org/elasticsearch/client/sniff/Sniffer.java

+        }
+
+        @Override
+        public Thread newThread(Runnable r) {


I think we should do this in a AccessController.doPrivileged block since we need a special permission to modify threadgroup. It's good to respect that since we use this in reindex etc. as well

I got rid of this whole topic by just calling Executors.defaultFactory().newThread(Runnable). Somebody copied the code from java.util.concurrent.Executors.DefaultThreadFactory#DefaultThreadFactory to EsExecutors.

retoo · 2017-10-09T13:16:03Z

@karmi thanks for the explanation, now it seems tow work.

s1monw · 2017-10-11T06:00:33Z

@elasticmachine ok to test

s1monw · 2017-10-11T06:05:58Z

client/sniffer/src/main/java/org/elasticsearch/client/sniff/Sniffer.java

+
+        private SnifferThreadFactory(String namePrefix) {
+            this.namePrefix = namePrefix;
+            this.originalThreadFactory = Executors.defaultThreadFactory();


I think we should still wrap this in a doPriviledged block same asn the entire thread creation and set* below. It will allow us and other users to give fine granular security permissions

retoo · 2017-10-11T09:03:51Z

@s1monw I've wrapped the build() of the sniffer in a doPrivileged, this mirros the creation of the http client in (org.elasticsearch.client.RestClientBuilder#createHttpClient).

Is this okay for you?

s1monw · 2017-10-12T07:31:20Z

Is this okay for you?

to be honest I think we should wrap each individual part that needs such a block. doing it on a higher level is not necessarily helpful. Just wrap the content of SnifferThreadFactory #newThread?

s1monw · 2017-10-12T07:32:58Z

@retoo can you also sync up with master it seems like there are some bwc issues that have been fixe in master and 6.x but not in your branch so tests can pass?

retoo · 2017-10-12T08:20:35Z

@s1monw I rebased to latest master, and I wrapped the two places where privileged calls are being made:
a) Executors.defaultThreadFactory();
b) originalThreadFactory.newThread(r);

Or do you only want it around b)/newThread?

s1monw

LGTM

s1monw · 2017-10-12T13:21:44Z

thanks @retoo I think I want to treat the fact that we start non-daemon threads as a bug wo I wonder if we should backport to 5.6.x @javanna WDYT?

jasontedor · 2017-10-12T13:27:50Z

I agree this is a bug and I'm +1 to integrate this into 5.6 and 6.0.

This change adds a dedicated thread group, configures threads with a corresponding thread name and starts all threads as daemon threads.

s1monw · 2017-10-12T13:30:54Z

backported - thanks a lot @retoo

javanna · 2017-10-12T14:46:14Z

++ on my end too

* master: (35 commits) Create weights lazily in filter and filters aggregation (#26983) Use a dedicated ThreadGroup in rest sniffer (#26897) Fire global checkpoint sync under system context Update by Query is modified to accept short `script` parameter. (#26841) Cat shards bytes (#26952) Add support for parsing inline script (#23824) (#26846) Change default value to true for transpositions parameter of fuzzy query (#26901) Adding unreleased 5.6.4 version number to Version.java Rename TCPTransportTests to TcpTransportTests (#26954) Fix NPE for /_cat/indices when no primary shard (#26953) [DOCS] Fixed indentation of the definition list. Fix formatting in channel close test Check for closed connection while opening Clarify systemd overrides [DOCS] Plugin Installation for Windows (#21671) Painless: add tests for cached boxing (#24163) Don't detect source's XContentType in DocumentParser.parseDocument() (#26880) Fix handling of paths containing parentheses Allow only a fixed-size receive predictor (#26165) Add Homebrew instructions to getting started ...

* 6.x: (32 commits) Use a dedicated ThreadGroup in rest sniffer (#26897) Fire global checkpoint sync under system context Update by Query is modified to accept short `script` parameter. (#26841) Cat shards bytes (#26952) Adding unreleased 5.6.4 version number to Version.java Rename TCPTransportTests to TcpTransportTests (#26954) Fix NPE for /_cat/indices when no primary shard (#26953) [DOCS] Fixed indentation of the definition list. Check for closed connection while opening Clarify systemd overrides [DOCS] Plugin Installation for Windows (#21671) Painless: add tests for cached boxing (#24163) Don't detect source's XContentType in DocumentParser.parseDocument() (#26880) Return List instead of an array from settings (#26903) Emit deprecation warning for variable size predictor Fix handling of paths containing parentheses Deprecate variable-size receive predictor Add Homebrew instructions to getting started ingest: Fix bug that prevent date_index_name processor from accepting timestamps specified as a json number Scripting: Fix expressions to temporarily support filter scripts (#26824) ...

* master: (356 commits) Do not set SO_LINGER on server channels (elastic#26997) Fix inconsistencies in the rest api specs for *_script (elastic#26971) fix inconsistencies in the rest api specs for cat.snapshots (elastic#26996) Add docs on full_id parameter in cat nodes API [TEST] Add test that replicates versioned updates with random flushes Use internal searcher for all indexing related operations in the engine Reformat paragraph in template docs to 80 columns Clarify settings and template on create index Fix reference to TcpTransport in documentation Allow Uid#decodeId to decode from a byte array slice (elastic#26987) Fix a typo in the similarity docs (elastic#26970) Use separate searchers for "search visibility" vs "move indexing buffer to disk (elastic#26972) Create weights lazily in filter and filters aggregation (elastic#26983) Use a dedicated ThreadGroup in rest sniffer (elastic#26897) Fire global checkpoint sync under system context Update by Query is modified to accept short `script` parameter. (elastic#26841) Cat shards bytes (elastic#26952) Add support for parsing inline script (elastic#23824) (elastic#26846) Change default value to true for transpositions parameter of fuzzy query (elastic#26901) Adding unreleased 5.6.4 version number to Version.java ...

javanna added :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch >enhancement feedback_needed labels Oct 5, 2017

javanna self-assigned this Oct 5, 2017

retoo force-pushed the feature/make-threadfactory-configurable-for-sniffer branch 3 times, most recently from f75ce27 to 3c5f35e Compare October 6, 2017 09:37

s1monw reviewed Oct 7, 2017

View reviewed changes

retoo force-pushed the feature/make-threadfactory-configurable-for-sniffer branch 2 times, most recently from 14f0b59 to 093a5bf Compare October 9, 2017 13:13

s1monw requested changes Oct 11, 2017

View reviewed changes

retoo force-pushed the feature/make-threadfactory-configurable-for-sniffer branch 2 times, most recently from 0f9c2ac to 346088b Compare October 11, 2017 09:07

retoo added 2 commits October 12, 2017 10:07

Sniffer: use daemon threads and define a reasonable thread name

0c8d4ac

SnifferBuilder: do sniffer creation within doPrivileged

34549c6

retoo force-pushed the feature/make-threadfactory-configurable-for-sniffer branch from 346088b to 34549c6 Compare October 12, 2017 08:19

s1monw approved these changes Oct 12, 2017

View reviewed changes

s1monw merged commit ab94150 into elastic:master Oct 12, 2017

s1monw added >bug v6.1.0 v7.0.0 and removed >enhancement feedback_needed labels Oct 12, 2017

s1monw added v5.6.0 v6.0.0 v5.6.4 and removed v5.6.0 labels Oct 12, 2017

s1monw pushed a commit that referenced this pull request Oct 12, 2017

Use a dedicated ThreadGroup in rest sniffer (#26897)

ec6ef36

This change adds a dedicated thread group, configures threads with a corresponding thread name and starts all threads as daemon threads.

s1monw pushed a commit that referenced this pull request Oct 12, 2017

Use a dedicated ThreadGroup in rest sniffer (#26897)

bb92bb2

This change adds a dedicated thread group, configures threads with a corresponding thread name and starts all threads as daemon threads.

s1monw pushed a commit that referenced this pull request Oct 12, 2017

Use a dedicated ThreadGroup in rest sniffer (#26897)

22811b5

This change adds a dedicated thread group, configures threads with a corresponding thread name and starts all threads as daemon threads.

lcawl added v6.0.0-rc2 and removed v6.0.0 labels Oct 30, 2017

lcawl removed the v6.1.0 label Dec 12, 2017

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rest-client-sniffer: configurable threadfactory #26897

rest-client-sniffer: configurable threadfactory #26897

retoo commented Oct 5, 2017

elasticmachine commented Oct 5, 2017

javanna commented Oct 5, 2017

s1monw commented Oct 5, 2017

retoo commented Oct 5, 2017 •

edited

Loading

s1monw commented Oct 5, 2017

karmi commented Oct 7, 2017

s1monw left a comment

s1monw Oct 7, 2017

retoo Oct 9, 2017

retoo commented Oct 9, 2017

s1monw commented Oct 11, 2017

s1monw Oct 11, 2017

retoo commented Oct 11, 2017 •

edited

Loading

s1monw commented Oct 12, 2017

s1monw commented Oct 12, 2017

retoo commented Oct 12, 2017

s1monw left a comment

s1monw commented Oct 12, 2017

jasontedor commented Oct 12, 2017

s1monw commented Oct 12, 2017

javanna commented Oct 12, 2017

rest-client-sniffer: configurable threadfactory #26897

rest-client-sniffer: configurable threadfactory #26897

Conversation

retoo commented Oct 5, 2017

elasticmachine commented Oct 5, 2017

javanna commented Oct 5, 2017

s1monw commented Oct 5, 2017

retoo commented Oct 5, 2017 • edited Loading

s1monw commented Oct 5, 2017

karmi commented Oct 7, 2017

s1monw left a comment

Choose a reason for hiding this comment

s1monw Oct 7, 2017

Choose a reason for hiding this comment

retoo Oct 9, 2017

Choose a reason for hiding this comment

retoo commented Oct 9, 2017

s1monw commented Oct 11, 2017

s1monw Oct 11, 2017

Choose a reason for hiding this comment

retoo commented Oct 11, 2017 • edited Loading

s1monw commented Oct 12, 2017

s1monw commented Oct 12, 2017

retoo commented Oct 12, 2017

s1monw left a comment

Choose a reason for hiding this comment

s1monw commented Oct 12, 2017

jasontedor commented Oct 12, 2017

s1monw commented Oct 12, 2017

javanna commented Oct 12, 2017

retoo commented Oct 5, 2017 •

edited

Loading

retoo commented Oct 11, 2017 •

edited

Loading