Allow freezing searchable snapshots by DaveCTurner · Pull Request #52653 · elastic/elasticsearch

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

    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/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

	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;
	}

Conversation

DaveCTurner commented Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Feb 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaveCTurner commented Mar 2, 2020

Uh oh!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaveCTurner commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DaveCTurner commented Feb 21, 2020 •

edited

Loading

DaveCTurner commented Mar 2, 2020 •

edited

Loading