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

Switch to write once mode for snapshot metadata files #8782

Merged

Conversation

Projects
None yet
4 participants
@imotov
Copy link
Member

commented Dec 4, 2014

This commit removes creation of in-progress snapshot file and makes creation of the final snapshot file atomic.

Fixes #8696

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2014

w00t! - I will review

@s1monw

View changes

src/main/java/org/elasticsearch/common/blobstore/fs/FsBlobContainer.java Outdated
public void move(String source, String target) throws IOException {
Path sourcePath = path.resolve(source);
Path targetPath = path.resolve(target);
Files.move(sourcePath, targetPath, new CopyOption[]{StandardCopyOption.ATOMIC_MOVE});

This comment has been minimized.

Copy link
@s1monw

s1monw Dec 4, 2014

Contributor

I think this can just be

Files.move(sourcePath, targetPath, StandardCopyOption.ATOMIC_MOVE);
@s1monw

View changes

src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java Outdated
@@ -118,6 +116,8 @@

private static final String SNAPSHOT_PREFIX = "snapshot-";

private static final String TEMP_SNAPSHOT_FILE_PREFIX = "temp-";

This comment has been minimized.

Copy link
@s1monw

s1monw Dec 4, 2014

Contributor

we use pending_ in lucene can we be consistent?

@@ -400,29 +375,8 @@ public MetaData readSnapshotMetaData(SnapshotId snapshotId, ImmutableList<String
@Override
public Snapshot readSnapshot(SnapshotId snapshotId) {
try {
String blobName = snapshotBlobName(snapshotId);

This comment has been minimized.

Copy link
@s1monw

s1monw Dec 4, 2014

Contributor

w00t!

@s1monw

View changes

src/main/java/org/elasticsearch/common/blobstore/BlobContainer.java Outdated
@@ -60,4 +60,6 @@
ImmutableMap<String, BlobMetaData> listBlobs() throws IOException;

ImmutableMap<String, BlobMetaData> listBlobsByPrefix(String blobNamePrefix) throws IOException;

void move(String sourceBlobName, String targetBlobName) throws IOException;

This comment has been minimized.

Copy link
@s1monw

s1monw Dec 4, 2014

Contributor

can I ask you for some JavaDocs? ;)

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2014

I really like it. I think we should have a small unittest in BlobStoreTest but other than that LGTM

@tlrx

View changes

src/main/java/org/elasticsearch/snapshots/SnapshotsService.java Outdated
@@ -295,19 +304,19 @@ public ClusterState execute(ClusterState currentState) {
SnapshotMetaData snapshots = metaData.custom(SnapshotMetaData.TYPE);
ImmutableList.Builder<SnapshotMetaData.Entry> entries = ImmutableList.builder();
for (SnapshotMetaData.Entry entry : snapshots.entries()) {
if (entry.snapshotId().equals(snapshot.snapshotId())) {
if (entry.snapshotId().equals(entry.snapshotId())) {

This comment has been minimized.

Copy link
@tlrx

tlrx Dec 5, 2014

Member

Is that comparison really intended?

}
snapshotsBlobContainer.move(tempBlobName, blobName);

This comment has been minimized.

Copy link
@tlrx

tlrx Dec 5, 2014

Member

👍

@tlrx

View changes

src/main/java/org/elasticsearch/common/blobstore/fs/FsBlobContainer.java Outdated
public void move(String source, String target) throws IOException {
Path sourcePath = path.resolve(source);
Path targetPath = path.resolve(target);
Files.move(sourcePath, targetPath, StandardCopyOption.ATOMIC_MOVE);

This comment has been minimized.

Copy link
@tlrx

tlrx Dec 5, 2014

Member

Are there some major filesystems that don't support atomic moves? If so, I think we should document them as not usable with the FS repository.

@tlrx

This comment has been minimized.

Copy link
Member

commented Dec 5, 2014

The more I dig into the snapshot/restore code, the more I like it :)

LGTM, left two comments.

Snapshot/Restore: switch to write once mode for snapshot metadata files
This commit removes creation of in-progress snapshot file and makes creation of the final snapshot file atomic.

Fixes #8696

@imotov imotov force-pushed the imotov:issue-8696-dont-overwrite-snapshot-files branch to 0b024ad Dec 5, 2014

@imotov imotov merged commit 0b024ad into elastic:master Dec 5, 2014

dadoonet added a commit to dadoonet/elasticsearch-cloud-azure that referenced this pull request Jan 30, 2015

Support new method `BlobContainer#move()`
This has been added in elasticsearch 2.0.0. See elastic/elasticsearch#8782

Wondering if this PR should be split in two parts:

* one that clean the code and which is portable to 1.4, 1.x and master
* one that simply add move() method for master branch

@imotov WDYT?

Closes elastic#49.

dadoonet added a commit to elastic/elasticsearch-cloud-azure that referenced this pull request Feb 12, 2015

Support new method `BlobContainer#move()`
This has been added in elasticsearch 2.0.0. See elastic/elasticsearch#8782

Wondering if this PR should be split in two parts:

* one that clean the code and which is portable to 1.4, 1.x and master
* one that simply add move() method for master branch

@imotov WDYT?

Closes #49.

imotov added a commit to imotov/elasticsearch that referenced this pull request Feb 25, 2015

Snapshot/Restore: add ability to retrieve currently running snapshots
Together with elastic#8782 it should help in the situations simliar to elastic#8887 by adding an ability to get information about currently running snapshot without accessing the repository itself.

Closes elastic#8887

@clintongormley clintongormley removed the review label Mar 19, 2015

@clintongormley clintongormley changed the title Snapshot/Restore: switch to write once mode for snapshot metadata files Switch to write once mode for snapshot metadata files Jun 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.