removing background state update of Request object by RequestConverte… #40156

kutty-kumar · 2019-03-18T12:43:14Z

Changes required for removing background state update of request object by RequestConverters.Params object
#39666

javanna · 2019-03-18T14:01:02Z

jenkins test this please

elasticmachine · 2019-03-18T14:01:23Z

Pinging @elastic/es-core-features

javanna · 2019-03-18T14:03:10Z

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

            builder.addCommaSeparatedPathParts(clearRealmCacheRequest.getRealms().toArray(Strings.EMPTY_ARRAY));
        } else {
            builder.addPathPart("_all");
        }
        final String endpoint = builder.addPathPartAsIs("_clear_cache").build();
        Request request = new Request(HttpPost.METHOD_NAME, endpoint);
-        if (clearRealmCacheRequest.getUsernames().isEmpty() == false) {
-            RequestConverters.Params params = new RequestConverters.Params(request);
+        if (!clearRealmCacheRequest.getUsernames().isEmpty()) {


can you please revert this change and all the similar ones? we tend to prefer the boolean == false over !boolean as the exclamation mark can easily be missed.

can you please revert this?

javanna · 2019-03-18T14:04:17Z

client/rest/src/main/java/org/elasticsearch/client/Request.java

@@ -50,6 +49,14 @@ public Request(String method, String endpoint) {
        this.endpoint = Objects.requireNonNull(endpoint, "endpoint cannot be null");
    }

+    /**
+     * Copy the parameters from {@literal RequestConverters.Params} into {@linkplain Request}.


I think that we should just say "Set the parameters" , who sets them may change in the future

Also, note that RequestConverters are part of the high-level REST client while Request is part of the low-level REST client which is used independently.

javanna · 2019-03-18T14:05:52Z

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

+        private final Map<String, String> requestParamsMap;
+
+        Params() {
+            this.requestParamsMap = new HashMap<>();


nit: you could initialize the map above and remove the default constructor.

javanna · 2019-03-18T14:10:32Z

client/rest/src/main/java/org/elasticsearch/client/Request.java

@@ -50,6 +49,14 @@ public Request(String method, String endpoint) {
        this.endpoint = Objects.requireNonNull(endpoint, "endpoint cannot be null");
    }

+    /**
+     * Copy the parameters from {@literal RequestConverters.Params} into {@linkplain Request}.
+     * @param requestConverterParams the request converter parameter map


can we remove the mention of request converters here?

javanna · 2019-03-18T14:10:48Z

client/rest/src/main/java/org/elasticsearch/client/Request.java

+     * Copy the parameters from {@literal RequestConverters.Params} into {@linkplain Request}.
+     * @param requestConverterParams the request converter parameter map
+     */
+    public void setParameters(Map<String, String> requestConverterParams){


and rename the argument to not refer to request converters?

javanna · 2019-03-18T14:14:50Z

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

        }

        Params putParam(String name, String value) {
            if (Strings.hasLength(value)) {
-                request.addParameter(name, value);
+                requestParamsMap.put(name, value);


with this change we lose two checks that we were previously performing by calling Request#addParameter:

check that the key is not null

check that the param has not already been set (it would get overridden by default, but we prefer to throw error)

I think that we would want to keep these two checks.

@javanna what error should i throw? i think that check would move to the copy method of Request class

Suggested change

requestParamsMap.put(name, value);

public void setParameters(Map<String, String> paramSource){

paramSource.keySet().forEach(key -> {

if(this.parameters.get(key) != null){

throw new Error(String.format("%s is already set", key));

}

});

this.parameters.putAll(paramSource);

}

does this look good?

can you throw the same error that we throw in the Request class please? IllegalArgumentException?

javanna · 2019-03-18T14:16:05Z

thanks @kutty-kumar for opening this PR! I just did a first quick review and left some comments, also I started tests which are running. Let me know if you have any question on addressing the comments.

javanna · 2019-03-18T21:21:06Z

@kutty-kumar it seems like there's a couple of failing tests. It's fine not to run all of our tests before submitting a PR, but in your case, could you run ../../gradlew check from client/rest-high-level directory please? that should help you reproduce the problem. Let me know if you need help fixing those failures.

kutty-kumar · 2019-03-18T22:52:45Z

@javanna thanks for the initial review, i will work on resolving them and update the PR.

kutty-kumar · 2019-03-19T11:26:47Z

@javanna i have made the required changes please review them also i am currently fixing the tests, will update here once done. Thanks

javanna

I left some more comments, let me know if you need help with the test failures.

client/rest/src/main/java/org/elasticsearch/client/Request.java

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

javanna · 2019-03-20T13:19:41Z

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

            builder.addCommaSeparatedPathParts(clearRealmCacheRequest.getRealms().toArray(Strings.EMPTY_ARRAY));
        } else {
            builder.addPathPart("_all");
        }
        final String endpoint = builder.addPathPartAsIs("_clear_cache").build();
        Request request = new Request(HttpPost.METHOD_NAME, endpoint);
-        if (clearRealmCacheRequest.getUsernames().isEmpty() == false) {
-            RequestConverters.Params params = new RequestConverters.Params(request);
+        if (!clearRealmCacheRequest.getUsernames().isEmpty()) {


can you please revert this?

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

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

kutty-kumar · 2019-03-21T14:16:11Z

@javanna yes i need some help in fixing the tests, not able to figure out where exactly they are failing, please direct me towards that, i will take care of the changes
Thanks

javanna · 2019-03-21T14:54:30Z

retest this please

kutty-kumar · 2019-03-21T16:00:36Z

jenkins test this please

javanna · 2019-03-21T16:05:32Z

@kutty-kumar the magic jenkins commands work only if you are part of the Elastic organization, for security reasons, I will run the tests again.

javanna · 2019-03-21T16:05:42Z

retest this please

kutty-kumar · 2019-03-21T16:08:47Z

@javanna i have started the gradle check process, its still running, doesn't seem to have any failures. Hence i wanted to test it on jenkins, thanks for the info :)

javanna · 2019-03-25T12:51:56Z

@kutty-kumar there are still two failures:

org.elasticsearch.client.IndicesRequestConvertersTests.testExistsAlias
org.elasticsearch.client.SecurityRequestConvertersTests.testDeleteUser

would you mind having a look at those? You can run client tests only by executing ../../gradlew check from client/rest-high-level folder. Thanks

kutty-kumar · 2019-03-26T10:51:30Z

done @javanna, please rebuild the source.
Thanks

javanna · 2019-03-26T10:55:24Z

retest this please

kutty-kumar · 2019-03-26T11:19:54Z

`➜ rest-high-level git:(refactoring/requestConverterParams) ✗ ../../gradlew check

Configure project :benchmarks
=======================================
Elasticsearch Build Hamster says Hello!
Gradle Version : 5.2.1
OS Info : Mac OS X 10.14.3 (x86_64)
JDK Version : 11 (Oracle Corporation 11.0.2 [Java HotSpot(TM) 64-Bit Server VM 11.0.2+9-LTS])
JAVA_HOME : /Library/Java/JavaVirtualMachines/jdk-11.0.2.jdk/Contents/Home
Random Testing Seed : 5FC48F357ECA8590
=======================================

Task :client:rest-high-level:unitTest
==> Test Info: seed=5FC48F357ECA8590; jvms=4; suites=275

[ant:junit4] JVM J0: stderr was not empty, see: /Users/halodoc/Projects/elasticsearch1/client/rest-high-level/build/testrun/unitTest/temp/junit4-J0-20190326_162010_90016372490423214617704.syserr
[ant:junit4] JVM J2: stderr was not empty, see: /Users/halodoc/Projects/elasticsearch1/client/rest-high-level/build/testrun/unitTest/temp/junit4-J2-20190326_162010_90018010291334947152229.syserr
[ant:junit4] JVM J3: stderr was not empty, see: /Users/halodoc/Projects/elasticsearch1/client/rest-high-level/build/testrun/unitTest/temp/junit4-J3-20190326_162010_9011526902967533266904.syserr

Task :client:rest-high-level:unitTest
==> Test Summary: 275 suites, 870 tests

Task :client:rest-high-level:integTestRunner
==> Test Info: seed=5FC48F357ECA8590; jvm=1; suites=45

HEARTBEAT J0 PID(12684@Kumards-MacBook-Pro.local): 2019-03-26T16:27:33, stalled for 10.7s at: SecurityDocumentationIT.testInvalidateToken

Task :client:rest-high-level:integTestRunner
Slow Tests Summary:
102.99s | org.elasticsearch.client.SearchIT
93.71s | org.elasticsearch.client.MachineLearningIT
89.45s | org.elasticsearch.client.documentation.MlClientDocumentationIT
50.11s | org.elasticsearch.client.documentation.SecurityDocumentationIT
45.63s | org.elasticsearch.client.IndicesClientIT

==> Test Summary: 45 suites, 490 tests

Deprecated Gradle features were used in this build, making it incompatible with Gradle 6.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/5.2.1/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 13m 16s
273 actionable tasks: 56 executed, 217 up-to-date`
Did that @javanna

javanna · 2019-03-26T11:23:28Z

can you please merge master in @kutty-kumar ?

kutty-kumar · 2019-03-26T11:29:39Z

@javanna its already up to date with master, please let me know what exactly i should do.
Thanks

javanna · 2019-03-26T11:47:41Z

retest this please

javanna · 2019-03-26T13:17:27Z

@kutty-kumar it does not look up-to-date to me, could you pull latest master (doing git checkout master, then git pull origin master) and then merge it in your branch (by doing git checkout refactoring/requestConverterParams, then git merge master) and then push your updated branch to your remote?

…rs.Params object

kutty-kumar · 2019-03-26T13:35:03Z

@javanna can you please check now, my fork was behind the actual repository, hence the branch was behind, i have taken the changes from the actual repository and force pushed.

Thanks

javanna · 2019-03-26T13:48:52Z

retest this please

javanna · 2019-03-26T14:36:23Z

@kutty-kumar some code needs updating now, as there are more users of the internal API that you have modified in your change. It should be straight-forward to address.

javanna · 2019-04-08T14:41:59Z

hi @kutty-kumar have you had a chance to look into the needed changes? Do you need help with this?

kutty-kumar · 2019-04-08T14:46:08Z

Hi @javanna i thought that i had to refactor a lot, hence i wanted your suggestions on doing this the right way. Please add in some thoughts which i could use to refine the code.

javanna · 2019-04-08T14:56:15Z

@kutty-kumar as far as I can see, while you PR was getting reviewed, new API were added to the high-level REST client which use the mechanism that you were refactoring. You need to make the same changes to those newly added API.

jasontedor · 2019-05-16T00:03:44Z

@kutty-kumar Are you interested in bringing this PR to completion?

kutty-kumar · 2019-05-17T04:38:59Z

@jasontedor yes i will complete the PR by this weekend, sorry for the delay!

javanna · 2019-08-06T10:54:58Z

This has stalled as no work has been done on it for a couple of months. Feel free to reopen or open a new PR when/if you get the time to work on it again.

javanna added the :Core/Features/Java High Level REST Client label Mar 18, 2019

javanna added >enhancement v8.0.0 v7.2.0 labels Mar 18, 2019

javanna reviewed Mar 18, 2019

View reviewed changes

javanna self-assigned this Mar 18, 2019

javanna requested changes Mar 20, 2019

View reviewed changes

removing background state update of Request object by RequestConverte…

41dedb4

…rs.Params object

jakelandis added v7.3.0 and removed v7.2.0 labels Jun 17, 2019

jpountz added v7.4.0 and removed v7.3.0 labels Jul 3, 2019

javanna closed this Aug 6, 2019

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

-                requestParamsMap.put(name, value);
+  public void setParameters(Map<String, String> paramSource){
+        paramSource.keySet().forEach(key -> {
+            if(this.parameters.get(key) != null){
+                throw new Error(String.format("%s is already set", key));
+            }
+        });
+        this.parameters.putAll(paramSource);
+    }

removing background state update of Request object by RequestConverte… #40156

removing background state update of Request object by RequestConverte… #40156

Conversation

kutty-kumar commented Mar 18, 2019

javanna commented Mar 18, 2019

elasticmachine commented Mar 18, 2019

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

kutty-kumar Mar 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Mar 18, 2019

javanna commented Mar 18, 2019

kutty-kumar commented Mar 18, 2019

kutty-kumar commented Mar 19, 2019

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kutty-kumar commented Mar 21, 2019

javanna commented Mar 21, 2019

kutty-kumar commented Mar 21, 2019

javanna commented Mar 21, 2019

javanna commented Mar 21, 2019

kutty-kumar commented Mar 21, 2019

javanna commented Mar 25, 2019

kutty-kumar commented Mar 26, 2019

javanna commented Mar 26, 2019

kutty-kumar commented Mar 26, 2019

javanna commented Mar 26, 2019

kutty-kumar commented Mar 26, 2019

javanna commented Mar 26, 2019

javanna commented Mar 26, 2019

kutty-kumar commented Mar 26, 2019

javanna commented Mar 26, 2019

javanna commented Mar 26, 2019

javanna commented Apr 8, 2019

kutty-kumar commented Apr 8, 2019

javanna commented Apr 8, 2019

jasontedor commented May 16, 2019

kutty-kumar commented May 17, 2019

javanna commented Aug 6, 2019

kutty-kumar Mar 19, 2019 •

edited

Loading