Make peer recovery send file chunks async #44040

dnhatn · 2019-07-06T20:40:40Z

Relates #36981
Relates #36195
Supersedes #39769

This reverts commit 782920c.

elasticmachine · 2019-07-06T20:40:42Z

Pinging @elastic/es-distributed

original-brownbear · 2019-07-07T08:25:24Z

Jenkins run elasticsearch-ci/2
Jenkins run elasticsearch-ci/docbldesx

(both unrelated failures)

original-brownbear

Some question/suggestion type comments, in general this looks very nice I think :)

server/src/main/java/org/elasticsearch/indices/recovery/MultiFileReader.java

server/src/main/java/org/elasticsearch/indices/recovery/RecoverySourceHandler.java

server/src/main/java/org/elasticsearch/indices/recovery/MultiFileReader.java

dnhatn · 2019-07-08T03:35:37Z

@original-brownbear Thank you for reviewing. I have addressed your comments. Would you mind taking another look?

dnhatn · 2019-07-08T04:26:04Z

Please hold off the review. There's a dead-lock issue after I pushed 5c1977a.

dnhatn · 2019-07-08T14:56:32Z

This is ready again.

server/src/main/java/org/elasticsearch/indices/recovery/RecoverySourceHandler.java

henningandersen

Thanks for looking into this @dnhatn , I have left a few initial comments to consider.

server/src/main/java/org/elasticsearch/indices/recovery/RecoverySourceHandler.java

dnhatn · 2019-07-09T22:11:12Z

@henningandersen I've pushed e8dfd75 to make MultiFileSender an AsyncIOProcessor. Can you please take another look? Thank you!

original-brownbear

Looks good in general :)
One suggestion (that might not be so trivial so feel free to ignore for now) and +1 to Yannick's comment on the exception ignoring . We should def. not do that imo.

original-brownbear · 2019-07-15T09:22:59Z

server/src/main/java/org/elasticsearch/indices/recovery/MultiFileTransfer.java

+            }
+            listener.onResponse(null);
+        } catch (Exception ignored) {
+            // we can safely ignore this exception as it happens after we have released the resource and notified the caller.


++ I don't think we should have this. If we run into a Exception in the listener.onResponse we should fix the listener to handle that exception? Otherwise we're adding another situation where a listener behaves unexpectedly and we won't even see a log line for it?

original-brownbear · 2019-07-15T09:26:57Z

server/src/main/java/org/elasticsearch/indices/recovery/MultiFileTransfer.java

+    }
+
+    private void addItem(long requestSeqId, StoreFileMetaData md, Exception failure) {
+        processor.put(new FileChunkResponseItem(requestSeqId, md, failure), e -> { assert e == null : e; });


I wonder if we should somehow assert that this never blocks? (since AsyncIOProcessor might be using blocking ArrayBlockingQueue#put on the item). It seems to me logically that's not an option with the current code, but maybe something we should make sure of to avoid (admittedly super unlikely) deadlocks in the future?

The current implementation is perfectly fine as we control the number of possible items in the queue. We can assert the remaining capacity before putting an item but this option subjects to a race condition. Another option is to use "offer" instead of "put" in AsyncIOProcessor if the assertion is enabled and blocking is not allowed. I will think about it more.

Yea, I didn't have a good idea on how to do this now either. Just figured I'd raise it :) Either way, once we start using this in tests that use DeterministicTaskQueue we'll probably be guarded against deadlocks/blocking anyway :)

dnhatn · 2019-07-15T13:27:31Z

@ywelsch @original-brownbear Can you take another look? Thank you

original-brownbear

LGTM, thanks @dnhatn !

ywelsch

LGTM

dnhatn · 2019-07-16T13:51:40Z

Thanks everyone :)

This reverts commit a2b4687.

This reverts commit 30b7545.

dnhatn · 2019-07-16T18:21:35Z

I have reverted this change on master and 7.x as the new assertion was tripped.

Relates #44040 Relates #36195

dnhatn added 3 commits July 6, 2019 12:59

Peer recovery sends file chunks non-blocking

25d0156

read ahead

782920c

Revert "read ahead"

eb4b097

This reverts commit 782920c.

dnhatn added >enhancement :Distributed/Recovery Anything around constructing a new shard, either from a local or a remote source. v8.0.0 v7.4.0 labels Jul 6, 2019

dnhatn requested review from s1monw, ywelsch, original-brownbear and henningandersen July 6, 2019 20:40

style check

8d64abe

original-brownbear reviewed Jul 7, 2019

View reviewed changes

dnhatn added 2 commits July 7, 2019 23:24

merge MultiFileReader

5c1977a

inline

93d45c4

dnhatn requested a review from original-brownbear July 8, 2019 03:35

report error in the loop

472bd70

original-brownbear reviewed Jul 8, 2019

View reviewed changes

server/src/main/java/org/elasticsearch/indices/recovery/RecoverySourceHandler.java Outdated Show resolved Hide resolved

henningandersen requested changes Jul 8, 2019

View reviewed changes

dnhatn added 2 commits July 8, 2019 23:24

henning’s comment

97b2099

move checking error down

ec2194f

henningandersen reviewed Jul 9, 2019

View reviewed changes

server/src/main/java/org/elasticsearch/indices/recovery/RecoverySourceHandler.java Outdated Show resolved Hide resolved

dnhatn added 2 commits July 9, 2019 18:08

integrate with AsyncIOProcessor

e8dfd75

Merge branch 'master' into send-chunks

649aad2

original-brownbear requested changes Jul 15, 2019

View reviewed changes

dnhatn added 2 commits July 15, 2019 08:42

let bubble up exception

6d76894

super close

21c2a0a

dnhatn requested review from ywelsch and original-brownbear July 15, 2019 13:27

original-brownbear approved these changes Jul 15, 2019

View reviewed changes

ywelsch approved these changes Jul 16, 2019

View reviewed changes

dnhatn merged commit 30b7545 into elastic:master Jul 16, 2019

dnhatn deleted the send-chunks branch July 16, 2019 13:52

dnhatn added the backport pending label Jul 16, 2019

dnhatn added a commit that referenced this pull request Jul 16, 2019

Make peer recovery send file chunks async (#44040)

a2b4687

dnhatn removed the backport pending label Jul 16, 2019

dnhatn added a commit that referenced this pull request Jul 16, 2019

Revert "Make peer recovery send file chunks async (#44040)"

301c8da

This reverts commit a2b4687.

dnhatn added a commit that referenced this pull request Jul 16, 2019

Revert "Make peer recovery send file chunks async (#44040)"

16eb9ad

This reverts commit 30b7545.

dnhatn removed v7.4.0 v8.0.0 labels Jul 16, 2019

dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Jul 17, 2019

Make peer recovery send file chunks async (elastic#44040)

3aaa986

dnhatn mentioned this pull request Jul 17, 2019

Make peer recovery send file chunks async #44468

Merged

dnhatn added a commit that referenced this pull request Jul 17, 2019

Make peer recovery send file chunks async (#44468)

34f65c6

Relates #44040 Relates #36195

dnhatn added a commit that referenced this pull request Jul 18, 2019

Make peer recovery send file chunks async (#44468)

51180af

Relates #44040 Relates #36195

pcsanwald added v8.0.0 and removed v8.0.0 labels Jul 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make peer recovery send file chunks async #44040

Make peer recovery send file chunks async #44040

dnhatn commented Jul 6, 2019 •

edited

Loading

elasticmachine commented Jul 6, 2019

original-brownbear commented Jul 7, 2019

original-brownbear left a comment

dnhatn commented Jul 8, 2019

dnhatn commented Jul 8, 2019

dnhatn commented Jul 8, 2019

henningandersen left a comment

dnhatn commented Jul 9, 2019

original-brownbear left a comment

original-brownbear Jul 15, 2019

original-brownbear Jul 15, 2019

dnhatn Jul 15, 2019

original-brownbear Jul 15, 2019

dnhatn commented Jul 15, 2019

original-brownbear left a comment

ywelsch left a comment

dnhatn commented Jul 16, 2019

dnhatn commented Jul 16, 2019

Make peer recovery send file chunks async #44040

Make peer recovery send file chunks async #44040

Conversation

dnhatn commented Jul 6, 2019 • edited Loading

elasticmachine commented Jul 6, 2019

original-brownbear commented Jul 7, 2019

original-brownbear left a comment

Choose a reason for hiding this comment

dnhatn commented Jul 8, 2019

dnhatn commented Jul 8, 2019

dnhatn commented Jul 8, 2019

henningandersen left a comment

Choose a reason for hiding this comment

dnhatn commented Jul 9, 2019

original-brownbear left a comment

Choose a reason for hiding this comment

original-brownbear Jul 15, 2019

Choose a reason for hiding this comment

original-brownbear Jul 15, 2019

Choose a reason for hiding this comment

dnhatn Jul 15, 2019

Choose a reason for hiding this comment

original-brownbear Jul 15, 2019

Choose a reason for hiding this comment

dnhatn commented Jul 15, 2019

original-brownbear left a comment

Choose a reason for hiding this comment

ywelsch left a comment

Choose a reason for hiding this comment

dnhatn commented Jul 16, 2019

dnhatn commented Jul 16, 2019

dnhatn commented Jul 6, 2019 •

edited

Loading