Skip to content

Commit

Permalink
Addresses code review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
Ali Beyad committed Apr 25, 2016
1 parent dd3fbef commit 751f5a8
Show file tree
Hide file tree
Showing 7 changed files with 60 additions and 50 deletions.
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);
}

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();

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

}
Expand Up @@ -386,10 +386,19 @@ private static ClusterState executeIndicesChangesTest(final ClusterState previou
}
final int numDel;
switch (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 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
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());
}

}
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

0 comments on commit 751f5a8

Please sign in to comment.