Consolidate the last easy parser construction #22095
Conversation
Moves the last of the "easy" parser construction into `RestRequest`, this time with a new method `RestRequest#contentParser`. The rest of the production code that builds `XContentParser` isn't "easy" because it is exposed in the Transport Client API (a Builder) object.
| } else if ("_source".equals(currentFieldName)) { | ||
| fetchSourceContext = FetchSourceContext.parse(parser); | ||
| XContentParser.Token token = parser.nextToken(); | ||
| if (token == null) { |
There was a problem hiding this comment.
Just indentation changed.
| restoreSnapshotRequest.masterNodeTimeout(request.paramAsTime("master_timeout", restoreSnapshotRequest.masterNodeTimeout())); | ||
| restoreSnapshotRequest.waitForCompletion(request.paramAsBoolean("wait_for_completion", false)); | ||
| restoreSnapshotRequest.source(request.content().utf8ToString()); | ||
| if (request.hasContent()) { |
There was a problem hiding this comment.
maybe for usages like this one we can add a helper method on RestRequest? Like this one:
public final void applySource(Consumer<Map<String, Object>> target) throws IOException {
if (hasContent()) {
try (XContentParser parser = contentParser()) {
target.accept(parser.mapOrdered());
}
}
}Then this snippet can be replaced with:
request.applySource(restoreSnapshotRequest::source);and we could have a similar helper that passes a XContentParser to an action request?
There was a problem hiding this comment.
Sure! I'll make that change.
|
@martijnvg I made the change you asked for. |
imotov
left a comment
imotov
left a comment
There was a problem hiding this comment.
Left a couple of comments.
| return this; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
This is user-facing interface and removing this calls might break applications that are using transport client to create repositories. So, I think we need to start with marking this as deprecated before removing it completely. Same in other requests.
There was a problem hiding this comment.
I thought only our builder classes were user facing? I specifically only did this to the classes that didn't pipe these through to the builders.
There was a problem hiding this comment.
I am not totally sure about this one. I think it depends on user preferences at the moment. The transport client can be used with builders or without. All our examples are showing client.prepareBlah(...) version, but the non-builder version client.blah(...) is readily accessible to anyone who is using client and I don't remember us ever mentioning that it's internal and shouldn't be used in apps.
There was a problem hiding this comment.
they are both user facing like Igor said, but not sure we have been keeping bw comp on the java api between minor versions. I think it's ok to make these changes as long as we mark breaking-java and we document what the replacement is. We generally deprecate stuff that has to do with the REST layer. I think.
There was a problem hiding this comment.
I've marked this as breaking-java and I'll leave a note in https://github.com/elastic/elasticsearch/blob/5.x/docs/reference/migration/migrate_5_2.asciidoc when I merge to the 5.x branch. I think that is in line with what we've done with the transport API in the past.
| package org.elasticsearch.rest.action.admin.indices; | ||
|
|
||
| import org.elasticsearch.action.admin.indices.rollover.RolloverRequest; | ||
| import org.elasticsearch.action.admin.indices.shrink.ShrinkRequest; |
There was a problem hiding this comment.
Nitpicking - unused imports were added to this and other IndexActions.
There was a problem hiding this comment.
Ah, sorry about that one.
Moves the last of the "easy" parser construction into `RestRequest`, this time with a new method `RestRequest#contentParser`. The rest of the production code that builds `XContentParser` isn't "easy" because it is exposed in the Transport Client API (a Builder) object.
5 participants
Moves the last of the "easy" parser construction into
RestRequest, this time with a new methodRestRequest#contentParser. The rest of the productioncode that builds
XContentParserisn't "easy" because it isexposed in the Transport Client API (a Builder) object.