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

Adds tombstones to cluster state for index deletions #17265

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,16 @@ final public class IndexGraveyard implements MetaData.Custom {
public static final IndexGraveyard PROTO = new IndexGraveyard(new ArrayList<>());
public static final String TYPE = "index-graveyard";
private static final ParseField TOMBSTONES_FIELD = new ParseField("tombstones");
private static final ObjectParser<List<Tombstone>, Void> GRAVEYARD_PARSER = new ObjectParser<>("index_graveyard", ArrayList::new);
private static final ObjectParser<List<Tombstone>, Void> GRAVEYARD_PARSER;
static {
GRAVEYARD_PARSER = new ObjectParser<>("index_graveyard", ArrayList::new);
GRAVEYARD_PARSER.declareObjectArray(List::addAll, Tombstone.getParser(), TOMBSTONES_FIELD);
}

private final List<Tombstone> tombstones;

private IndexGraveyard(final List<Tombstone> list) {
assert list != null;
tombstones = Collections.unmodifiableList(list);
Copy link
Member

Choose a reason for hiding this comment

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

I think that we want a null check here?

Copy link
Author

Choose a reason for hiding this comment

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

Its a private constructor, only called from the Builder, would an assert be more appropriate?

Copy link
Member

@jasontedor jasontedor Apr 25, 2016

Choose a reason for hiding this comment

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

Its a private constructor, only called from the Builder, would an assert be more appropriate?

An assert is fine.

}

Expand Down Expand Up @@ -109,7 +111,7 @@ public boolean equals(Object obj) {

@Override
public int hashCode() {
return Objects.hash(tombstones);
return tombstones.hashCode();
}

/**
Expand Down Expand Up @@ -175,6 +177,7 @@ public static IndexGraveyard.Builder builder(final IndexGraveyard graveyard) {
final public static class Builder {
private List<Tombstone> tombstones;
private int numPurged = -1;
private long currentTime = System.currentTimeMillis();
Copy link
Member

Choose a reason for hiding this comment

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

Can it be final?


private Builder() {
tombstones = new ArrayList<>();
Expand All @@ -195,7 +198,7 @@ public List<Tombstone> tombstones() {
* Add a deleted index to the list of tombstones in the cluster state.
*/
public Builder addTombstone(final Index index) {
tombstones.add(new Tombstone(index, System.currentTimeMillis()));
tombstones.add(new Tombstone(index, currentTime));
return this;
}

Expand Down Expand Up @@ -309,11 +312,10 @@ public void writeTo(final StreamOutput out) throws IOException {

@Override
public IndexGraveyard apply(final MetaData.Custom previous) {
@SuppressWarnings("unchecked")
final IndexGraveyard old = (IndexGraveyard) previous;
@SuppressWarnings("unchecked") final IndexGraveyard old = (IndexGraveyard) previous;
if (removedCount > old.tombstones.size()) {
throw new IllegalStateException("IndexGraveyardDiff cannot remove " + removedCount + " entries from " +
old.tombstones.size() + " tombstones.");
throw new IllegalStateException("IndexGraveyardDiff cannot remove [" + removedCount + "] entries from [" +
old.tombstones.size() + "] tombstones.");
}
final List<Tombstone> newTombstones = new ArrayList<>(old.tombstones.subList(removedCount, old.tombstones.size()));
for (Tombstone tombstone : added) {
Expand Down Expand Up @@ -341,16 +343,16 @@ final public static class Tombstone implements ToXContent, Writeable<Tombstone>
private static final String INDEX_KEY = "index";
private static final String DELETE_DATE_IN_MILLIS_KEY = "delete_date_in_millis";
private static final String DELETE_DATE_KEY = "delete_date";
private static final ObjectParser<Tombstone.Builder, Void> TOMBSTONE_PARSER =
new ObjectParser<>("tombstoneEntry", Tombstone.Builder::new);
private static final ObjectParser<Tombstone.Builder, Void> TOMBSTONE_PARSER;
static {
TOMBSTONE_PARSER = new ObjectParser<>("tombstoneEntry", Tombstone.Builder::new);
TOMBSTONE_PARSER.declareObject(Tombstone.Builder::index, Index::parseIndex, new ParseField(INDEX_KEY));
TOMBSTONE_PARSER.declareLong(Tombstone.Builder::deleteDateInMillis, new ParseField(DELETE_DATE_IN_MILLIS_KEY));
TOMBSTONE_PARSER.declareString((b,s) -> {}, new ParseField(DELETE_DATE_KEY));
TOMBSTONE_PARSER.declareString((b, s) -> {}, new ParseField(DELETE_DATE_KEY));
}

static BiFunction<XContentParser, Void, Tombstone> getParser() {
return (p,c) -> TOMBSTONE_PARSER.apply(p, c).build();
return (p, c) -> TOMBSTONE_PARSER.apply(p, c).build();
}

private final Index index;
Expand Down Expand Up @@ -404,8 +406,7 @@ public boolean equals(final Object other) {
if (other == null || getClass() != other.getClass()) {
return false;
}
@SuppressWarnings("unchecked")
Tombstone that = (Tombstone) other;
@SuppressWarnings("unchecked") Tombstone that = (Tombstone) other;
if (index.equals(that.index) == false) {
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -931,8 +931,7 @@ public Builder indexGraveyard(final IndexGraveyard indexGraveyard) {
}

public IndexGraveyard indexGraveyard() {
@SuppressWarnings("unchecked")
IndexGraveyard graveyard = (IndexGraveyard) getCustom(IndexGraveyard.TYPE);
@SuppressWarnings("unchecked") IndexGraveyard graveyard = (IndexGraveyard) getCustom(IndexGraveyard.TYPE);
return graveyard;
}

Expand Down
6 changes: 2 additions & 4 deletions core/src/main/java/org/elasticsearch/index/Index.java
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,8 @@ public class Index implements Writeable<Index>, ToXContent {
private final String uuid;

public Index(String name, String uuid) {
Objects.requireNonNull(name);
Objects.requireNonNull(uuid);
this.name = name.intern();
this.uuid = uuid.intern();
this.name = Objects.requireNonNull(name).intern();
this.uuid = Objects.requireNonNull(uuid).intern();
}

public Index(StreamInput in) throws IOException {
Expand Down
19 changes: 9 additions & 10 deletions core/src/main/java/org/elasticsearch/indices/IndicesService.java
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.BiFunction;
import java.util.function.Predicate;
import java.util.stream.Collectors;

Expand Down Expand Up @@ -567,11 +566,11 @@ void deleteIndexStore(String reason, IndexMetaData metaData, ClusterState cluste
}

private void deleteIndexStore(String reason, Index index, IndexSettings indexSettings) throws IOException {
deleteIndexStoreWithPredicate(reason, index, indexSettings, DEFAULT_INDEX_DELETION_PREDICATE);
deleteIndexStoreIfDeletionAllowed(reason, index, indexSettings, DEFAULT_INDEX_DELETION_PREDICATE);
}

private void deleteIndexStoreWithPredicate(final String reason, final Index index, final IndexSettings indexSettings,
final IndexDeletionPredicate predicate) throws IOException {
private void deleteIndexStoreIfDeletionAllowed(final String reason, final Index index, final IndexSettings indexSettings,
final IndexDeletionAllowedPredicate predicate) throws IOException {
boolean success = false;
try {
// we are trying to delete the index store here - not a big deal if the lock can't be obtained
Expand Down Expand Up @@ -696,9 +695,9 @@ public IndexMetaData verifyIndexIsDeleted(final Index index, final ClusterState
}
final IndexSettings indexSettings = buildIndexSettings(metaData);
try {
deleteIndexStoreWithPredicate("stale deleted index", index, indexSettings, ALWAYS_TRUE);
deleteIndexStoreIfDeletionAllowed("stale deleted index", index, indexSettings, ALWAYS_TRUE);
} catch (IOException e) {
// we just warn about the exception here because if deleteIndexStoreWithPredicate
// we just warn about the exception here because if deleteIndexStoreIfDeletionAllowed
// throws an exception, it gets added to the list of pending deletes to be tried again
logger.warn("[{}] failed to delete index on disk", e, metaData.getIndex());
Copy link
Member

Choose a reason for hiding this comment

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

I really don't like that we just warn if an exception occurs during deletion, is there any sort of retry that is triggered by this? If not, we may want to add a followup to add that

Copy link
Author

Choose a reason for hiding this comment

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

The deleteIndexStoreWithPredicate method adds the deletion to the pending queue for trying again.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, would be good to add a comment for that so future devs looking at this don't worry about the exception swallowing

}
Expand Down Expand Up @@ -1119,12 +1118,12 @@ public void onRemoval(RemovalNotification<IndicesRequestCache.Key, IndicesReques
}

@FunctionalInterface
interface IndexDeletionPredicate extends BiFunction<Index, IndexSettings, Boolean> {
Boolean apply(Index index, IndexSettings indexSettings);
interface IndexDeletionAllowedPredicate {
boolean apply(Index index, IndexSettings indexSettings);
}

private final IndexDeletionPredicate DEFAULT_INDEX_DELETION_PREDICATE =
private final IndexDeletionAllowedPredicate DEFAULT_INDEX_DELETION_PREDICATE =
(Index index, IndexSettings indexSettings) -> canDeleteIndexContents(index, indexSettings);
private final IndexDeletionPredicate ALWAYS_TRUE = (Index index, IndexSettings indexSettings) -> true;
private final IndexDeletionAllowedPredicate ALWAYS_TRUE = (Index index, IndexSettings indexSettings) -> true;

}
Original file line number Diff line number Diff line change
Expand Up @@ -386,10 +386,19 @@ private static ClusterState executeIndicesChangesTest(final ClusterState previou
}
final int numDel;
switch (deletionQuantity) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to see some braces on these case blocks.

case DELETE_ALL: numDel = stateIndices.size(); break;
case DELETE_NONE: numDel = 0; break;
case DELETE_RANDOM: numDel = randomIntBetween(0, Math.max(stateIndices.size() - 1, 0)); break;
default: throw new IllegalArgumentException("Unhandled mode " + deletionQuantity);
case DELETE_ALL: {
numDel = stateIndices.size();
break;
}
case DELETE_NONE: {
numDel = 0;
break;
}
case DELETE_RANDOM: {
numDel = randomIntBetween(0, Math.max(stateIndices.size() - 1, 0));
break;
}
default: throw new AssertionError("Unhandled mode [" + deletionQuantity + "]");
}
final List<Index> addedIndices = addIndices(numAdd, randomAsciiOfLengthBetween(5, 10));
List<Index> delIndices = delIndices(numDel, stateIndices);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,13 @@
import org.elasticsearch.test.InternalTestCluster.RestartCallback;

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.List;

import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery;
import static org.elasticsearch.index.query.QueryBuilders.matchQuery;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.nullValue;
Expand Down Expand Up @@ -555,4 +557,23 @@ public void testArchiveBrokenClusterSettings() throws Exception {
+ ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getKey()));
assertHitCount(client().prepareSearch().setQuery(matchAllQuery()).get(), 1L);
}


/**
* Creates a shadow replica index and asserts that the index creation was acknowledged.
* Can only be invoked on a cluster where each node has been configured with shared data
* paths and the other necessary settings for shadow replicas.
*/
private void createShadowReplicaIndex(final String name, final Path dataPath, final int numReplicas) {
assert Files.exists(dataPath);
assert numReplicas >= 0;
final Settings idxSettings = Settings.builder()
.put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1)
.put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, numReplicas)
.put(IndexMetaData.SETTING_DATA_PATH, dataPath.toAbsolutePath().toString())
.put(IndexMetaData.SETTING_SHADOW_REPLICAS, true)
.build();
assertAcked(prepareCreate(name).setSettings(idxSettings).get());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -708,23 +708,6 @@ public final void createIndex(String... names) {
}
}

/**
* Creates a shadow replica index and asserts that the index creation was acknowledged.
* Can only be invoked on a cluster where each node has been configured with shared data
* paths and the other necessary settings for shadow replicas.
*/
public final void createShadowReplicaIndex(final String name, final Path dataPath, final int numReplicas) {
assert Files.exists(dataPath);
assert numReplicas >= 0;
final Settings idxSettings = Settings.builder()
.put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1)
.put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, numReplicas)
.put(IndexMetaData.SETTING_DATA_PATH, dataPath.toAbsolutePath().toString())
.put(IndexMetaData.SETTING_SHADOW_REPLICAS, true)
.build();
assertAcked(prepareCreate(name).setSettings(idxSettings).get());
}

/**
* Creates a new {@link CreateIndexRequestBuilder} with the settings obtained from {@link #indexSettings()}.
*/
Expand Down