Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add `ignore_idle_threads` (default: true) to hot threads #8985

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
5 participants
@mikemccand
Copy link
Contributor

mikemccand commented Dec 17, 2014

Adds ignore_idle_threads to hot threads request, defaulting to true.

It's somewhat heuristicky ... but seems to work.

Closes #8908

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Dec 17, 2014

LGTM

Can you add this new parameter to rest-api-spec/api/nodes.hot_threads.json as well when pushing?

@@ -84,6 +94,7 @@ public NodesHotThreadsRequest snapshots(int snapshots) {
public void readFrom(StreamInput in) throws IOException {
super.readFrom(in);
threads = in.readInt();
ignoreIdleThreads = in.readBoolean();

This comment has been minimized.

Copy link
@javanna

javanna Dec 17, 2014

Member

we need a version check here if this goes in 1.x

@@ -93,6 +104,7 @@ public void readFrom(StreamInput in) throws IOException {
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
out.writeInt(threads);
out.writeBoolean(ignoreIdleThreads);

This comment has been minimized.

Copy link
@javanna

javanna Dec 17, 2014

Member

we need a version check here if this goes in 1.x

@javanna

This comment has been minimized.

Copy link
Member

javanna commented Dec 17, 2014

Left a comment on backwards compatibility, I see the PR is labelled 1.4.3, do we see this as a bugfix then?

@mikemccand

This comment has been minimized.

Copy link
Contributor Author

mikemccand commented Dec 17, 2014

Can you add this new parameter to rest-api-spec/api/nodes.hot_threads.json as well when pushing?

Woops, will do.

@mikemccand

This comment has been minimized.

Copy link
Contributor Author

mikemccand commented Dec 17, 2014

Left a comment on backwards compatibility, I see the PR is labelled 1.4.3, do we see this as a bugfix then?

Thanks, I'll add the version check.

I added 1.4.3 here because it seems important (we all could save a lot of time not having to look at so much noise when we stare at hot threads, which we do quite frequently), and it seemed like a low risk improvement. But it really isn't a bug, so I'll remove 1.4.3 and just do 1.5 (with a back-compat check in readFrom/writeTo).

@mikemccand mikemccand removed the v1.4.3 label Dec 17, 2014

@nik9000

This comment has been minimized.

Copy link
Contributor

nik9000 commented Dec 17, 2014

It's somewhat heuristicky ... but seems to work.

I imagine its fine as is but you could support a bit more on the fly hackery by allowing the user to specify a script on for ignore_idle_threads. Or a script (or patterns) in the config. I believe what you have covers most of the cases I've seen though.

@mikemccand

This comment has been minimized.

Copy link
Contributor Author

mikemccand commented Dec 17, 2014

by allowing the user to specify a script on for ignore_idle_threads

Hmm this seems a little dangerous ... I think at this low level we need to keep it as simple as possible.

@mikemccand

This comment has been minimized.

Copy link
Contributor Author

mikemccand commented Dec 17, 2014

@javanna I added back-compat checks for the new ignore_idle_threads in readFrom/writeTo ... does that look OK?

@@ -84,6 +95,11 @@ public NodesHotThreadsRequest snapshots(int snapshots) {
public void readFrom(StreamInput in) throws IOException {
super.readFrom(in);
threads = in.readInt();
if (in.getVersion().before(Version.V_1_5_0)) {
ignoreIdleThreads = true;

This comment has been minimized.

Copy link
@javanna

javanna Dec 17, 2014

Member

I wonder if the default value when the request comes from an older node should be false instead. I know that the default for the new flag is true, but given that the previous behaviour was false, when the request comes from an older node it should still be false, otherwise we'd get mixed results overall, some (from older nodes) that don't ignore idle threads, and some that do?

This comment has been minimized.

Copy link
@mikemccand

mikemccand Dec 17, 2014

Author Contributor

OK, good point, I'll change to false.

mikemccand added a commit that referenced this pull request Dec 17, 2014

Core: ignore known idle threads by default in /_nodes/hot_threads
Add a new ignore_idle_threads boolean option (default true) to
/_nodes/hot_threads, to filter out threads in known idle places like
waiting on a socket select or on pulling the next task from an empty
queue.

Closes #8985

Closes #8908

@clintongormley clintongormley changed the title Core: add ignore_idle_threads (default: true) to hot threads Add `ignore_idle_threads` (default: true) to hot threads Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.