-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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.
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
Clean Up Snapshot Create Rest API #31779
Changes from 3 commits
ddace8f
1b903ce
c8f89d4
746cd8d
9777fd1
a0f4347
3fa1103
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,9 +42,9 @@ | |
|
||
import static org.elasticsearch.action.ValidateActions.addValidationError; | ||
import static org.elasticsearch.common.Strings.EMPTY_ARRAY; | ||
import static org.elasticsearch.common.settings.Settings.Builder.EMPTY_SETTINGS; | ||
import static org.elasticsearch.common.settings.Settings.readSettingsFromStream; | ||
import static org.elasticsearch.common.settings.Settings.writeSettingsToStream; | ||
import static org.elasticsearch.common.settings.Settings.Builder.EMPTY_SETTINGS; | ||
import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeBooleanValue; | ||
|
||
/** | ||
|
@@ -430,11 +430,6 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws | |
builder.endObject(); | ||
} | ||
builder.field("include_global_state", includeGlobalState); | ||
if (indicesOptions != null) { | ||
indicesOptions.toXContent(builder, params); | ||
} | ||
builder.field("wait_for_completion", waitForCompletion); | ||
builder.field("master_node_timeout", masterNodeTimeout.toString()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch , these should not be printed out, nor parsed back, they are query_string parameters. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Credit to @vladimirdolzhenko for pointing this out. |
||
builder.endObject(); | ||
return builder; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,14 +21,16 @@ | |
|
||
import org.elasticsearch.action.ActionResponse; | ||
import org.elasticsearch.common.Nullable; | ||
import org.elasticsearch.common.ParseField; | ||
import org.elasticsearch.common.io.stream.StreamInput; | ||
import org.elasticsearch.common.io.stream.StreamOutput; | ||
import org.elasticsearch.common.xcontent.ObjectParser; | ||
import org.elasticsearch.common.xcontent.ToXContentObject; | ||
import org.elasticsearch.common.xcontent.XContentBuilder; | ||
import org.elasticsearch.common.xcontent.XContentParser; | ||
import org.elasticsearch.common.xcontent.XContentParser.Token; | ||
import org.elasticsearch.rest.RestStatus; | ||
import org.elasticsearch.snapshots.SnapshotInfo; | ||
import org.elasticsearch.snapshots.SnapshotInfo.SnapshotInfoBuilder; | ||
|
||
import java.io.IOException; | ||
import java.util.Objects; | ||
|
@@ -38,6 +40,14 @@ | |
*/ | ||
public class CreateSnapshotResponse extends ActionResponse implements ToXContentObject { | ||
|
||
private static final ObjectParser<CreateSnapshotResponse, Void> CREATE_SNAPSHOT_RESPONSE_PARSER = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'd prefer a short name - just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
new ObjectParser<>(CreateSnapshotResponse.class.getName(), true, CreateSnapshotResponse::new); | ||
|
||
static { | ||
CREATE_SNAPSHOT_RESPONSE_PARSER.declareObject(CreateSnapshotResponse::setSnapshotInfoFromBuilder, | ||
SnapshotInfo.SNAPSHOT_INFO_PARSER, new ParseField("snapshot")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think it would be better to simplify There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to leave this one for now because there are two parsers in SnapshotInfo. |
||
} | ||
|
||
@Nullable | ||
private SnapshotInfo snapshotInfo; | ||
|
||
|
@@ -48,8 +58,8 @@ public class CreateSnapshotResponse extends ActionResponse implements ToXContent | |
CreateSnapshotResponse() { | ||
} | ||
|
||
void setSnapshotInfo(SnapshotInfo snapshotInfo) { | ||
this.snapshotInfo = snapshotInfo; | ||
private void setSnapshotInfoFromBuilder(SnapshotInfoBuilder snapshotInfoBuilder) { | ||
this.snapshotInfo = snapshotInfoBuilder.build(); | ||
} | ||
|
||
/** | ||
|
@@ -101,38 +111,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws | |
return builder; | ||
} | ||
|
||
public static CreateSnapshotResponse fromXContent(XContentParser parser) throws IOException { | ||
CreateSnapshotResponse createSnapshotResponse = new CreateSnapshotResponse(); | ||
|
||
parser.nextToken(); // move to '{' | ||
|
||
if (parser.currentToken() != Token.START_OBJECT) { | ||
throw new IllegalArgumentException("unexpected token [" + parser.currentToken() + "], expected ['{']"); | ||
} | ||
|
||
parser.nextToken(); // move to 'snapshot' || 'accepted' | ||
|
||
if ("snapshot".equals(parser.currentName())) { | ||
createSnapshotResponse.snapshotInfo = SnapshotInfo.fromXContent(parser); | ||
} else if ("accepted".equals(parser.currentName())) { | ||
parser.nextToken(); // move to 'accepted' field value | ||
|
||
if (parser.booleanValue()) { | ||
// ensure accepted is a boolean value | ||
} | ||
|
||
parser.nextToken(); // move past 'true'/'false' | ||
} else { | ||
throw new IllegalArgumentException("unexpected token [" + parser.currentToken() + "] expected ['snapshot', 'accepted']"); | ||
} | ||
|
||
if (parser.currentToken() != Token.END_OBJECT) { | ||
throw new IllegalArgumentException("unexpected token [" + parser.currentToken() + "], expected ['}']"); | ||
} | ||
|
||
parser.nextToken(); // move past '}' | ||
|
||
return createSnapshotResponse; | ||
public static CreateSnapshotResponse fromXContent(XContentParser parser) { | ||
return CREATE_SNAPSHOT_RESPONSE_PARSER.apply(parser, null); | ||
} | ||
|
||
@Override | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ | |
package org.elasticsearch.rest.action.admin.cluster; | ||
|
||
import org.elasticsearch.action.admin.cluster.snapshots.create.CreateSnapshotRequest; | ||
import org.elasticsearch.action.support.IndicesOptions; | ||
import org.elasticsearch.client.node.NodeClient; | ||
import org.elasticsearch.common.settings.Settings; | ||
import org.elasticsearch.rest.BaseRestHandler; | ||
|
@@ -54,6 +55,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC | |
request.applyContentParser(p -> createSnapshotRequest.source(p.mapOrdered())); | ||
createSnapshotRequest.masterNodeTimeout(request.paramAsTime("master_timeout", createSnapshotRequest.masterNodeTimeout())); | ||
createSnapshotRequest.waitForCompletion(request.paramAsBoolean("wait_for_completion", false)); | ||
createSnapshotRequest.indicesOptions(IndicesOptions.fromRequest(request, createSnapshotRequest.indicesOptions())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am missing why we need to add support for these params as request parameters. Aren't they part of the request body for this API? Are they still supported in the body, meaning do we parse them back when reading the request body? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vladimirdolzhenko you are referring to delete index, but create snapshot is a different API . See its docs: https://www.elastic.co/guide/en/elasticsearch/reference/6.3/modules-snapshots.html#_snapshot . It seems like ignore_unavailable (which is one of the indices options), as well as index, are specified in the request body. That's why I am asking on what we intend to do here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @javanna @vladimirdolzhenko I like the consistency and methods available for switching to indices options the way delete index is doing it; however at the same time, it's probably not worth deprecating the current way right now. My instinct is to revert this to how it was for now, and we can decide if we want to switch them to parameters at a later time. Would that work for you guys? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes that's a good idea. let's discuss API changes separately. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @javanna Will do. Thanks for the review here! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverted. |
||
return channel -> client.admin().cluster().createSnapshot(createSnapshotRequest, new RestToXContentListener<>(channel)); | ||
} | ||
} |
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.
I can't comment line 2065 here (it's 4 lines above)
Boolean waitForCompletion = randomBoolean();
- no any reason for autoboxingThere 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.
I use the toString method from it for convenience.