Get snapshots support for multiple repositories #42090
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
|
|
||
| package org.elasticsearch.action.admin.cluster.snapshots.get; | ||
|
|
||
| import org.elasticsearch.Version; | ||
| import org.elasticsearch.action.ActionRequestValidationException; | ||
| import org.elasticsearch.action.support.master.MasterNodeRequest; | ||
| import org.elasticsearch.common.Strings; | ||
|
|
@@ -37,8 +38,9 @@ public class GetSnapshotsRequest extends MasterNodeRequest<GetSnapshotsRequest> | |
| public static final String ALL_SNAPSHOTS = "_all"; | ||
| public static final String CURRENT_SNAPSHOT = "_current"; | ||
| public static final boolean DEFAULT_VERBOSE_MODE = true; | ||
| public static final Version MULTIPLE_REPOSITORIES_SUPPORT_ADDED = Version.V_8_0_0; | ||
|
|
||
| private String[] repositories; | ||
|
|
||
| private String[] snapshots = Strings.EMPTY_ARRAY; | ||
|
|
||
|
|
@@ -50,28 +52,32 @@ public GetSnapshotsRequest() { | |
| } | ||
|
|
||
| /** | ||
| * Constructs a new get snapshots request with given repository names and list of snapshots | ||
| * | ||
| * @param repositories repository names | ||
| * @param snapshots list of snapshots | ||
| */ | ||
| public GetSnapshotsRequest(String[] repositories, String[] snapshots) { | ||
| this.repositories = repositories; | ||
| this.snapshots = snapshots; | ||
| } | ||
|
|
||
| /** | ||
| * Constructs a new get snapshots request with given repository names | ||
| * | ||
| * @param repositories repository names | ||
| */ | ||
| public GetSnapshotsRequest(String... repositories) { | ||
| this.repositories = repositories; | ||
| } | ||
|
|
||
| public GetSnapshotsRequest(StreamInput in) throws IOException { | ||
| super(in); | ||
| if (in.getVersion().onOrAfter(MULTIPLE_REPOSITORIES_SUPPORT_ADDED)) { | ||
| repositories = in.readStringArray(); | ||
| } else { | ||
| repositories = new String[]{in.readString()}; | ||
| } | ||
| snapshots = in.readStringArray(); | ||
| ignoreUnavailable = in.readBoolean(); | ||
| verbose = in.readBoolean(); | ||
|
|
@@ -80,7 +86,11 @@ public GetSnapshotsRequest(StreamInput in) throws IOException { | |
| @Override | ||
| public void writeTo(StreamOutput out) throws IOException { | ||
| super.writeTo(out); | ||
| if (out.getVersion().onOrAfter(MULTIPLE_REPOSITORIES_SUPPORT_ADDED)) { | ||
| out.writeStringArray(repositories); | ||
| } else { | ||
| out.writeString(repositories[0]); | ||
|
There was a problem hiding this comment. This could lead to some strange behaviour in a mixed cluster couldn't it? If I fire a request for multiple snapshots at an older Shouldn't we assert There was a problem hiding this comment. I think, There was a problem hiding this comment. However, I'm not sure that writeTo/readFrom logic is executed if local transport is used. There was a problem hiding this comment.
Not sure I understand. We are simply writing a truncated request to 7.x nodes here then we won't get failures but simply send a truncated request and get unexpected results (concretely, if we request multiple repos and hit an 8.0 node we will simply get a response containing the snapshots for the first repository and simply ignore the rest won't we?)? |
||
| } | ||
| out.writeStringArray(snapshots); | ||
| out.writeBoolean(ignoreUnavailable); | ||
| out.writeBoolean(verbose); | ||
|
|
@@ -89,30 +99,30 @@ public void writeTo(StreamOutput out) throws IOException { | |
| @Override | ||
| public ActionRequestValidationException validate() { | ||
| ActionRequestValidationException validationException = null; | ||
| if (repositories == null || repositories.length == 0) { | ||
| validationException = addValidationError("repositories are missing", validationException); | ||
| } | ||
| return validationException; | ||
| } | ||
|
|
||
| /** | ||
| * Sets repository names | ||
| * | ||
| * @param repositories repository names | ||
| * @return this request | ||
| */ | ||
| public GetSnapshotsRequest repositories(String... repositories) { | ||
| this.repositories = repositories; | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Returns repository names | ||
| * | ||
| * @return repository names | ||
| */ | ||
| public String[] repositories() { | ||
| return this.repositories; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -176,4 +186,4 @@ public boolean verbose() { | |
| public void readFrom(StreamInput in) throws IOException { | ||
| throw new UnsupportedOperationException("usage of Streamable is to be replaced by Writeable"); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why add this but not fully convert class (see e.g. AddVotingConfigExclusionsResponse)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand, could you please clarify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally want to move away from the
Streamableinterface and retire itsreadFrom(StreamInput)method. We have been doing this step-by-step throughout the codebase. The replacement is to define a constructor on the request/response object which takesStreamInputas parameter. This also allows us to make many fields in these (Request/Reponse) classesfinal(see e.g.GetCcrRestoreFileChunkRequest). The PR here does half of the work in this regard. It adds a constructor that just delegates to thereadFrommethod, i.e. adding more code without the benefit of decluttering the existing one.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ywelsch thanks for the clarification, 682d583 and e2f2e9a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ywelsch sorry, I made it wrong first time and fixed here e441032. I have also made similar changes to
GetSnapshotsResponseas a part of this commit.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍