Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REST high-level client: add force merge API #28896

Merged
merged 7 commits into from
Mar 22, 2018

Conversation

PnPie
Copy link
Contributor

@PnPie PnPie commented Mar 4, 2018

Force Merge xD.
Relates to #27205

@elasticmachine
Copy link
Collaborator

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?

1 similar comment
@elasticmachine
Copy link
Collaborator

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?

@s1monw
Copy link
Contributor

s1monw commented Mar 4, 2018

@elasticmachine ok to test

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @PnPie I left two tiny comments, LGTM otherwise. I will merge once you addressed those.

assertThat(request.getEndpoint(), equalTo(endpoint.toString()));
assertThat(request.getParameters(), equalTo(expectedParams));
assertThat(request.getEntity(), nullValue());
assertThat(request.getMethod(), equalTo(HttpPost.METHOD_NAME));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a test for the case when indices are not set or set to null? I found also in the clear cache PR that things break :) (also with refresh and flush)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @javanna, when indices are not set or set to null, we need to do operations on all the indices ? or throw an exception ? According to our docs it should be the former ? But I see currently if we didn't set indices (null), we will get an exception when we build the endpoint here, shall we need to modify it ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should use the endpoint that requires no indices (e.g. /_forcemerge), the server will expand that to all indices.

* This class works well for object that do have a constructor argument or that can be built using information available from earlier in the
* XContent. For objects that have constructors with required arguments that are specified on the same level as other fields see
* This class works well for object that doesn't have a constructor argument or that can be built using information available from earlier
* in the XContent. For objects that have constructors with required arguments that are specified on the same level as other fields see
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you leave this change out please? I don't exactly understand these docs, so maybe we can try and fix them separately and give such change more visibility.

Copy link
Contributor Author

@PnPie PnPie Mar 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll remove it here. I just find this when I read the docs of ObjectParser/ConstructingObjectParser :) because I feel like it seems a typo ? To say ConstructingObjectParser is to construct object which has a constructor with arguments, and ObjectParser is rather for object has a constructor without arguments, so we need to set its fields by declaring other methods (ex. setter) ?

@PnPie
Copy link
Contributor Author

PnPie commented Mar 6, 2018

@javanna I updated the 2 PRs.

}
{
ForceMergeRequest forceMergeRequestAll = new ForceMergeRequest();
Map<String, String> expectedParams = new HashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case seems the same as the one above L586-L612 when the randomIndicesNames(0, 5) returned an empty array ( in BroadcastRequest.java : protected String[] indices = Strings.EMPTY_ARRAY ).

@javanna
Copy link
Member

javanna commented Mar 20, 2018

thanks again @PnPie, and sorry about the radio silence. I just merged master in as there were conflicts, I will merge as soon as I get a green build.

@javanna
Copy link
Member

javanna commented Mar 22, 2018

retest this please

@javanna
Copy link
Member

javanna commented Mar 22, 2018

retest this please

@javanna javanna merged commit 24c8d8f into elastic:master Mar 22, 2018
javanna pushed a commit that referenced this pull request Mar 22, 2018
martijnvg added a commit that referenced this pull request Mar 26, 2018
* es/master: (27 commits)
  [Docs] Add rank_eval size parameter k (#29218)
  [DOCS] Remove ignore_z_value parameter link
  Docs: Update docs/index_.asciidoc (#29172)
  Docs: Link C++ client lib elasticlient (#28949)
  [DOCS] Unregister repository instead of deleting it (#29206)
  Docs: HighLevelRestClient#multiSearch (#29144)
  Add Z value support to geo_shape
  Remove type casts in logging in server component (#28807)
  Change BroadcastResponse from ToXContentFragment to ToXContentObject (#28878)
  REST : Split `RestUpgradeAction` into two actions (#29124)
  Add error file docs to important settings
  Add note to low-level client docs for DNS caching (#29213)
  Harden periodically check to avoid endless flush loop (#29125)
  Remove deprecated options for query_string (#29203)
  REST high-level client: add force merge API (#28896)
  Remove license information from README.textile (#29198)
  Decouple more classes from XContentBuilder and make builder strict (#29197)
  [Docs] Fix missing closing block in cluster/misc.asciidoc
  RankEvalRequest should implement IndicesRequest (#29188)
  Use EnumMap in ClusterBlocks (#29112)
  ...
martijnvg added a commit that referenced this pull request Mar 26, 2018
* es/6.x: (29 commits)
  [Docs] Add rank_eval size parameter k (#29218)
  Docs: Update docs/index_.asciidoc (#29172)
  Docs: Link C++ client lib elasticlient (#28949)
  Docs: HighLevelRestClient#multiSearch (#29144)
  [DOCS] Remove ignore_z_value parameter link
  Add Z value support to geo_shape
  Change BroadcastResponse from ToXContentFragment to ToXContentObject (#28878)
  REST : Split `RestUpgradeAction` into two actions (#29124)
  [DOCS] Unregister repository instead of deleting it (#29206)
  Remove type casts in logging in server component (#28807)
  Add error file docs to important settings
  Add note to low-level client docs for DNS caching (#29213)
  testShrinkAfterUpgrade should only set mapping.single_type if bwc version > 5.5.0
  Harden periodically check to avoid endless flush loop (#29125)
  REST high-level client: add force merge API (#28896)
  Remove license information from README.textile (#29198)
  Decouple more classes from XContentBuilder and make builder strict (#29197)
  Propagate mapping.single_type setting on shrinked index (#29202)
  [Docs] Fix missing closing block in cluster/misc.asciidoc
  RankEvalRequest should implement IndicesRequest (#29188)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants