Skip to content

Allow freezing searchable snapshots #52653

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.

Sign up for GitHub

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

Merged
DaveCTurner merged 8 commits into from
Mar 2, 2020
Merged

Allow freezing searchable snapshots #52653

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2ca67a8
Allow freezing searchable snapshots
DaveCTurner Feb 21, 2020
2774b7f
Missed by git add
DaveCTurner Feb 21, 2020
206856a
Move to REST tests
DaveCTurner Feb 27, 2020
8e0a73a
Merge branch 'feature/searchable-snapshots' into 2020-02-21-freeze-se…
DaveCTurner Feb 27, 2020
3f0cca1
Revert
DaveCTurner Feb 27, 2020
9e3d4bf
Merge branch 'feature/searchable-snapshots' into 2020-02-21-freeze-se…
DaveCTurner Feb 27, 2020
b10dd74
AssertOK
DaveCTurner Mar 2, 2020
bc85c25
Merge branch 'feature/searchable-snapshots' into 2020-02-21-freeze-se…
DaveCTurner Mar 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ...pshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshots.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ public Map<String, DirectoryFactory> getDirectoryFactories() {

@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) {
Copy link
Contributor Author

@DaveCTurner DaveCTurner Feb 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ywelsch ywelsch Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@DaveCTurner DaveCTurner Feb 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return Optional.of(engineConfig -> new ReadOnlyEngine(engineConfig, null, new TranslogStats(), false, Function.identity()));
}
return Optional.empty();
Expand Down
19 changes: 17 additions & 2 deletions .../test/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import org.elasticsearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse;
import org.elasticsearch.action.admin.indices.recovery.RecoveryResponse;
import org.elasticsearch.action.index.IndexRequestBuilder;
import org.elasticsearch.action.support.IndicesOptions;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.common.Priority;
Expand All @@ -21,14 +22,17 @@
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.indices.recovery.RecoveryState;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.protocol.xpack.frozen.FreezeRequest;
import org.elasticsearch.snapshots.SnapshotInfo;
import org.elasticsearch.test.ESIntegTestCase;
import org.elasticsearch.xpack.core.frozen.action.FreezeIndexAction;
import org.elasticsearch.xpack.frozen.FrozenIndices;
Copy link
Contributor Author

@DaveCTurner DaveCTurner Feb 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ywelsch ywelsch Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For tests, it's ok I think

Copy link
Member

@tlrx tlrx Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@DaveCTurner DaveCTurner Feb 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I moved the tests to the REST suite.

import org.elasticsearch.xpack.searchablesnapshots.cache.CacheService;

import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Locale;
import java.util.concurrent.CountDownLatch;
Expand All @@ -50,7 +54,7 @@ protected boolean addMockInternalEngine() {

@Override
protected Collection<Class<? extends Plugin>> nodePlugins() {
return Collections.singletonList(SearchableSnapshots.class);
return Arrays.asList(SearchableSnapshots.class, FrozenIndices.class);
}

@Override
Expand Down Expand Up @@ -139,6 +143,15 @@ public void testCreateAndRestoreSearchableSnapshot() throws Exception {

assertRecovered(restoredIndexName, originalAllHits, originalBarHits);

if (randomBoolean()) {
assertAcked(client().execute(FreezeIndexAction.INSTANCE, new FreezeRequest(restoredIndexName)).actionGet());
ensureGreen(restoredIndexName);

// ensure that the index really was frozen by searching it without ignore_throttled=false and checking that it returns nothing
final TotalHits totalHits = client().prepareSearch(restoredIndexName).setTrackTotalHits(true).get().getHits().getTotalHits();
assertThat(totalHits, equalTo(new TotalHits(0L, TotalHits.Relation.EQUAL_TO)));
}

internalCluster().fullRestart();
assertRecovered(restoredIndexName, originalAllHits, originalBarHits);

Expand Down Expand Up @@ -173,8 +186,10 @@ private void assertRecovered(String indexName, TotalHits originalAllHits, TotalH
throw new RuntimeException(e);
}
allHits.set(t, client().prepareSearch(indexName).setTrackTotalHits(true)
.setIndicesOptions(IndicesOptions.STRICT_EXPAND_OPEN_FORBID_CLOSED)
.get().getHits().getTotalHits());
barHits.set(t, client().prepareSearch(indexName).setTrackTotalHits(true)
.setIndicesOptions(IndicesOptions.STRICT_EXPAND_OPEN_FORBID_CLOSED)
.setQuery(matchQuery("foo", "bar")).get().getHits().getTotalHits());
});
threads[i].start();
Expand Down