Allow freezing searchable snapshots #52653

DaveCTurner · 2020-02-21T17:15:20Z

Today it does not work to freeze an index restored as a searchable snapshot,
because both features try and supply their own Engine implementation. However
a searchable snapshot works with both a ReadOnlyEngine and a FrozenEngine,
so we can check to see if the searchable snapshot is frozen and, if so, avoid
supplying a second Engine in that case.

Relates #50999

Today it does not work to freeze an index restored as a searchable snapshot, because both features try and supply their own `Engine` implementation. However a searchable snapshot works with both a `ReadOnlyEngine` and a `FrozenEngine`, so we can check to see if the searchable snapshot is frozen and, if so, avoid supplying a second `Engine` in that case.

elasticmachine · 2020-02-21T17:15:22Z

Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore)

DaveCTurner · 2020-02-21T17:16:20Z

...snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshots.java

@@ -111,7 +111,8 @@ public void onRepositoriesModule(RepositoriesModule repositoriesModule) {

    @Override
    public Optional<EngineFactory> getEngineFactory(IndexSettings indexSettings) {
-        if (SearchableSnapshotRepository.SNAPSHOT_DIRECTORY_FACTORY_KEY.equals(INDEX_STORE_TYPE_SETTING.get(indexSettings.getSettings()))) {
+        if (SearchableSnapshotRepository.SNAPSHOT_DIRECTORY_FACTORY_KEY.equals(INDEX_STORE_TYPE_SETTING.get(indexSettings.getSettings()))
+                && indexSettings.getSettings().getAsBoolean("index.frozen", false) == false) {


Not sure about introducing this string literal here, vs adding a dependency on the frozen-indices module and referring to the setting directly.

I would prefer to avoid the dependency and just use the String literal (we have other such cases in our code base).

I also wonder if we need to handle closed replicated snapshot indices (i.e. use NoOpEngine for those).

We already bypass this mechanism for closed indices:

elasticsearch/server/src/main/java/org/elasticsearch/indices/IndicesService.java

Lines 622 to 627 in 0d1e67d

private EngineFactory getEngineFactory(final IndexSettings idxSettings) {

final IndexMetaData indexMetaData = idxSettings.getIndexMetaData();

if (indexMetaData != null && indexMetaData.getState() == IndexMetaData.State.CLOSE) {

// NoOpEngine takes precedence as long as the index is closed

return NoOpEngine::new;

}

I added a test case showing that closing and reopening a searchable snapshot index does work.

DaveCTurner · 2020-02-21T17:17:25Z

...src/test/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java

 import org.elasticsearch.snapshots.SnapshotInfo;
 import org.elasticsearch.test.ESIntegTestCase;
+import org.elasticsearch.xpack.core.frozen.action.FreezeIndexAction;
+import org.elasticsearch.xpack.frozen.FrozenIndices;


Similarly, not sure this is appropriate in terms of dependencies but I figured these interdependencies were ok for tests.

For tests, it's ok I think

I think it's OK too, but I'd like to avoid adding too many test conditionals to this test. Maybe we could test this in AbstractSearchableSnapshotsRestTestCase instead?

Ok, I moved the tests to the REST suite.

…archable-snapshots

DaveCTurner · 2020-03-02T09:35:09Z

@tlrx @ywelsch this is ready for review.

tlrx

LGTM

tlrx · 2020-03-02T10:07:25Z

...ava/org/elasticsearch/xpack/searchablesnapshots/AbstractSearchableSnapshotsRestTestCase.java

+    public void testSearchResultsWhenFrozen() throws Exception {
+        runSearchableSnapshotsTest((restoredIndexName, numDocs) -> {
+            final Request freezeRequest = new Request(HttpPost.METHOD_NAME, restoredIndexName + "/_freeze");
+            client().performRequest(freezeRequest);


Can you assertOK() this?

tlrx · 2020-03-02T10:07:44Z

...ava/org/elasticsearch/xpack/searchablesnapshots/AbstractSearchableSnapshotsRestTestCase.java

+    public void testCloseAndReopen() throws Exception {
+        runSearchableSnapshotsTest((restoredIndexName, numDocs) -> {
+            final Request closeRequest = new Request(HttpPost.METHOD_NAME, restoredIndexName + "/_close");
+            client().performRequest(closeRequest);


Can you assertOK() this?

tlrx · 2020-03-02T10:07:48Z

...ava/org/elasticsearch/xpack/searchablesnapshots/AbstractSearchableSnapshotsRestTestCase.java

+            ensureGreen(restoredIndexName);
+
+            final Request openRequest = new Request(HttpPost.METHOD_NAME, restoredIndexName + "/_open");
+            client().performRequest(openRequest);


Can you assertOK() this?

DaveCTurner · 2020-03-02T10:17:07Z

Thanks @tlrx. Good catch on the missing assertOKs, added in b10dd74.

…archable-snapshots

DaveCTurner added >enhancement :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Feb 21, 2020

DaveCTurner requested review from tlrx and ywelsch February 21, 2020 17:15

DaveCTurner commented Feb 21, 2020

View reviewed changes

DaveCTurner mentioned this pull request Feb 21, 2020

Lazy snapshot restores #50999

Closed

19 tasks

DaveCTurner added 5 commits February 21, 2020 17:32

Missed by git add

2774b7f

Move to REST tests

206856a

Merge branch 'feature/searchable-snapshots' into 2020-02-21-freeze-se…

8e0a73a

…archable-snapshots

Revert

3f0cca1

Merge branch 'feature/searchable-snapshots' into 2020-02-21-freeze-se…

9e3d4bf

…archable-snapshots

tlrx approved these changes Mar 2, 2020

View reviewed changes

AssertOK

b10dd74

Merge branch 'feature/searchable-snapshots' into 2020-02-21-freeze-se…

bc85c25

…archable-snapshots

DaveCTurner merged commit 050583e into elastic:feature/searchable-snapshots Mar 2, 2020

DaveCTurner deleted the 2020-02-21-freeze-searchable-snapshots branch March 2, 2020 11:59

tlrx mentioned this pull request Apr 6, 2020

Merge feature/searchable-snapshots branch into master #54803

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow freezing searchable snapshots #52653

Allow freezing searchable snapshots #52653

DaveCTurner commented Feb 21, 2020 •

edited

Loading

elasticmachine commented Feb 21, 2020

DaveCTurner Feb 21, 2020

ywelsch Feb 24, 2020

DaveCTurner Feb 27, 2020

DaveCTurner Feb 21, 2020

ywelsch Feb 24, 2020

tlrx Feb 24, 2020

DaveCTurner Feb 27, 2020

DaveCTurner commented Mar 2, 2020

tlrx left a comment

tlrx Mar 2, 2020

tlrx Mar 2, 2020

tlrx Mar 2, 2020

DaveCTurner commented Mar 2, 2020 •

edited

Loading

	private EngineFactory getEngineFactory(final IndexSettings idxSettings) {
	final IndexMetaData indexMetaData = idxSettings.getIndexMetaData();
	if (indexMetaData != null && indexMetaData.getState() == IndexMetaData.State.CLOSE) {
	// NoOpEngine takes precedence as long as the index is closed
	return NoOpEngine::new;
	}

Allow freezing searchable snapshots #52653

Allow freezing searchable snapshots #52653

Conversation

DaveCTurner commented Feb 21, 2020 • edited Loading

elasticmachine commented Feb 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner commented Mar 2, 2020

tlrx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner commented Mar 2, 2020 • edited Loading

DaveCTurner commented Feb 21, 2020 •

edited

Loading

DaveCTurner commented Mar 2, 2020 •

edited

Loading