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

Improve IndexingMemoryController a bit #13548

Merged
merged 6 commits into from Sep 15, 2015

Conversation

mikemccand
Copy link
Contributor

I started by trying to explain the test failure in #13487, but failed so far, but then in the process found some other things to fix:

  • The IndexingMemoryController sets indexing buffer sizes for each shard, but then doesn't always push those changes down to IndexWriter
  • Instead, we push settings changes down to IW only during flush and refresh, but I think this just hides bugs, and for some configurations could be quite a while ... we should push changes immediately whenever settings are updated
  • If you try to specify indices.memory.index_buffer_size in the node settings to a bytes (not %) value, you'll hit NPE on starting up the node
  • Removed some dead code, added some missing units (time -> timeMS) in variable names
  • We currently wait for all merges in a shard to finish before considering it inactive, but I think that's not necessary, i.e. we can safely drop the indexing buffer for an inactive shard while merges are still running. Other shards can use that heap.

I think we should separately fix these (this PR) ... but I'll keep trying to explain the original test failure.

I did add some more logger.debug so maybe next failure reveals something.

  - promptly push indexing buffer changes to IndexWriter, instead of waiting for next refresh/flush
  - don't wait for merges to finish before dropping a shards's indexing buffer to 512 KB
  - fix NPE if indices.memory.index_buffer_size is in node's settings with a bytes (not %) unit
  - add some more logger.debug
* This setting is <b>not</b> realtime updateable.
* Index setting to control the initial index buffer size. NOTE: this setting is somewhat
* useless, since IndexingMemoryController will take over quickly and partition the
* indices.memory.index_buffer_size for this node across all shards.
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to remove it for 2.0 then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we want to remove it for 2.0 then?

+1, but I'll open a separate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1, but I'll open a separate issue.

I opened #13563 for this.

@bleskes
Copy link
Contributor

bleskes commented Sep 14, 2015

Thx mike. Left comments/questions here and there.

@mikemccand
Copy link
Contributor Author

Thanks @bleskes, I folded in the feedback!

@mikemccand mikemccand added the :Core/Infra/Core Core issues without another label label Sep 15, 2015

this.interval = this.settings.getAsTime(SHARD_INACTIVE_INTERVAL_TIME_SETTING, TimeValue.timeValueSeconds(30));

logger.debug("using indexing buffer size [{}], with {} [{}], {} [{}], {} [{}], {} [{}]",
Copy link
Contributor

Choose a reason for hiding this comment

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

++

@bleskes
Copy link
Contributor

bleskes commented Sep 15, 2015

LGTM. Thx @mikemccand

mikemccand pushed a commit that referenced this pull request Sep 15, 2015
Improve IndexingMemoryController a bit:
      - promptly push indexing buffer changes to IndexWriter, instead of waiting for next refresh/flush
      - don't wait for merges to finish before dropping a shards's indexing buffer to 512 KB once it's inactive
      - fix NPE if indices.memory.index_buffer_size is in node's settings with a bytes (not %) unit
      - add some more logger.debug
@mikemccand mikemccand merged commit 028acdd into elastic:master Sep 15, 2015
@mikemccand mikemccand deleted the indexing_memory_controller branch September 15, 2015 14:39
mikemccand pushed a commit that referenced this pull request Sep 15, 2015
Improve IndexingMemoryController a bit:
      - promptly push indexing buffer changes to IndexWriter, instead of waiting for next refresh/flush
      - don't wait for merges to finish before dropping a shards's indexing buffer to 512 KB once it's inactive
      - fix NPE if indices.memory.index_buffer_size is in node's settings with a bytes (not %) unit
      - add some more logger.debug
mikemccand pushed a commit that referenced this pull request Sep 15, 2015
Improve IndexingMemoryController a bit:
      - promptly push indexing buffer changes to IndexWriter, instead of waiting for next refresh/flush
      - don't wait for merges to finish before dropping a shards's indexing buffer to 512 KB once it's inactive
      - fix NPE if indices.memory.index_buffer_size is in node's settings with a bytes (not %) unit
      - add some more logger.debug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Core Core issues without another label v2.0.0-rc1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants