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

Return empty CommitID from ShadowEngine#flush #11554

Merged
merged 1 commit into from Jun 10, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/main/java/org/elasticsearch/index/engine/ShadowEngine.java
Expand Up @@ -23,15 +23,19 @@
import org.apache.lucene.search.SearcherFactory;
import org.apache.lucene.search.SearcherManager;
import org.apache.lucene.store.AlreadyClosedException;
import org.apache.lucene.util.BytesRef;
import org.apache.lucene.util.IOUtils;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.common.lucene.Lucene;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.common.util.concurrent.ReleasableLock;
import org.elasticsearch.index.deletionpolicy.SnapshotIndexCommit;
import org.elasticsearch.index.shard.IndexShardException;

import java.io.FileNotFoundException;
import java.io.IOException;
import java.nio.file.NoSuchFileException;
import java.util.Arrays;
import java.util.List;

Expand All @@ -58,6 +62,7 @@ public class ShadowEngine extends Engine {
/** how long to wait for an index to exist */
public final static String NONEXISTENT_INDEX_RETRY_WAIT = "index.shadow.wait_for_initial_commit";
public final static TimeValue DEFAULT_NONEXISTENT_INDEX_RETRY_WAIT = TimeValue.timeValueSeconds(5);
private static final CommitId EMPTY_COMMIT_ID = new CommitId(new BytesRef());

private volatile SearcherManager searcherManager;

Expand Down Expand Up @@ -139,12 +144,10 @@ public CommitId flush(boolean force, boolean waitIfOngoing) throws EngineExcepti
* dec the store reference which can essentially close the store and unless we can inc the reference
* we can't use it.
*/
CommitId id = null;
store.incRef();
try (ReleasableLock lock = readLock.acquire()) {
// reread the last committed segment infos
lastCommittedSegmentInfos = store.readLastCommittedSegmentsInfo();
id = CommitId.readCommitID(store, lastCommittedSegmentInfos);
} catch (Throwable e) {
if (isClosed.get() == false) {
logger.warn("failed to read latest segment infos on flush", e);
Expand All @@ -155,7 +158,9 @@ public CommitId flush(boolean force, boolean waitIfOngoing) throws EngineExcepti
} finally {
store.decRef();
}
return id;
// We can just return an empty commit ID since this is a read only engine that
// doesn't modify anything so the content of this ID doesn't really matter
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a little note about avoiding race conditions between read and writes on a shared FS ? I think this will people we not see why we opted to EMPTY_COMMIT_ID

return EMPTY_COMMIT_ID;
}

@Override
Expand Down
Expand Up @@ -543,6 +543,9 @@ final static class PreSyncedFlushResponse extends TransportResponse {
}

PreSyncedFlushResponse(Engine.CommitId commitId) {
if (commitId == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious - did you run into this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well we accepted null and further down the road we failed where did expect a non-null value so I added this to fail early

throw new IllegalArgumentException("CommitID must not be null");
}
this.commitId = commitId;
}

Expand Down