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

Log when dynamically changing merge thread pool size #6882

Closed
nik9000 opened this Issue Jul 15, 2014 · 11 comments

Comments

Projects
None yet
4 participants
@nik9000
Copy link
Contributor

commented Jul 15, 2014

I tried to dynamically set index.merge.scheduler.max_thread_count and it looked as though nothing happened. No log or anything. It looks like that is supposed to be possible in ConcurrentMergeSchedulerProvider.java:161 but it wasn't working.

@mikemccand

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2014

Hmm are you sure it didn't "work" though? E.g. if you get stack traces for all threads do you see that they are still busy merging vs waiting for other merge threads to finish first?

@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2014

I had some long running merges on the box that were eating all the IO. I ended up restarting Elasticsearch on that machine after it didn't look like anything changed so I don't have good records. The stack traces I saw didn't look to have changed. I had imagined some of the merges would finish and new ones wouldn't start/block but without some log I wasn't going to wait another twenty minutes of brownout to be sure.

@clintongormley clintongormley changed the title Can't dynamically change merge thread pool size Log when dynamically changing merge thread pool size Jul 16, 2014

@clintongormley

This comment has been minimized.

Copy link
Member

commented Jul 16, 2014

We have a test that shows that this works (#6842), so the thing that is missing is a log message to inform the user that their change has been accepted.

@mikemccand

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2014

Actually, this issue (changing index.merge.scheduler.max_thread_count) is different from #6842 (changing index.store.throttle_type). We should first add a test here; the test could probably just peek at the lucene.iw TRACE logs to confirm merges are being paused.

@nik9000 the max_thread_count just controls how many merges are allowed to run at once, not how many merges are "pending". I don't think anything is logged at the default verbosity, so you wouldn't know things had actually changed. The simplest way to verify might be to take a full thread dump and then count how many merge threads (Lucene Merge Thread #XXX) are actually doing something vs. paused waiting "their turn" to run.

@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2014

@mikemccand yeah - when I did that I still got quite a few threads but I imagine that is because they were in flight when I changed the setting. I'm happy to call the issue settled. I was mostly panicking when I filed the bug because servers were falling over because they were merging when we were close to the edge.

In addition to the log, it'd be nice if the result of the curl command contained what was changed like it usually does. That parameter just didn't come back to me which is normally the signal I use to know that changing something dynamically works.

@mikemccand mikemccand self-assigned this Jul 18, 2014

@mikemccand

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2014

OK I dug into this, and when I made a test, it hit an exception claiming index.merge.scheduler.max_thread_count is not a dynamic setting. @nik9000 did you see any error when you tried?

Yet, in ConcurrentMergeSchedulerProvider, it's clearly prepared to update these settings, so I added them to IndexDynamicSettingsModule and the test passes. The test also verifies that an INFO message is logged with the change. I'll open a PR.

mikemccand added a commit to mikemccand/elasticsearch that referenced this issue Jul 18, 2014

Allow index.merge.scheduler.max_thread_count to be dynamically changed
Lucene allows the max_thread_count to be updated, but this wasn't
fully exposed in Elasticsearch.

Closes elastic#6882
@mikemccand

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2014

OK I opened #6925

@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2014

OK I dug into this, and when I made a test, it hit an exception claiming index.merge.scheduler.max_thread_count is not a dynamic setting. @nik9000 did you see any error when you tried?

No BUT I didn't look at all the nodes. I imagine it'd have been on the master?

@mikemccand

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2014

Hmm I'm honestly not sure how the error would have propagated back to how you requested; my test case just uses the Java client API (I think!) and somehow the error came back through there.

Maybe there's another issue, that the error fails to properly propagate back to the HTTP response to your curl request?

@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2014

That is certainly possible. I don't really use the Java api at all except
when I'm working on Elasticsearch itself.

On Fri, Jul 18, 2014 at 12:46 PM, Michael McCandless <
notifications@github.com> wrote:

Hmm I'm honestly not sure how the error would have propagated back to how
you requested; my test case just uses the Java client API (I think!) and
somehow the error came back through there.

Maybe there's another issue, that the error fails to properly propagate
back to the HTTP response to your curl request?


Reply to this email directly or view it on GitHub
#6882 (comment)
.

@jpountz jpountz removed the adoptme label Aug 1, 2014

@mikemccand

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2014

This was fixed with #6925

@mikemccand mikemccand closed this Aug 1, 2014

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.