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.

Sign up for GitHub

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

Jump to bottom

Use MultiFileTransfer in CCR remote recovery #44514

Merged
merged 10 commits into from
Oct 22, 2019
Merged

Use MultiFileTransfer in CCR remote recovery #44514

merged 10 commits into from
Oct 22, 2019

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Jul 17, 2019

Relates #44468

@dnhatn dnhatn added >enhancement :Distributed/Recovery Anything around constructing a new shard, either from a local or a remote source. :Distributed/CCR Issues around the Cross Cluster State Replication features v8.0.0 v7.4.0 labels Jul 17, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@ywelsch ywelsch requested a review from Tim-Brooks July 18, 2019 07:24
ywelsch
ywelsch suggested changes Jul 18, 2019
View reviewed changes
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

Wow, this makes that code much simpler. I've left two comments, o.w. looking very good already. I think we should also run a stress test (a.k.a. benchmark) on this code, as that used to find subtle bugs in earlier versions of this.

server/src/main/java/org/elasticsearch/indices/recovery/MultiFileWriter.java Show resolved Hide resolved
x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/repository/CcrRepository.java Outdated Show resolved Hide resolved
x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/repository/CcrRepository.java Outdated Show resolved Hide resolved
ywelsch
ywelsch approved these changes Jul 18, 2019
View reviewed changes
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

LGTM. I would like to get a second pair of eyes on this PR + stress test as said above.

server/src/main/java/org/elasticsearch/indices/recovery/MultiFileWriter.java Show resolved Hide resolved
original-brownbear
original-brownbear approved these changes Aug 22, 2019
View reviewed changes
Copy link
Member

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

LGTM, sorry for missing this one!

@colings86 colings86 added v7.5.0 and removed v7.4.0 labels Aug 30, 2019
@dnhatn dnhatn removed the :Distributed/Recovery Anything around constructing a new shard, either from a local or a remote source. label Sep 9, 2019
@dnhatn
Copy link
Member Author

dnhatn commented Oct 22, 2019

I've run several remote recovery benchmarks for this change. The result looks good. No regression. I am going to merge this change to master and 7.x. Thanks for reviewing @ywelsch and @original-brownbear.

@dnhatn dnhatn merged commit 8066e23 into elastic:master Oct 22, 2019
@dnhatn dnhatn deleted the ccr-chunks branch October 22, 2019 03:19
@dnhatn dnhatn added the v7.6.0 label Oct 22, 2019
dnhatn added a commit that referenced this pull request Oct 22, 2019
@dnhatn
Use MultiFileTransfer in CCR remote recovery (#44514)
d0a4bad 
Relates #44468
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Oct 22, 2019
@jasontedor
Merge remote-tracking branch 'elastic/master' into system-jvm-options
07bd9d8 
* elastic/master:
  [Docs] Fix opType options in IndexRequest API example. (elastic#48290)
  Simplify Shard Snapshot Upload Code (elastic#48155)
  Mute ClassificationIT tests (elastic#48338)
  Reenable azure repository tests and remove some randomization in http servers  (elastic#48283)
  Use an env var for the classpath of jar hell task (elastic#48240)
  Refactor FIPS BootstrapChecks to simple checks (elastic#47499)
  Add "format" to "range" queries resulted from optimizing a logical AND (elastic#48073)
  [DOCS][Transform] document limitation regarding rolling upgrade with 7.2, 7.3 (elastic#48118)
  Fail with a better error when if there are no ingest nodes (elastic#48272)
  Fix executing enrich policies stats (elastic#48132)
  Use MultiFileTransfer in CCR remote recovery (elastic#44514)
  Make BytesReference an interface (elastic#48171)
  Also validate source index at put enrich policy time. (elastic#48254)
  Add 'javadoc' task to lifecycle check tasks (elastic#48214)
  Remove option to enable direct buffer pooling (elastic#47956)
  [DOCS] Add 'Selecting gateway and seed nodes' section to CCS docs (elastic#48297)
  Add Enrich Origin (elastic#48098)
  fix incorrect comparison (elastic#48208)
This was referenced Feb 3, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ywelsch ywelsch ywelsch approved these changes

@original-brownbear original-brownbear original-brownbear approved these changes

@Tim-Brooks Tim-Brooks Awaiting requested review from Tim-Brooks

Assignees
No one assigned
Labels
:Distributed/CCR Issues around the Cross Cluster State Replication features >enhancement v7.6.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dnhatn @elasticmachine @ywelsch @original-brownbear @colings86 @jakelandis