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

Make `indices.memory.index_buffer_size` dynamically updatable #7550

Closed
wants to merge 7 commits into from

Conversation

Projects
None yet
5 participants
@mikemccand
Copy link
Contributor

commented Sep 3, 2014

This is PR for #7045

It also makes related settings (min/max_index_buffer_size and min/max_shard_index_buffer_size dynamically updatable.

But I have some concerns I wasn't sure about. First, whether the ShardsIndicesStatusChecker.run (the methods that "bubbles down" the allowed RAM to all active shards) is thread-safe or not since it can now be called from two threads at once. Probably I will just put a lock around it to be safe ...

Second, I just added these to ClusterDynamicSettingsModule, yet I use NodeSettingsService.addListener to subscribe to changes; is this right? Also, in the test, I'm using "persistent" cluster settings, but I'm not familiar with this (where do these changes persist?) and I couldn't figure out how to do a full restart and confirm the dynamic changes did in fact persist somewhere ... so I could use some pointers here :)

// LUCENE MONITOR: Based on this thread, currently (based on Mike), having a large buffer does not make a lot of sense: https://issues.apache.org/jira/browse/LUCENE-2324?focusedCommentId=13005155&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13005155
this.maxShardIndexBufferSize = componentSettings.getAsBytesSize("max_shard_index_buffer_size", new ByteSizeValue(512, ByteSizeUnit.MB));
this.minShardIndexBufferSize = componentSettings.getAsBytesSize(MIN_SHARD_INDEX_BUFFER_SIZE_KEY, DEFAULT_MIN_SHARD_INDEX_BUFFER_SIZE);
this.maxShardIndexBufferSize = componentSettings.getAsBytesSize(MAX_SHARD_INDEX_BUFFER_SIZE_KEY, DEFAULT_MAX_SHARD_INDEX_BUFFER_SIZE);

This comment has been minimized.

Copy link
@kimchy

kimchy Sep 3, 2014

Member

instead of using componentSettings, we can just use settings, and then we don't need the constants without the indices.memory prefix.

}

/** If forced is true, which happens when dynamic memory settings are updating, we recalculate even if active/inactive shards didn't change. */
public void run(boolean forced) {

This comment has been minimized.

Copy link
@kimchy

kimchy Sep 3, 2014

Member

should this be sync'ed, since it can run from 2 different threads now? I think it should to be safe.

This comment has been minimized.

Copy link
@kimchy

kimchy Sep 3, 2014

Member

just saw your non commit, I think its safer to just sync it

}

// #7045
public void testDynamicSettings() throws Exception {

This comment has been minimized.

Copy link
@kimchy

kimchy Sep 3, 2014

Member

for the test, can't we set the settings, and then assertBusy on the stats API to see that the updated index writer buffers have been applies? This will also test the actual logic to set it, which is a nice benefit, and then won't need to custom appender and depend on logging?

This comment has been minimized.

Copy link
@mikemccand

mikemccand Sep 3, 2014

Author Contributor

This will also test the actual logic to set it, which is a nice benefit, and then won't need to custom appender and depend on logging?

Hmm, I actually wanted to also assert that the change is in fact logged, because I don't want that to break in the future (e.g. this is useful info on autopsy). Though I agree it's not great scraping for the actual strings used in the messages ...

Note that the test (in the end) also pulls node stats and verifies the index_buffer_size change "took".

Hmm, assertBusy looks interesting, but ... since I do a .get() on each request ... doesn't that mean the change is done by the time .get() returns? Ie, they are not async requests... so I'm not sure why I need assertBusy.

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 3, 2014

Contributor

does this test to be so complicated? I mean we could just do a very simple SingleNodeTest like:

public class IndexingMemoryControllerTest extends ElasticsearchSingleNodeTest {
      public void testDynamicSettings() {
          IndexingMemoryController controller = getInstanceFromNode(IndexingMemoryController.class);
          ImmutableSettings settings = ImmutableSettings.builder()
                  .put(IndexingMemoryController.MIN_SHARD_INDEX_BUFFER_SIZE, "1mb")
                  .build();
          client().admin().cluster().prepareUpdateSettings().setPersistentSettings(settings).get();
          assertThat(controller.getMinShardIndexBufferSize(), is("1mb"));
    }
}

This comment has been minimized.

Copy link
@mikemccand

mikemccand Sep 3, 2014

Author Contributor

Ooh, that is much simpler.

OK I'll switch to that, and skip testing that logging is working when these settings are changed ...

@@ -95,6 +95,7 @@ public RecoverySettings(Settings settings, NodeSettingsService nodeSettingsServi
}

public void close() {
// TODO: shouldn't we nodeSettingsService.removeListener here?

This comment has been minimized.

Copy link
@kimchy

kimchy Sep 3, 2014

Member

yea, nice spotting it!. Its not the end of the world, since the node is closing anyhow, but its cleaner.

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 5, 2014

Contributor

mike wanna add that though?

mikemccand added some commits Sep 3, 2014

@mikemccand mikemccand added the review label Sep 3, 2014

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2014

I like the patch but the test is too complicated.... I left an example

@mikemccand

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2014

OK I simplified the test, added docs, added test cases to catch a bug in my last iteration that would lose previous settings (revert them to defaults) if you changed only one setting.

But the tests faile during cleanup because ElasticsearchSingleNodeTest is upset that I left persistent cluster settings behind ... how can I fix that? (Is it bad to use persistent settings?) I noticed ElasticsearchIntegrationTest has a ClusterScope annotation that lets you turn off this checking, but ESSingleNodeTest doesn't...

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2014

@mikemccand argh that is a node setting - I need to fix the single node test to allow that... stay tuned

mikemccand added some commits Sep 5, 2014

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2014

left a small comment - other that that LGTM

@mikemccand

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2014

OK I folded in all feedback, but... the new test cases intermittently fail, because the settings have not actually changed after the request returns.

I suspect the change is actually asynchronous? Like I must somehow wait for the cluster state to be updated / onRefreshSettings invoked, not just for the request to return (.get())? Maybe this is what @kimchy was getting at with assertBusy... but then I tried using assertBusy as well, which tries for up to 10 seconds to get assertions passing, but it also fails ... now I'm confused :) I'll keep digging...

@clintongormley clintongormley changed the title make indices.memory.index_buffer_size dynamically updatable Indexing: make indices.memory.index_buffer_size dynamically updatable Sep 8, 2014

ByteSizeValue minIndexingBuffer = componentSettings.getAsBytesSize("min_index_buffer_size", new ByteSizeValue(48, ByteSizeUnit.MB));
ByteSizeValue maxIndexingBuffer = componentSettings.getAsBytesSize("max_index_buffer_size", null);
// TODO: should we validate min/max here? somewhat dangerous because on upgrade this means existing configs may fail, since we are
// now lenient... but this would be a "favor" to such users since they don't realize their config is messed up now:

This comment has been minimized.

Copy link
@nik9000

nik9000 Sep 10, 2014

Contributor

It'd mean that Elasticsearch fails to start up with a message in the log files (I think). That'd be OK with me.

@s1monw s1monw removed the v1.5.0 label Mar 17, 2015

@clintongormley clintongormley removed the v1.6.0 label May 29, 2015

@clintongormley clintongormley changed the title Indexing: make indices.memory.index_buffer_size dynamically updatable Make `indices.memory.index_buffer_size` dynamically updatable Jun 7, 2015

@mikemccand

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2015

I could never get to the bottom of why the test would fail, but now I don't think we should make this setting dynamically updatable: this isn't a setting you should need to dynamically update, and I think it's risky to make it possible...

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.