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

Skip shard refreshes if shard is `search idle` #27500

Merged
merged 22 commits into from Nov 27, 2017

Conversation

Projects
None yet
6 participants
@s1monw
Copy link
Contributor

commented Nov 22, 2017

Today we refresh automatically in the backgroud by default very second.
This default behavior has a significant impact on indexing performance
if the refreshes are not needed.
This change introduces a notion of a shard being search idle which a
shard transitions to after (default) 30s without any access to an
external searcher. Once a shard is search idle all scheduled refreshes
will be skipped unless there are any refresh listeners registered.
If a search happens on a serach idle shard the search request park
on a refresh listener and will be executed once the next scheduled refresh
occurs. This will also turn the shard into the non-idle state immediately.

This behavior is only applied if there is no explicit refresh interval set.

Skip shard refreshes if shard is `search idle`
Today we refresh automatically in the backgroud by default very second.
This default behavior has a significant impact on indexing performance
if the refreshes are not needed.
This change introduces a notion of a shard being `search idle` which a
shard transitions to after (default) `30s` without any access to an
external searcher. Once a shard is search idle all scheduled refreshes
will be skipped unless there are any refresh listeners registered.
If a search happens on a `serach idle` shard the search request _park_
on a refresh listener and will be executed once the next scheduled refresh
occurs. This will also turn the shard into the `non-idle` state immediately.

This behavior is only applied if there is no explicit refresh interval set.
@jpountz
Copy link
Contributor

left a comment

Not very familiar with this part of the code base but what I understand makes sense to me. You will probably want to add docs as well.

} while (pendingRefreshLocation.compareAndSet(location, lastWriteLocation) == false);
}

public void awaitPendingRefresh(Consumer<Boolean> listener) {

This comment has been minimized.

Copy link
@jpountz

jpountz Nov 23, 2017

Contributor

Please add javadocs.

if (refreshListeners.refreshNeeded() == false // if we have a listener that is waiting for a refresh we need to force it
&& isSearchIdle() && indexSettings.isExplicitRefresh() == false) {
// lets skip this refresh since we are search idle and
// don't necessarily need to refresh. the next search execute cause a

This comment has been minimized.

Copy link
@tlrx

tlrx Nov 23, 2017

Member

nit: truncated comment

@bleskes
Copy link
Member

left a comment

I can't fully comment to the integration with the search system but the IndexShard approach is nice. I left some comments for discussion.

protected void shardOperation(GetRequest request, ShardId shardId, ActionListener<GetResponse> listener) throws IOException {
IndexService indexService = indicesService.indexServiceSafe(shardId.getIndex());
IndexShard indexShard = indexService.getShard(shardId.id());
indexShard.awaitPendingRefresh(b -> {

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 27, 2017

Member

I'm on the fence as to whether we should only do this on non-realtime get. Real time gets don't really relate to refresh cycles (they force a refresh if needed). They are already "efficient" in the sense that they only refresh if they need to (i.e., there's a pending doc change in the version map).

@@ -97,6 +100,19 @@ protected void doExecute(Request request, ActionListener<Response> listener) {

protected abstract Response shardOperation(Request request, ShardId shardId) throws IOException;

protected void shardOperation(Request request, ShardId shardId, ActionListener<Response> listener) throws IOException {

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 27, 2017

Member

maybe call this asyncShardOperation to avoid confusion? also, can you please java doc the fact that this is still called on the networking thread?

protected void shardOperation(TermVectorsRequest request, ShardId shardId, ActionListener<TermVectorsResponse> listener) throws IOException {
IndexService indexService = indicesService.indexServiceSafe(shardId.getIndex());
IndexShard indexShard = indexService.getShard(shardId.id());
indexShard.awaitPendingRefresh(b -> {

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 27, 2017

Member

same comment here w.r.t real time requests.

// once we change the refresh interval we schedule yet another refresh
// to ensure we are in a clean and predictable state.
// it doesn't matter if we move from or to <code>-1</code> in both cases we want
// docs to become visible immediately

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 27, 2017

Member

I guess the important part here is the we need to flush any pending search requests which are waiting for the next refresh. If so, can you add it to the comment? It's not trivial to figure out.

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 27, 2017

Member

Also, if that's the case, maybe we're better off doing it in IndexShard#onSettingsChanged. Then all the corresponding code is contained in the same class.
PS I wonder if we do the right thing with RefreshListeners - we people stop refreshing we should do a refresh and release all pending listeners (and refuse to add new ones). This is, of course, not relating to your PR.

This comment has been minimized.

Copy link
@s1monw

s1monw Nov 27, 2017

Author Contributor

yeah so I wonder if we should rather build it into the refresh listener that is forces a refresh if refreshes are disabled

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 27, 2017

Member

I'm not sure people setting wait_for will want a immediately executed because refreshes are set to -1. I'm +1 to solving it in the refresh listeners - out of scope for this PR of course.

private void setRefreshPending() {
Engine engine = getEngine();
if (isSearchIdle()) {
acquireSearcher("setRefreshPending").close(); // move the shard into non-search idle

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 27, 2017

Member

why is this needed?

This comment has been minimized.

Copy link
@s1monw

s1monw Nov 27, 2017

Author Contributor

it's a bug I removed it and added a test

IndexShard shard = indexService.getShard(0);
boolean hasRefreshed = shard.scheduledRefresh();
if (randomTimeValue == TimeValue.ZERO) {
// with ZERO we are guaranteed to see the doc since we will wait for a refresh in the background

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 27, 2017

Member

I'm not sure what you mean by see the doc? I guess triggering a refresh due to it?

assertFalse(shard.isSearchIdle());
}
assertHitCount(client().prepareSearch().get(), 1);
for (int i = 1; i < numDocs; i++) {

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 27, 2017

Member

what does this extra indexing buy us?

This comment has been minimized.

Copy link
@s1monw

s1monw Nov 27, 2017

Author Contributor

that's just how the test works?

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 27, 2017

Member

we do nothing after the indexing is done nor during the indexing?

assertFalse(shard.scheduledRefresh());
assertTrue(shard.isSearchIdle());
CountDownLatch refreshLatch = new CountDownLatch(1);
client().admin().indices().prepareRefresh().execute(ActionListener.wrap(refreshLatch::countDown)); // async on purpose

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 27, 2017

Member

this is just to speed up the test, right? if so, can you add a comment? if not please tell me what I miss ;)

This comment has been minimized.

Copy link
@s1monw

s1monw Nov 27, 2017

Author Contributor

just to not span a thread, it happens concurrently?

t.join();
}

public void testPendingRefreshWithIntervalChange() throws InterruptedException {

This comment has been minimized.

Copy link
@bleskes
settings = Settings.builder().put(settings).put(IndexSettings.INDEX_SEARCH_IDLE_AFTER.getKey(), TimeValue.timeValueMillis(10))
.build();
scopedSettings.applySettings(settings);
while (primary.isSearchIdle() == false) {

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 27, 2017

Member

lol. assert busy?

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2017

@bleskes I pushed changes..

I also run benchmarks and the results are promising:

|                        Metric |         Task |   Baseline |   Contender |     Diff |   Unit |
|------------------------------:|-------------:|-----------:|------------:|---------:|-------:|
|                 Indexing time |              |     25.308 |     23.4078 | -1.90015 |    min |
|                    Merge time |              |    5.72372 |     3.62805 | -2.09567 |    min |
|                  Refresh time |              |    1.84012 |     0.30375 | -1.53637 |    min |
|                    Flush time |              |   0.425417 |    0.456483 |  0.03107 |    min |
|           Merge throttle time |              |    0.93475 |    0.714817 | -0.21993 |    min |
|            Total Young Gen GC |              |      9.917 |       9.457 |    -0.46 |      s |
|              Total Old Gen GC |              |       0.34 |       0.602 |    0.262 |      s |
|                    Index size |              |    5.83426 |     5.90199 |  0.06773 |     GB |
|               Totally written |              |    16.4158 |     12.9643 | -3.45156 |     GB |
|        Heap used for segments |              |    20.6814 |      3.5807 | -17.1007 |     MB |
|      Heap used for doc values |              |  0.0629349 |   0.0243225 | -0.03861 |     MB |
|           Heap used for terms |              |    19.4335 |     3.35073 | -16.0828 |     MB |
|           Heap used for norms |              |    0.11969 |   0.0499878 |  -0.0697 |     MB |
|          Heap used for points |              |   0.277458 |   0.0367823 | -0.24068 |     MB |
|   Heap used for stored fields |              |   0.787804 |    0.118881 | -0.66892 |     MB |
|                 Segment count |              |        154 |          64 |      -90 |        |
|                Min Throughput | index-append |    71529.7 |       75340 |   3810.3 | docs/s |
|             Median Throughput | index-append |    72673.4 |     75821.7 |  3148.25 | docs/s |
|                Max Throughput | index-append |    73106.8 |     76198.4 |  3091.59 | docs/s |
|       50th percentile latency | index-append |     466.36 |     346.672 | -119.689 |     ms |
|       90th percentile latency | index-append |    771.469 |     704.963 | -66.5056 |     ms |
|       99th percentile latency | index-append |    1006.82 |     1755.92 |  749.099 |     ms |
|      100th percentile latency | index-append |    1081.72 |     1904.62 |    822.9 |     ms |
|  50th percentile service time | index-append |     466.36 |     346.672 | -119.689 |     ms |
|  90th percentile service time | index-append |    771.469 |     704.963 | -66.5056 |     ms |
|  99th percentile service time | index-append |    1006.82 |     1755.92 |  749.099 |     ms |
| 100th percentile service time | index-append |    1081.72 |     1904.62 |    822.9 |     ms |
|                    error rate | index-append |          0 |           0 |        0 |      % |

especially since it seems like the indices that are created by this are much more efficient (better compressed and less merges etc.)

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2017

I like the reduced refresh time, total bytes written and segments memory usage!

@bleskes
Copy link
Member

left a comment

LGTM. Thanks @s1monw .

PS - did you purposely skip over #27500 (comment) ? I'm fine is so, but want to make sure it was not a mistake.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2017

PS - did you purposely skip over #27500 (comment) ? I'm fine is so, but want to make sure it was not a mistake.

I did, I think it's not easy to implement since I don' have a history in that method W.R.T the refresh interval.

@s1monw s1monw merged commit f23ed61 into elastic:master Nov 27, 2017

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

jasontedor added a commit that referenced this pull request Nov 27, 2017

Merge branch 'master' into ccr
* master:
  Skip shard refreshes if shard is `search idle` (#27500)
  Remove workaround in translog rest test (#27530)
  inner_hits: Return an empty _source for nested inner hit when filtering on a field that doesn't exist.
  percolator: Avoid TooManyClauses exception if number of terms / ranges is exactly equal to 1024
  Dedup translog operations by reading in reverse (#27268)
  Ensure logging is configured for CLI commands
  Ensure `doc_stats` are changing even if refresh is disabled (#27505)
  Fix classes that can exit
  Revert "Adjust CombinedDeletionPolicy for multiple commits (#27456)"
  Transpose expected and actual, and remove duplicate info from message. (#27515)
  [DOCS] Fixed broken link in breaking changes

s1monw added a commit to s1monw/elasticsearch that referenced this pull request Nov 28, 2017

Ensure shard is refreshed once it's inactive
Once a shard goes inactive we want the shard to be refreshed if
the refresh interval is default since we might hold on to unnecessary
segments and in the inactive case we stopped indexing and can release
old segments.

Relates to elastic#27500

s1monw added a commit that referenced this pull request Nov 30, 2017

Ensure shard is refreshed once it's inactive (#27559)
Once a shard goes inactive we want the shard to be refreshed if
the refresh interval is default since we might hold on to unnecessary
segments and in the inactive case we stopped indexing and can release
old segments.

Relates to #27500

s1monw added a commit to s1monw/elasticsearch that referenced this pull request Sep 18, 2018

Ensure realtime `_get` and `_termvectors` don't run on the network th…
…read

The change in elastic#27500 introduces this regression that causes `_get` and `_term_vector`
actions to run on the network thread if the realtime flag is set.
This fixes the issue by delegating to the super method forking on the corresponding threadpool.

s1monw added a commit that referenced this pull request Sep 18, 2018

Ensure realtime `_get` and `_termvectors` don't run on the network th…
…read (#33814)

The change in #27500 introduces this regression that causes `_get` and `_term_vector`
actions to run on the network thread if the realtime flag is set.
This fixes the issue by delegating to the super method forking on the corresponding threadpool.

@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

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.