Allow setting of `copy_settings` in the HLRC by dliappis · Pull Request #39752 · elastic/elasticsearch

dliappis · 2019-03-06T15:38:50Z

#30255 introduced the copy_settings parameter in 6.x.
This commit adds support for setting it via setCopySettings in the
HLRC.

elastic#30255 introduced the `copy_settings` parameter in 6.x. This commit adds support for setting it via `setCopySettings` in the HLRC.

dliappis · 2019-03-06T15:48:38Z

AFAIU (e.g. in

elasticsearch/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestResizeHandler.java

Lines 60 to 72 in 73d474e

    
           if (rawCopySettings == null) { 
        
               copySettings = resizeRequest.getCopySettings(); 
        
           } else { 
        
               if (rawCopySettings.isEmpty()) { 
        
                   copySettings = true; 
        
               } else { 
        
                   copySettings = Booleans.parseBoolean(rawCopySettings); 
        
                   if (copySettings == false) { 
        
                       throw new IllegalArgumentException("parameter [copy_settings] can not be explicitly set to [false]"); 
        
                   } 
        
               } 
        
               deprecationLogger.deprecated("parameter [copy_settings] is deprecated and will be removed in 8.0.0"); 
        
           }

) we don't need to backport this to 7.x/7.0 as we don't emit special deprecation warnings when it is unset.

Will be happy to add PRs if this is incorrect though.

elasticmachine · 2019-03-06T15:48:54Z

Pinging @elastic/es-core-features

dliappis · 2019-03-06T17:11:14Z

@elasticmachine run elasticsearch-ci/default-distro

hub-cap · 2019-03-06T17:46:17Z

@dliappis can you add some docs here as well pls?

jasontedor

The production code looks good. I left a comment about the deprecation Javadoc, and I agree with @hub-cap to update the documentation that he referenced too.

jasontedor · 2019-03-07T01:55:48Z

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

        }

+        /**
+         * @deprecated <code>copy_settings</code> defaults to <code>true</code> and can not be set to false


Note that the default is unset and behaves as false, however false can not be set explicitly. copy_settings is either unset (default) or set to true (future behavior).

Thanks great catch this got forgotten while I was looking at the behavior of other branches.
Addressed in ce68119 and e4b6cb4.

jasontedor · 2019-03-07T01:56:58Z

...nt/rest-high-level/src/test/java/org/elasticsearch/client/IndicesRequestConvertersTests.java

        resizeRequest.setResizeType(resizeType);
        Map<String, String> expectedParams = new HashMap<>();
+
+        if (ESTestCase.randomBoolean()) {


I see this is in the existing style in this class, but it is at odds with the rest of the codebase where we would simply write randomBoolean().

Addressed in ce68119; in a separate PR we can convert everything in this class to randomBoolean() for consistency.

and use of ESTestCase.randomBoolean()

dliappis · 2019-03-07T10:16:31Z

Docs have been added in 5f05978

…-settings

jasontedor

Looking good now, I left some minor feedback.

jasontedor · 2019-03-07T17:31:28Z

...level/src/test/java/org/elasticsearch/client/documentation/IndicesClientDocumentationIT.java

        request.setWaitForActiveShards(ActiveShardCount.DEFAULT); // <2>
        // end::shrink-index-request-waitForActiveShards
+        // tag::shrink-index-request-copySettings
+        request.setCopySettings(Boolean.TRUE); // <1>


true is more natural

Addressed in de8ca18

jasontedor · 2019-03-07T17:31:42Z

docs/java-rest/high-level/indices/shrink_index.asciidoc

+--------------------------------------------------
+include-tagged::{doc-tests-file}[{api}-request-copySettings]
+--------------------------------------------------
+<1> Use `Boolean.TRUE` to copy the settings from the source index to the target


Addressed in de8ca18

jasontedor · 2019-03-07T17:32:33Z

docs/java-rest/high-level/indices/shrink_index.asciidoc

+include-tagged::{doc-tests-file}[{api}-request-copySettings]
+--------------------------------------------------
+<1> Use `Boolean.TRUE` to copy the settings from the source index to the target
+index. This cannot be `Boolean.FALSE`. If this method is not used, default behavior is not to copy.


Maybe false?

Addressed in de8ca18

jasontedor · 2019-03-07T22:01:11Z

...nt/rest-high-level/src/test/java/org/elasticsearch/client/IndicesRequestConvertersTests.java

        Map<String, String> expectedParams = new HashMap<>();
+
+        if (randomBoolean()) {
+            resizeRequest.setCopySettings(Boolean.TRUE);


Suggested change

resizeRequest.setCopySettings(Boolean.TRUE);

resizeRequest.setCopySettings(true);

Fixed in b20c245

jasontedor · 2019-03-07T22:01:58Z

docs/java-rest/high-level/indices/shrink_index.asciidoc

+include-tagged::{doc-tests-file}[{api}-request-copySettings]
+--------------------------------------------------
+<1> Use `true` to copy the settings from the source index to the target
+index. This cannot be `false`. If this method is not used, default behavior is not to copy.


Can you wrap to 80 columns?

Fixed in b20c245

jasontedor

I left two more minors, no need for another review round. LGTM. Fire at will.

dliappis · 2019-03-08T08:15:28Z

I left two more minors, no need for another review round. LGTM. Fire at will.

Thanks a lot for the review effort here. Will merge after CI is green.

Allow setting of copy_settings in the HLRC

27b0e2c

elastic#30255 introduced the `copy_settings` parameter in 6.x. This commit adds support for setting it via `setCopySettings` in the HLRC.

dliappis added the :Core/Features/Java High Level REST Client label Mar 6, 2019

dliappis added v6.7.0 >bug labels Mar 6, 2019

dliappis requested review from hub-cap and jasontedor March 6, 2019 15:51

Fix wrong import

fe987aa

jasontedor reviewed Mar 7, 2019

View reviewed changes

dliappis added 2 commits March 7, 2019 12:07

Add HLRC docs

5f05978

Address PR comments for deprecation warning

ce68119

and use of ESTestCase.randomBoolean()

dliappis added 2 commits March 7, 2019 12:18

Fix typo

e4b6cb4

Merge branch '6.7' of github.com:elastic/elasticsearch into hlrc-copy…

27aaf3e

…-settings

jasontedor reviewed Mar 7, 2019

View reviewed changes

Address PR comments

de8ca18

jasontedor reviewed Mar 7, 2019

View reviewed changes

jasontedor approved these changes Mar 7, 2019

View reviewed changes

Address PR formatting comments

b20c245

dliappis merged commit bc689f7 into elastic:6.7 Mar 8, 2019

dliappis deleted the hlrc-copy-settings branch March 8, 2019 10:03

	resizeRequest.setCopySettings(Boolean.TRUE);
	resizeRequest.setCopySettings(true);

Conversation

dliappis commented Mar 6, 2019

Uh oh!

dliappis commented Mar 6, 2019

Uh oh!

elasticmachine commented Mar 6, 2019

Uh oh!

dliappis commented Mar 6, 2019

Uh oh!

hub-cap commented Mar 6, 2019

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dliappis Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dliappis commented Mar 7, 2019

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

dliappis commented Mar 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dliappis Mar 7, 2019 •

edited

Loading