Add BlobContainer.writeBlobAtomic() #30902

tlrx · 2018-05-28T11:11:26Z

This commit adds a new writeBlobAtomic() method to the BlobContainer interface that can be implemented by repository implementations which support atomic writes operations.

When the repository does not support atomic writes, this method just delegate the write operation to the usual writeBlob() method.

As a follow up method, I think we could just remove the BlobContainer.move() method now it's just used during repository verification.

Related to #30680

elasticmachine · 2018-05-28T11:11:28Z

Pinging @elastic/es-distributed

ywelsch

I would prefer if you also replace the last occurrence of move, and then remove the move method. In terms of testing, I wonder if we need to inject more failures now so that FSBlobContainer call randomly fail between the write and atomic move.

ywelsch · 2018-05-28T16:39:45Z

server/src/main/java/org/elasticsearch/common/blobstore/fs/FsBlobContainer.java

@@ -131,6 +132,28 @@ public void writeBlob(String blobName, InputStream inputStream, long blobSize) t
        IOUtils.fsync(path, true);
    }

+    @Override
+    public void writeBlobAtomic(final String blobName, final InputStream inputStream, final long blobSize) throws IOException {
+        final Path tempBlobPath = path.resolve("pending-" + blobName + "-" + UUIDs.randomBase64UUID());


There is a relation between the name here and between ChecksumBlobStoreFormat.isTempBlobName, which is now easy to break. I think we should make this more explicit.

tlrx · 2018-05-29T09:45:34Z

I would prefer if you also replace the last occurrence of move, and then remove the move method.

I removed the last occurance (but added one in test). I'll remove the method in a follow up pull request if that's fine for you (I expect to remove many things in repository plugins related to this).

I wonder if we need to inject more failures now so that FSBlobContainer

I agree. I updated the code, can you have another look please?

I also move two tests classes were they should be and fix the checkstyle violations on the way.

ywelsch

I've left two more comments.

ywelsch · 2018-06-04T08:04:47Z

server/src/main/java/org/elasticsearch/common/blobstore/BlobContainer.java

@@ -74,6 +74,28 @@
     */
    void writeBlob(String blobName, InputStream inputStream, long blobSize) throws IOException;

+    /**
+     * Reads blob content from the input stream and writes it to the container in a new blob with the given name,
+     * using an atomic write operation if then implementation supports it. When atomic writes are not supported,


s/then/the/
Also it's rather the opposite. If the implementation support atomic writes by default, it then delegates to writeBlob...?

"When the BlobContainer implementation does not provide a specific implementation for wireBlobAtomic, then the usual writeBlob method is used." Would that be better?

ywelsch · 2018-06-04T08:10:53Z

server/src/main/java/org/elasticsearch/common/blobstore/fs/FsBlobContainer.java

+            }
+            Files.move(tempBlobPath, blobPath, StandardCopyOption.ATOMIC_MOVE);
+        } finally {
+            IOUtils.deleteFilesIgnoringExceptions(tempBlobPath);


Should we really ignore all exceptions here? This means we can up with lingering files without any warnings.
I preferred the previous approach of:

} catch (IOException ex) { // temporary blob creation or move failed - try cleaning up try { snapshotsBlobContainer.deleteBlobIgnoringIfNotExists(tempBlobName); } catch (IOException e) { ex.addSuppressed(e); } throw ex; }

Right, good point

tlrx · 2018-06-04T13:35:41Z

Thanks @ywelsch - I applied your feedback. Can you have another look please?

ywelsch

I've left one last comment, o.w. LGTM. Thanks for this.

ywelsch · 2018-06-04T13:39:05Z

server/src/main/java/org/elasticsearch/common/blobstore/fs/FsBlobContainer.java

+            }
+            throw ex;
+        } finally {
+            IOUtils.deleteFilesIgnoringExceptions(tempBlobPath);


this line should be removed now.

raaah thanks

I should pay a 🍺 every time I forgot something like that

tlrx · 2018-06-04T18:47:35Z

@elasticmachine test this please

This commit adds a new writeBlobAtomic() method to the BlobContainer interface that can be implemented by repository implementations which support atomic writes operations. When the repository does not support atomic writes, this method just delegate the write operation to the usual writeBlob() method. Related to elastic#30680

tlrx · 2018-06-05T11:01:01Z

Thanks @ywelsch

This commit adds a new writeBlobAtomic() method to the BlobContainer interface that can be implemented by repository implementations which support atomic writes operations. When the BlobContainer implementation does not provide a specific implementation of writeBlobAtomic(), then the writeBlob() method is used. Related to #30680

* elastic/master: [Tests] Muting RatedRequestsTests#testXContentParsingIsNotLenient TEST: Retry synced-flush if ongoing ops on primary (elastic#30978) Fix docs build. Only auto-update license signature if all nodes ready (elastic#30859) Add BlobContainer.writeBlobAtomic() (elastic#30902) Add a doc value format to binary fields. (elastic#30860) Take into account the return value of TcpTransport.readMessageLength(...) in Netty4SizeHeaderFrameDecoder Move caching of the size of a directory to `StoreDirectory`. (elastic#30581) Clarify docs about boolean operator precedence. (elastic#30808) Docs: remove notes on sparsity. (elastic#30905) Fix MatchPhrasePrefixQueryBuilderTests#testPhraseOnFieldWithNoTerms run overflow forecast a 2nd time as regression test for elastic/ml-cpp#110 (elastic#30969) Improve documentation of dynamic mappings. (elastic#30952) Decouple MultiValueMode. (elastic#31075) Docs: Clarify constraints on scripted similarities. (elastic#31076)

* 6.x: Share common readFrom/writeTo code in AcknowledgeResponse (#30983) [Tests] Muting RatedRequestsTests#testXContentParsingIsNotLenient Fix rest test skip version Fix docs build. Add a doc value format to binary fields. (#30860) Only auto-update license signature if all nodes ready (#30859) Add BlobContainer.writeBlobAtomic() (#30902) Move caching of the size of a directory to `StoreDirectory`. (#30581) Clarify docs about boolean operator precedence. (#30808) Docs: remove notes on sparsity. (#30905) Improve documentation of dynamic mappings. (#30952) Decouple MultiValueMode. (#31075) Docs: Clarify constraints on scripted similarities. (#31076)

* master: Removing erroneous repeat Adapt bwc versions after backporting #30983 to 6.4 [Tests] Muting RatedRequestsTests#testXContentParsingIsNotLenient TEST: Retry synced-flush if ongoing ops on primary (#30978) Fix docs build. Only auto-update license signature if all nodes ready (#30859) Add BlobContainer.writeBlobAtomic() (#30902) Add a doc value format to binary fields. (#30860)

* ccr: [DOCS] Creates rest-api folder in docs [Rollup] Disallow index patterns that match the rollup index (elastic#30491) Replace exact numDocs by soft-del count in SegmentInfo (elastic#31086) Upgrade to Lucene-7.4.0-snapshot-0a7c3f462f (elastic#31073) Add cors support to NioHttpServerTransport (elastic#30827) [DOCS] Fixes security example (elastic#31082) Allow terms query in _rollup_search (elastic#30973) Removing erroneous repeat Adapt bwc versions after backporting elastic#30983 to 6.4 [Tests] Muting RatedRequestsTests#testXContentParsingIsNotLenient TEST: Retry synced-flush if ongoing ops on primary (elastic#30978) Fix docs build. Only auto-update license signature if all nodes ready (elastic#30859) Add BlobContainer.writeBlobAtomic() (elastic#30902) Add a doc value format to binary fields. (elastic#30860)

tlrx added review :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v7.0.0 >refactoring v6.4.0 labels May 28, 2018

tlrx requested a review from ywelsch May 28, 2018 11:11

ywelsch reviewed May 28, 2018

View reviewed changes

ywelsch reviewed Jun 4, 2018

View reviewed changes

ywelsch approved these changes Jun 4, 2018

View reviewed changes

tlrx added 6 commits June 5, 2018 08:45

Make temp blob name more explicit

b1b2af7

Remove last usage of move()

793a9e0

Inject more failures, fix checkstyle violations

c7c5f19

Apply fidebaque

795a0a1

Remove leftover

9a71131

tlrx force-pushed the writeblobatomic branch from e521ad3 to 9a71131 Compare June 5, 2018 06:46

tlrx merged commit 9531b7b into elastic:master Jun 5, 2018

tlrx deleted the writeblobatomic branch June 5, 2018 11:00

colings86 added the v7.0.0-beta1 label Feb 7, 2019

colings86 removed the v7.0.0 label Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BlobContainer.writeBlobAtomic() #30902

Add BlobContainer.writeBlobAtomic() #30902

tlrx commented May 28, 2018

elasticmachine commented May 28, 2018

ywelsch left a comment

ywelsch May 28, 2018

tlrx May 29, 2018

tlrx commented May 29, 2018

ywelsch left a comment

ywelsch Jun 4, 2018

tlrx Jun 4, 2018

ywelsch Jun 4, 2018

ywelsch Jun 4, 2018

tlrx Jun 4, 2018

tlrx commented Jun 4, 2018

ywelsch left a comment

ywelsch Jun 4, 2018

tlrx Jun 4, 2018

tlrx commented Jun 4, 2018

tlrx commented Jun 5, 2018

Add BlobContainer.writeBlobAtomic() #30902

Add BlobContainer.writeBlobAtomic() #30902

Conversation

tlrx commented May 28, 2018

elasticmachine commented May 28, 2018

ywelsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlrx commented May 29, 2018

ywelsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlrx commented Jun 4, 2018

ywelsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlrx commented Jun 4, 2018

tlrx commented Jun 5, 2018