Make Recovery API support `detailed` params #29076

PnPie · 2018-03-14T22:19:15Z

Properly forward the detailed parameter to show extra details in the recovery stats.

elasticmachine · 2018-03-14T22:19:17Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine · 2018-03-14T22:19:17Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine · 2018-03-23T14:41:11Z

Pinging @elastic/es-distributed

bleskes

Thx for picking this up. I left some comments

bleskes · 2018-03-25T14:30:55Z

server/src/main/java/org/elasticsearch/action/admin/indices/recovery/RecoveryRequest.java

@@ -31,7 +31,7 @@
 */
 public class RecoveryRequest extends BroadcastRequest<RecoveryRequest> {

-    private boolean detailed = false;       // Provides extra details in the response
+    private boolean details = false;       // Provides extra details in the response


can we remove this too?

The details variable in RecoveryRequest ? It's to pass the details flag in Recovery API ? can we remove it ?

bleskes · 2018-03-25T14:33:07Z

server/src/main/java/org/elasticsearch/action/admin/indices/recovery/RecoveryRequest.java

@@ -91,14 +82,14 @@ public void activeOnly(boolean activeOnly) {
    @Override
    public void writeTo(StreamOutput out) throws IOException {
        super.writeTo(out);
-        out.writeBoolean(detailed);
+        out.writeBoolean(details);


when you remove this, watch out for BWC aspects - see https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/action/admin/cluster/storedscripts/GetStoredScriptRequest.java#L59 as an example.

Here I just changed the name of variable (to be the same as flag name), the value is always a boolean, so we don't need to do some bwc changes ?

javanna · 2018-03-27T18:55:39Z

server/src/main/java/org/elasticsearch/action/admin/indices/recovery/RecoveryResponse.java

@@ -39,7 +39,6 @@
 */
 public class RecoveryResponse extends BroadcastResponse implements ToXContentFragment {

-    private boolean detailed = false;


wasn't this already removed in another PR? Maybe you need to merge master in?

Yes, I didn't merge master before.

PnPie

Hello @bleskes @javanna I update it and added some comments.

PnPie · 2018-03-27T20:09:02Z

server/src/main/java/org/elasticsearch/action/admin/indices/recovery/RecoveryRequest.java

@@ -31,7 +31,7 @@
 */
 public class RecoveryRequest extends BroadcastRequest<RecoveryRequest> {

-    private boolean detailed = false;       // Provides extra details in the response
+    private boolean details = false;       // Provides extra details in the response


The details variable in RecoveryRequest ? It's to pass the details flag in Recovery API ? can we remove it ?

PnPie · 2018-03-27T20:16:38Z

server/src/main/java/org/elasticsearch/action/admin/indices/recovery/RecoveryRequest.java

@@ -91,14 +82,14 @@ public void activeOnly(boolean activeOnly) {
    @Override
    public void writeTo(StreamOutput out) throws IOException {
        super.writeTo(out);
-        out.writeBoolean(detailed);
+        out.writeBoolean(details);


Here I just changed the name of variable (to be the same as flag name), the value is always a boolean, so we don't need to do some bwc changes ?

PnPie · 2018-03-27T20:17:14Z

server/src/main/java/org/elasticsearch/action/admin/indices/recovery/RecoveryResponse.java

@@ -39,7 +39,6 @@
 */
 public class RecoveryResponse extends BroadcastResponse implements ToXContentFragment {

-    private boolean detailed = false;


Yes, I didn't merge master before.

PnPie · 2018-04-04T20:18:22Z

Hello @javanna @bleskes, so as we discussed in the issue, I changed it back to detailed ?

pcsanwald · 2019-01-03T21:42:06Z

@javanna @bleskes we discussed this in FixIt Thursday, any thoughts on what is best, here?

ywelsch · 2019-03-04T15:52:56Z

@javanna @bleskes ping

javanna · 2019-03-05T09:57:24Z

hi @PnPie sorry it took ages to get back to you. We have discussed this with the team and we think that we should rather change the code to expose detailed like the docs mention, rather than accepting details. This is more consistent with other API that already accept detailed. Would you be up for updating your PR and merging master in, or opening a new one given all the changes that went in meanwhile? Thanks!

PnPie · 2019-03-07T12:47:56Z

Hi @javanna thanks for your reply and I'm sorry I just get occupied these days and I'll update it during this week-end ;)

javanna · 2019-03-07T12:57:52Z

no problem at all @PnPie we made you wait much longer than that ;)

elasticcla · 2019-03-17T08:12:39Z

Hi @PnPie, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

PnPie · 2019-03-17T08:57:22Z

Hi @javanna I updated it and added a test case for RecoveryState, sorry for the slight delay :)

ywelsch · 2019-03-19T13:06:38Z

@elasticmachine retest this please

ywelsch

Thanks for updating this @PnPie. I've left some comments. We should also update the REST spec (rest-api-spec/api/cat.recovery.json) to include this flag.

ywelsch · 2019-03-19T13:47:49Z

...er/src/main/java/org/elasticsearch/action/admin/indices/recovery/RecoveryRequestBuilder.java

undo this change

ywelsch · 2019-03-19T13:49:35Z

server/src/main/java/org/elasticsearch/indices/recovery/RecoveryState.java

@@ -343,7 +343,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
        static final String REUSED = "reused";
        static final String REUSED_IN_BYTES = "reused_in_bytes";
        static final String PERCENT = "percent";
-        static final String DETAILS = "details";
+        static final String DETAILED = "detailed";


let's leave this field as is. "details" in the output is nicer, and we don't break existing parsers.

ywelsch · 2019-03-19T13:51:12Z

server/src/main/java/org/elasticsearch/indices/recovery/RecoveryState.java

@@ -929,8 +929,8 @@ public synchronized XContentBuilder toXContent(XContentBuilder builder, Params p
            builder.field(Fields.REUSED, reusedFileCount());
            builder.field(Fields.RECOVERED, recoveredFileCount());
            builder.field(Fields.PERCENT, String.format(Locale.ROOT, "%1.1f%%", recoveredFilesPercent()));
-            if (params.paramAsBoolean("details", false)) {
-                builder.startArray(Fields.DETAILS);
+            if (params.paramAsBoolean("detailed", false)) {


this is the only change that we need to do in this class

ywelsch · 2019-03-19T13:54:37Z

server/src/test/java/org/elasticsearch/indices/recovery/RecoveryStateTests.java

+
+public class RecoveryStateTests extends ESTestCase {
+
+    public void testRecoveryStateIndexToXContent() {


I would prefer a REST test to show that the detailed flag is honoured (see rest-api-spec/test/cat.recovery/10_basic.yml). This test here is not needed.

PnPie · 2019-04-06T09:46:46Z

Hi @ywelsch I updated the PR and added a rest layer test.

ywelsch · 2019-06-18T08:55:29Z

@elasticmachine update-branch

ywelsch

@PnPie sorry that I missed the ping here. Thank you for iterating with me on this. I think we will need to make the test conditional on the ES version that it will run against. I've started CI on this, and if necessary, can make the adjustment myself as well.

ywelsch · 2019-06-18T09:00:28Z

rest-api-spec/src/main/resources/rest-api-spec/test/indices.recovery/10_basic.yml

@@ -130,3 +130,25 @@
        index: [v*]

  - match: { $body: {} }
+---
+"Indices recovery test with detailed parameter":


I think we will have to skip this test for versions that don't have this fix.

ywelsch · 2019-06-20T12:19:18Z

@elasticmachine update-branch

ywelsch · 2019-06-20T12:19:53Z

@elasticmachine retest this please

ywelsch · 2019-06-20T13:07:54Z

@elasticmachine retest this please

ywelsch · 2019-06-20T14:23:58Z

@elasticmachine retest this please

ywelsch · 2019-06-20T18:05:35Z

@elasticmachine run elasticsearch-ci/1

Properly forwards the `detailed` parameter to show the recovery stats details. Closes #28910

nik9000 added the :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. label Mar 23, 2018

bleskes suggested changes Mar 25, 2018

View reviewed changes

javanna reviewed Mar 27, 2018

View reviewed changes

PnPie commented Mar 27, 2018

View reviewed changes

colings86 added >enhancement >breaking labels Apr 24, 2018

PnPie changed the title ~~Make Recovery API support only details param~~ Make Recovery API support only detailed param Mar 17, 2019

PnPie changed the title ~~Make Recovery API support only detailed param~~ Make Recovery API support detailed params Mar 17, 2019

PnPie added 2 commits March 17, 2019 16:40

Make recovery API support only detailed params

32d5069

Merge branch 'master' into recovery_param

279afac

PnPie force-pushed the recovery_param branch from 163718d to 279afac Compare March 17, 2019 08:47

ywelsch self-requested a review March 19, 2019 11:58

ywelsch added v8.0.0 v7.2.0 >bug and removed >breaking >enhancement labels Mar 19, 2019

ywelsch suggested changes Mar 19, 2019

View reviewed changes

PnPie added 2 commits April 6, 2019 17:37

update and add rest layer test

a5b72c4

restore changes for Strings.java

e50cc7b

ywelsch self-requested a review June 17, 2019 09:05

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

ywelsch approved these changes Jun 18, 2019

View reviewed changes

ywelsch self-requested a review June 18, 2019 09:05

Merge remote-tracking branch 'elastic/master' into pr/29076

c7eea32

ywelsch added 2 commits June 20, 2019 16:11

Fix docs recovery test

8707e88

Add BWC conditon for newly added test

1ba84cf

ywelsch merged commit e8ea831 into elastic:master Jun 21, 2019

ywelsch pushed a commit that referenced this pull request Jun 21, 2019

Make Recovery API support detailed params (#29076)

c88f2f2

Properly forwards the `detailed` parameter to show the recovery stats details. Closes #28910

ywelsch added a commit that referenced this pull request Jun 21, 2019

Change BWC condition after backporting #29076

1765b38

Mpdreamz mentioned this pull request Aug 7, 2019

[meta] 7.3 Release elastic/elasticsearch-net#4001

Closed

16 tasks

codebrain mentioned this pull request Sep 2, 2019

Map additional Recovery API response properties. elastic/elasticsearch-net#4067

Merged

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


		public class RecoveryStateTests extends ESTestCase {

		public void testRecoveryStateIndexToXContent() {

Make Recovery API support detailed params #29076

Make Recovery API support detailed params #29076

Conversation

PnPie commented Mar 14, 2018 • edited by ywelsch Loading

elasticmachine commented Mar 14, 2018

elasticmachine commented Mar 14, 2018

elasticmachine commented Mar 23, 2018

bleskes 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PnPie 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

PnPie commented Apr 4, 2018

pcsanwald commented Jan 3, 2019

ywelsch commented Mar 4, 2019

javanna commented Mar 5, 2019

PnPie commented Mar 7, 2019

javanna commented Mar 7, 2019

elasticcla commented Mar 17, 2019

PnPie commented Mar 17, 2019

ywelsch commented Mar 19, 2019

ywelsch left a comment • edited Loading

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

PnPie commented Apr 6, 2019

ywelsch commented Jun 18, 2019

ywelsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywelsch commented Jun 20, 2019

ywelsch commented Jun 20, 2019

ywelsch commented Jun 20, 2019

ywelsch commented Jun 20, 2019

ywelsch commented Jun 20, 2019

Make Recovery API support `detailed` params #29076

Make Recovery API support `detailed` params #29076

PnPie commented Mar 14, 2018 •

edited by ywelsch

Loading

ywelsch left a comment •

edited

Loading