Ensure external refreshes will also refresh internal searcher to minimize segment creation #27253

s1monw · 2017-11-03T12:32:18Z

We cut over to internal and external IndexReader/IndexSeacher in #26972 which uses
two independent searcher managers. This has the downside that refreshes of the external
reader will never clear the internal version map which in-turn will trigger additional
and potentially unnecessary segment flushes since memory must be freed. Under heavy
indexing load with low refresh intervals this can cause excessive segment creation which
causes high GC activity and significantly increases the required segment merges.

This change adds a dedicated external reference manager that delegates refreshes to the
internal reference manager that then steals the refreshed reader from the internal
reference manager for external usage. This ensures that external and internal readers
are consistent on an external refresh. As a sideeffect this also releases old segments
referenced by the internal reference manager which can potentially hold on to already merged
away segments until it is refreshed due to a flush or indexing activity.

…ize segment creation We cut over to internal and external IndexReader/IndexSeacher in elastic#26972 which uses two independent searcher managers. This has the downside that refreshes of the external reader will never clear the internal version map which in-turn will trigger additional and potentially unnecessary segment flushes since memory must be freed. Under heavy indexing load with low refresh intervals this can cause excessive segment creation which causes high GC activity and significantly increases the required segment merges. This change adds a dedicated external reference manager that delegates refreshes to the internal reference manager that then `steals` the refreshed reader from the internal reference manager for external usage. This ensures that external and internal readers are consistent on an external refresh. As a sideeffect this also releases old segments referenced by the internal reference manager which can potentially hold on to already merged away segments until it is refreshed due to a flush or indexing activity.

bleskes

LGTM. Left some nits that would have helped me while reading this PR. feel free to do with them as you please.

bleskes · 2017-11-08T10:25:51Z

core/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

@@ -224,8 +224,9 @@ public InternalEngine(EngineConfig engineConfig) {
                    throw e;
                }
            }
-            internalSearcherManager = createSearcherManager(new SearcherFactory(), false);
-            externalSearcherManager = createSearcherManager(new SearchFactory(logger, isClosed, engineConfig), true);
+            externalSearcherManager = createSearcherManager(new SearchFactory(logger, isClosed,


nit: this fits on one line. yay.

also, good that this boolean is goon.

bleskes · 2017-11-08T10:26:24Z

core/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

+        private final SearcherFactory searcherFactory;
+        private final SearcherManager internalSearcherManager;
+
+        ExternalSearcherManager(SearcherManager manager, SearcherFactory searcherFactory) throws IOException {


nit: manger -> internalManager

bleskes · 2017-11-08T10:28:39Z

core/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

@@ -456,18 +526,17 @@ private String loadOrGenerateHistoryUUID(final IndexWriter writer, boolean force
        return uuid;
    }

-    private SearcherManager createSearcherManager(SearcherFactory searcherFactory, boolean readSegmentsInfo) throws EngineException {
+    private ExternalSearcherManager createSearcherManager(SearchFactory factory) throws EngineException {
        boolean success = false;
        SearcherManager searcherManager = null;


nit: searcherManager -> internalSearcherManager

bleskes · 2017-11-08T10:29:21Z

core/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

@@ -456,18 +526,17 @@ private String loadOrGenerateHistoryUUID(final IndexWriter writer, boolean force
        return uuid;
    }

-    private SearcherManager createSearcherManager(SearcherFactory searcherFactory, boolean readSegmentsInfo) throws EngineException {
+    private ExternalSearcherManager createSearcherManager(SearchFactory factory) throws EngineException {


nit: factory -> externalSearcherFactory

bleskes · 2017-11-08T10:32:53Z

core/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java

@@ -4226,7 +4226,7 @@ public void assertSameReader(Searcher left, Searcher right) {
        List<LeafReaderContext> rightLeaves = ElasticsearchDirectoryReader.unwrap(right.getDirectoryReader()).leaves();
        assertEquals(rightLeaves.size(), leftLeaves.size());
        for (int i = 0; i < leftLeaves.size(); i++) {
-            assertSame(leftLeaves.get(i).reader(), rightLeaves.get(0).reader());
+            assertSame(leftLeaves.get(i).reader(), rightLeaves.get(i).reader());


yeah suddenly we have a test that triggered it 💃

s1monw · 2017-11-08T13:23:20Z

@elasticmachine test this please

* master: (22 commits) Update Tika version to 1.15 Aggregations: bucket_sort pipeline aggregation (#27152) Introduce templating support to timezone/locale in DateProcessor (#27089) Increase logging on qa:mixed-cluster tests Update to AWS SDK 1.11.223 (#27278) Improve error message for parse failures of completion fields (#27297) Ensure external refreshes will also refresh internal searcher to minimize segment creation (#27253) Remove optimisations to reuse objects when applying a new `ClusterState` (#27317) Decouple `ChannelFactory` from Tcp classes (#27286) Fix find remote when building BWC Remove colons from task and configuration names Add unreleased 5.6.5 version number testCreateSplitIndexToN: do not set `routing_partition_size` to >= `number_of_routing_shards` Snapshot/Restore: better handle incorrect chunk_size settings in FS repo (#26844) Add limits for ngram and shingle settings (#27211) (#27318) Correct comment in index shard test Roll translog generation on primary promotion ObjectParser: Replace IllegalStateException with ParsingException (#27302) scripted_metric _agg parameter disappears if params are provided (#27159) Update discovery-ec2.asciidoc ...

…mize segment creation (#27253) We cut over to internal and external IndexReader/IndexSearcher in #26972 which uses two independent searcher managers. This has the downside that refreshes of the external reader will never clear the internal version map which in-turn will trigger additional and potentially unnecessary segment flushes since memory must be freed. Under heavy indexing load with low refresh intervals this can cause excessive segment creation which causes high GC activity and significantly increases the required segment merges. This change adds a dedicated external reference manager that delegates refreshes to the internal reference manager that then `steals` the refreshed reader from the internal reference manager for external usage. This ensures that external and internal readers are consistent on an external refresh. As a sideeffect this also releases old segments referenced by the internal reference manager which can potentially hold on to already merged away segments until it is refreshed due to a flush or indexing activity.

jasontedor · 2017-11-14T04:44:55Z

@s1monw It appears the "backport pending" label can be removed now?

* 6.x: (27 commits) Reduce synchronization on field data cache [Test] #27342 Fix SearchRequests#testValidate Fail queries with scroll that explicitely set request_cache (#27342) add json-processor support for non-map json types (#27335) Upgrade AWS SDK Jackson Databind to 2.6.7.1 Properly format IndexGraveyard deletion date as date (#27362) Stop responding to ping requests before master abdication (#27329) [Test] Fix POI version in packaging tests Added release notes for 6.0.0 Update 6.0.0-beta1.asciidoc Allow affix settings to specify dependencies (#27161) Tests: Improve size regex in documentation test (#26879) reword comment Ensure external refreshes will also refresh internal searcher to minimize segment creation (#27253) Remove unnecessary logger creation for doc values field data [DOCS] Fixed link to docker content Plugins: Add versionless alias to all security policy codebase properties (#26756) [DOCS] Move X-Pack-specific Docker content (#27333) [Test] Fix S3BlobStoreContainerTests.testNumberOfMultiparts() Set minimum_master_nodes to all nodes for REST tests (#27344) ...

bleskes · 2017-11-23T13:50:52Z

@jasontedor agreed. I removed the backport pending label.

s1monw added :Engine >enhancement v6.1.0 v7.0.0 labels Nov 3, 2017

s1monw requested a review from bleskes November 3, 2017 12:32

s1monw added 4 commits November 3, 2017 12:37

remove redundant modifier

177902e

Merge branch 'master' into only_refresh_internal_manager

3f1a154

add suppress forbidden - I know what I am doing

347aabb

Merge branch 'master' into only_refresh_internal_manager

1e0d10e

bleskes approved these changes Nov 8, 2017

View reviewed changes

s1monw added 2 commits November 8, 2017 12:36

apply review comments from @bleskes

4dd1718

Merge branch 'master' into only_refresh_internal_manager

adfb064

Merge branch 'master' into only_refresh_internal_manager

9d56739

s1monw merged commit a34c2f0 into elastic:master Nov 9, 2017

s1monw added the backport pending label Nov 9, 2017

bleskes removed the backport pending label Nov 23, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure external refreshes will also refresh internal searcher to minimize segment creation #27253

Ensure external refreshes will also refresh internal searcher to minimize segment creation #27253

s1monw commented Nov 3, 2017

bleskes left a comment

bleskes Nov 8, 2017

bleskes Nov 8, 2017

bleskes Nov 8, 2017

bleskes Nov 8, 2017

bleskes Nov 8, 2017

bleskes Nov 8, 2017

s1monw Nov 8, 2017

s1monw commented Nov 8, 2017

jasontedor commented Nov 14, 2017

bleskes commented Nov 23, 2017

Ensure external refreshes will also refresh internal searcher to minimize segment creation #27253

Ensure external refreshes will also refresh internal searcher to minimize segment creation #27253

Conversation

s1monw commented Nov 3, 2017

bleskes left a comment

Choose a reason for hiding this comment

bleskes Nov 8, 2017

Choose a reason for hiding this comment

bleskes Nov 8, 2017

Choose a reason for hiding this comment

bleskes Nov 8, 2017

Choose a reason for hiding this comment

bleskes Nov 8, 2017

Choose a reason for hiding this comment

bleskes Nov 8, 2017

Choose a reason for hiding this comment

bleskes Nov 8, 2017

Choose a reason for hiding this comment

s1monw Nov 8, 2017

Choose a reason for hiding this comment

s1monw commented Nov 8, 2017

jasontedor commented Nov 14, 2017

bleskes commented Nov 23, 2017