Use separate searchers for "search visibility" vs "move indexing buffer to disk"

[mikemccand]

Spinoff from #14121...

Today, when ES detects it's using too much heap vs the configured indexing buffer (default 10% of JVM heap) it opens a new searcher to force Lucene to move the bytes to disk, clear version map, etc.

But this has the unexpected side effect of making newly indexed/deleted documents visible to future searches, which is not nice for users who are trying to prevent that, e.g. #3593.

As @uschindler suggested in that issue, I think ES should have two separate searchers from the engine: one for search visibility, only ever refreshed according to the user's wishes, and another, used internally for freeing up heap, version map lookups, etc. Lucene will be efficient about this, sharing segment readers across those two searchers.

I haven't started on this (need to finish #14121 first!) so if someone wants to take it, please feel free!

[mikemccand]

I'll try to tackle this ... it doesn't look too hard, given the changes in #14121 which already begins separate "write indexing buffer to disk" from "refresh".

[mikemccand]

Note that with this change, a refresh only happens when the user expects it to: on the periodic (default: every 1 second) interval, or when refresh API is explicitly invoked.

But this is a biggish change to ES's behavior vs today, e.g. flush, forceMerge, moving indexing buffers to disk because they are too big, etc. does NOT refresh, and a good number of tests are angry because of this ... so I'm slowly inserting refresh() for such tests.

It also has implications for transient disk usage, since ES will "secretly" refresh less often, meaning we hold segments, which may now be merged or deleted, open for longer. Users who disable refresh_interval (set to -1) need to be careful to invoke refresh API at important times (after flush or forceMerge).

Still I think it is important we make ES's semantics/behavior crisp and well defined: refresh, and refresh alone, makes recent index changes visible to searches. No other operation should do this as an "accidental" side effect.

[bleskes]

Just a note - since ES moves shard around on it’s own will, if we want to support this, we’ll have to make sure shard relocation (i.e., copy all the files) maintains this semantics. This will be tricky for many reasons - for example, the user may issue a refresh command when the target shard is not yet ready to receive it (engine closed). Today we refresh the target at the end of every recovery for this reason. Have a “refresh when I say and only when I say” is much more complicated then the current “refresh at least when I say” (but whenever you want as well) semantics. I’m not sure it’s worth the complexity imho.

On 06 Jan 2016, at 19:56, Michael McCandless notifications@github.com wrote:

Note that with this change, a refresh only happens when the user expects it to: on the periodic (default: every 1 second) interval, or when refresh API is explicitly invoked.

But this is a biggish change to ES's behavior vs today, e.g. flush, forceMerge, moving indexing buffers to disk because they are too big, etc. does NOT refresh, and a good number of tests are angry because of this ... so I'm slowly inserting refresh() for such tests.

It also has implications for transient disk usage, since ES will "secretly" refresh less often, meaning we hold segments, which may now be merged or deleted, open for longer. Users who disable refresh_interval (set to -1) need to be careful to invoke refresh API at important times (after flush or forceMerge).

Still I think it is important we make ES's semantics/behavior crisp and well defined: refresh, and refresh alone, makes recent index changes visible to searches. No other operation should do this as an "accidental" side effect.

—
Reply to this email directly or view it on GitHub.

[mikemccand]

Thanks @bleskes, I agree this is too difficult to achieve "perfectly", and I think recovery should be unchanged here (refresh when the shard is done recovering).

Similarly, primary and each replica are in general searching slightly different of a shard today, i.e. when each refreshes every 1s by default, it's a different set of indexed docs that become visible, in general.

File-based replication would make this easier ;)

So I think those should remain out of scope, here, and we should still state that ES is a "refresh at least when I say", but with this issue "less often when I don't say" than today.

Or are you saying we shouldn't even try to make any change here, i.e. leave the engine doing a normal search-visible refresh when e.g. it wants to free up heap used by version map?

[s1monw]

So I think those should remain out of scope, here, and we should still state that ES is a "refresh at least when I say", but with this issue "less often when I don't say" than today.

+1 to this - I think for the replication case we should refresh since we have to but for stuff like clearing version maps etc. we can improve the situation.