Skip to content

Conversation

@tlrx
Copy link
Member

@tlrx tlrx commented Apr 6, 2020

This is a backport of #54803 for 7.x.

This pull request cherry picks the squashed commit from #54803 with the additional commits:

tlrx and others added 3 commits April 6, 2020 17:02
This commit merges the searchable-snapshots feature branch into master.
See elastic#54803 for the complete list of squashed commits.

Co-authored-by: David Turner <[email protected]>
Co-authored-by: Yannick Welsch <[email protected]>
Co-authored-by: Lee Hinman <[email protected]>
Co-authored-by: Andrei Dan <[email protected]>
@tlrx tlrx added :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs backport labels Apr 6, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore)

@tlrx tlrx requested review from DaveCTurner and andreidan April 7, 2020 10:03
}

public DeleteAction(StreamInput in) throws IOException {
if (in.getVersion().onOrAfter(Version.V_7_8_0)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@andreidan Version has been adjusted here


@Override
public void writeTo(StreamOutput out) throws IOException {
if (out.getVersion().onOrAfter(Version.V_7_8_0)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@andreidan Version has been adjusted here

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @tlrx

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

I left a question about the loss of a feature flag conditional.

I'm also concerned about the number of deviations away from List.of -- I think this will make backporting future work much harder. Could you try and bring these more in line between the branches, ideally with import org.elasticsearch.common.collect.List rather than using the fully-qualified name inline?

@tlrx
Copy link
Member Author

tlrx commented Apr 7, 2020

@DaveCTurner Thanks for your feedback. I tried to minimize the deviations away from List.of event though is most cases it ends up being a complete qualification of the org.elasticsearch.common.collect.List class.

@tlrx tlrx requested a review from DaveCTurner April 7, 2020 10:38
@DaveCTurner
Copy link
Contributor

it ends up being a complete qualification of the org.elasticsearch.common.collect.List class

Ugh, right, I see, if we also import java.util.List then we can't have both. Thanks anyway, I think it helps to align whitespace if nothing else.

@DaveCTurner
Copy link
Contributor

LGTM, thanks @tlrx and @andreidan

Copy link
Contributor

@andreidan andreidan left a comment

Choose a reason for hiding this comment

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

LGTM, great feature @tlrx @DaveCTurner

@tlrx tlrx merged commit 4d36917 into elastic:7.x Apr 7, 2020
@tlrx tlrx deleted the searchable-snapshots-on-7.x branch April 7, 2020 11:28
@tlrx
Copy link
Member Author

tlrx commented Apr 7, 2020

Thanks David and Andrei!

@tlrx tlrx added the v7.8.0 label Apr 7, 2020
tlrx added a commit that referenced this pull request Apr 7, 2020
This commit adjusts some versions on master now searchable 
snapshots has been backported to 7.x in #54825.
andreidan added a commit to andreidan/elasticsearch that referenced this pull request Apr 7, 2020
This fixes the version checks to use 7.8.0 instead of 8.0.0 since the
changes in elastic#54825 were backported to the 7.x branch.
andreidan added a commit that referenced this pull request Apr 7, 2020
This fixes the version checks to use 7.8.0 instead of 8.0.0 since the
changes in #54825 were backported to the 7.x branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v7.8.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants