HLRC: Create server agnostic request and response #32912

hub-cap · 2018-08-16T15:22:37Z

The HLRC has historically reused the same Request and Response classes
that the server module uses. This commit deprecates the use of any
server module Request and Response classes, and adds a small bit of
validation logic that differs from server slightly, in that it does not
assume a check for a null ValidationException class is not enough to
determine if validation failed.

The HLRC has historically reused the same Request and Response classes that the server module uses. This commit deprecates the use of any server module Request and Response classes, and adds a small bit of validation logic that differs from server slightly, in that it does not assume a check for a null ValidationException class is not enough to determine if validation failed.

javanna · 2018-08-16T15:36:04Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java

+     * layer has been added to the ReST client, and requests should extend
+     * {@link org.elasticsearch.protocol.xpack.common.ValidationException} instead of `ActionRequest`.
+     */
+    @Deprecated


aren't we deprecating the way that all our current API are implemented?

sadly, yes, if we want to use new server agnostic classes, we have to stop using the old ActionRequest classes.

I agree with the direction, reusing classes is what got us far relatively quickly in the beginning, but as that becomes harder and harder, it makes little sense to continue doing so especially given that it gets us only to an intermediate result that still depends on server. I think as long as we don't deprecate methods that are already out, and we only deprecate what they use internally, this is ok. Do we also plan on rewriting the currently supported API with this approach sometimes soon?

javanna · 2018-08-16T15:36:28Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java

+    /**
+     * @deprecated If creating a new HLRC ReST API call, consider creating new actions instead of reusing server actions. The Validation
+     * layer has been added to the ReST client, and requests should extend
+     * {@link org.elasticsearch.protocol.xpack.common.ValidationException} instead of `ActionRequest`.


the new recommended way requires using classes that sit under xpack packages?

@hub-cap seems that currently, we have only a protocol project for xpack request/responses, if we want rest client to be independent from server, we need to have protocol project for non-xpack classes or probably we can have both xpack and non-xpack classes under the same protocol project.

good catch, i should be moving these to a non xpack, common place. Ill fix that.

@andrershov this point makes me wonder if we should do away with protocol altogether once we fix the existing x-pack classes, and just put all of these request/responses directly in the HLRC. Thoughts cc @javanna ?

looking at this more, I think it probably makes sense to just move them now to HLRC so we can also put the new Validatable in HLRC too. this way its not only x-pack classes.

I think that anything that is not under xpack packages is fine, I don't mind whether that's a new oss library or within the high-level client directly.

For now Im going to move them into the same jar. If we find a need to put them into a new jar, we can always go that route. Id rather stick to simpler and then do it when we need it. Ive done this in d0ca91c

hub-cap · 2018-08-16T17:46:28Z

@elasticmachine test this please

davidkyle · 2018-08-16T17:50:39Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java

+    /**
+     * @deprecated If creating a new HLRC ReST API call, consider creating new actions instead of reusing server actions. The Validation
+     * layer has been added to the ReST client, and requests should extend
+     * {@link org.elasticsearch.protocol.xpack.common.ValidationException} instead of `ActionRequest`.


Shouldn't this link to ... Validatable not ... ValidationException. Same with the other comments

fixed in d0ca91c

andrershov

I've left one comment about naming, but overall looks good to me

andrershov · 2018-08-17T10:59:03Z

client/rest-high-level/src/main/java/org/elasticsearch/client/Validatable.java

+/**
+ * Defines a validation layer for Requests.
+ */
+public interface Validatable {


I'm not sure that Validatable is a good name for the base class of client requests. Probably we can name it ClientRequest, so that in the future if we decide that we need to add new methods to client request we would not need to rename it.

yea but its not about it being a base request. its just the fact that the HLRC does some validation before sending it. Honestly, this is the only thing that ties it to any sort of class hierarchy. Like the response, which has no class hierarchy.

hub-cap · 2018-08-17T14:19:45Z

In a subsequent PR I will be moving Validat* back in to protocol, but the plan is to move protocol under the hlrc, rather than in x-pack.

hub-cap · 2018-08-20T23:17:22Z

ok, WRT the Valid* I think the plan is to remove protocol altogether and just put everything in hlrc. It will be in a subsequent PR.

jasontedor

I left a question.

jasontedor · 2018-08-20T23:30:10Z

client/rest-high-level/src/main/java/org/elasticsearch/client/Validatable.java

+     * {@link ValidationException} that contains a list of all failed validation.
+     */
+    default ValidationException validate() {
+        return new ValidationException();


I'm not sure about creating a new instance on every invocation of validate. Can we share an empty instance and expect overriding implementations to create an instance if needed?

++, good idea.

pls note I had to remove final in order to facilitate overriding in the ifc and allowing me to not add anything to the list of msgs

jasontedor

I left another comment.

jasontedor · 2018-08-21T00:05:16Z

client/rest-high-level/src/main/java/org/elasticsearch/client/Validatable.java

+public interface Validatable {
+    ValidationException EMPTY_VALIDATION = new ValidationException() {
+        @Override
+        public void addValidationError(String error) { }


That’s dangerous! Maybe throw unsupported?

duh, what a rookie mistake :)

fixed in 8fac65d

jasontedor

I left one more comment. Please address one way or the other before merging, but no need for another round.

jasontedor · 2018-08-21T02:37:53Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java

@@ -949,6 +949,11 @@ public final void fieldCapsAsync(FieldCapabilitiesRequest fieldCapabilitiesReque
            FieldCapabilitiesResponse::fromXContent, listener, emptySet());
    }

+    /**
+     * @deprecated If creating a new HLRC ReST API call, consider creating new actions instead of reusing server actions. The Validation
+     * layer has been added to the ReST client, and requests should extend {@link Validatable} instead of `ActionRequest`.


Why backticks in the Javadoc instead of using {@link} or {@code}? I would tend to the user former here, it will resolve as long as ActionRequest is on the class `path and then when we reach that glorious day when it isn't, our Javadocs will fail to build and we will know it's time to remove this Javadoc!

rgr, will fix and then merge, ty for the review.

jasontedor · 2018-08-21T02:38:04Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java

+
+    /**
+     * @deprecated If creating a new HLRC ReST API call, consider creating new actions instead of reusing server actions. The Validation
+     * layer has been added to the ReST client, and requests should extend {@link Validatable} instead of `ActionRequest`.


See my comment about the backticks.

The HLRC has historically reused the same Request and Response classes that the server module uses. This commit deprecates the use of any server module Request and Response classes, and adds a small bit of validation logic that differs from server slightly, in that it does not assume a check for a null ValidationException class is not enough to determine if validation failed.

* 6.x: Introduce mapping version to index metadata (#33147) Move non duplicated actions back into xpack core (#32952) HLRC: Create server agnostic request and response (#32912) Build: forked compiler max memory matches jvmArgs (#33138) * Added breaking change section for GROUP BY behavior: now it considers null or empty values as a separate group/bucket. Previously, they were ignored. * This is part of backporting of #32832 SQL: Enable aggregations to create a separate bucket for missing values (#32832) [TEST] version guard for reload rest-api-spec Fix grammar in contributing docs APM server monitoring (#32515) Support only string `format` in date, root object & date range (#28117) [Rollup] Move toBuilders() methods out of rollup config objects (#32585) Accept Gradle build scan agreement (#30645) Fix forbiddenapis on java 11 (#33116) Run forbidden api checks with runtimeJavaVersion (#32947) Apply publishing to genreate pom (#33094) Fix a mappings update test (#33146) Reload Secure Settings REST specs & docs (#32990) Refactor CachingUsernamePassword realm (#32646)

hub-cap added >enhancement v7.0.0 :Core/Java High Level REST Client v6.5.0 labels Aug 16, 2018

hub-cap requested review from andrershov and jasontedor August 16, 2018 15:22

javanna reviewed Aug 16, 2018

View reviewed changes

davidkyle reviewed Aug 16, 2018

View reviewed changes

hub-cap added 2 commits August 16, 2018 21:23

Move into hlrc and fix comment

d0ca91c

Merge remote-tracking branch 'upstream/master' into hlrc_new_req

c23fdb5

andrershov reviewed Aug 17, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into hlrc_new_req

c44fb3f

hub-cap added 3 commits August 17, 2018 11:57

Merge remote-tracking branch 'upstream/master' into hlrc_new_req

ea52a83

Merge remote-tracking branch 'upstream/master' into hlrc_new_req

110854d

Merge remote-tracking branch 'upstream/master' into hlrc_new_req

2582d02

jasontedor requested changes Aug 20, 2018

View reviewed changes

Empty validation exception

89cce56

pls note I had to remove final in order to facilitate overriding in the ifc and allowing me to not add anything to the list of msgs

jasontedor requested changes Aug 21, 2018

View reviewed changes

throw if method is called

8fac65d

jasontedor approved these changes Aug 21, 2018

View reviewed changes

hub-cap added 2 commits August 21, 2018 09:16

Fix javadoc link

33c28bb

Merge remote-tracking branch 'upstream/master' into hlrc_new_req

0a26cdf

hub-cap merged commit e991208 into elastic:master Aug 22, 2018

hub-cap added the backport pending label Aug 22, 2018

hub-cap removed the backport pending label Aug 27, 2018

jaymode mentioned this pull request Aug 28, 2018

HLRC: add client side RefreshPolicy #33209

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HLRC: Create server agnostic request and response #32912

HLRC: Create server agnostic request and response #32912

hub-cap commented Aug 16, 2018

javanna Aug 16, 2018

hub-cap Aug 16, 2018

javanna Aug 16, 2018

javanna Aug 16, 2018

andrershov Aug 16, 2018

hub-cap Aug 16, 2018

hub-cap Aug 16, 2018

hub-cap Aug 16, 2018

javanna Aug 16, 2018

hub-cap Aug 17, 2018

hub-cap commented Aug 16, 2018

davidkyle Aug 16, 2018

hub-cap Aug 16, 2018

hub-cap Aug 17, 2018

andrershov left a comment

andrershov Aug 17, 2018

hub-cap Aug 17, 2018

hub-cap commented Aug 17, 2018

hub-cap commented Aug 20, 2018

jasontedor left a comment

jasontedor Aug 20, 2018

hub-cap Aug 20, 2018

hub-cap Aug 20, 2018

jasontedor left a comment

jasontedor Aug 21, 2018

hub-cap Aug 21, 2018

jasontedor left a comment

jasontedor Aug 21, 2018

hub-cap Aug 21, 2018

jasontedor Aug 21, 2018

HLRC: Create server agnostic request and response #32912

HLRC: Create server agnostic request and response #32912

Conversation

hub-cap commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hub-cap commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrershov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hub-cap commented Aug 17, 2018

hub-cap commented Aug 20, 2018

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment