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.

Sign up for GitHub

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

Jump to bottom

REST : Split RestUpgradeAction into two actions #29124

Merged
merged 4 commits into from
Mar 23, 2018
Merged

REST : Split RestUpgradeAction into two actions #29124

merged 4 commits into from
Mar 23, 2018

Conversation

milan15
Copy link
Contributor

@milan15 milan15 commented Mar 17, 2018 •
edited by javanna
Loading

@olcbean @nik9000 Let me know how this can be brought to attention of right people or next steps.

Closes #29062

@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?

olcbean
olcbean reviewed Mar 17, 2018
View reviewed changes
Copy link
Contributor

@olcbean olcbean left a comment

Choose a reason for hiding this comment

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

@milan15 Thanks for working on this :)
LGTM overall! I left a couple remarks, please address as you feel appropriate.

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestUpgradeStatusAction.java Outdated
* specific language governing permissions and limitations
* under the License.
*/
package org.elasticsearch.rest.action.admin.indices;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit : can you leave an empty line between the license header and the package ? It's not a big deal, but is somewhat of a convention throughout the codebase.

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

@Override
public String getName() {
return "upgrade_action";
Copy link
Contributor

Choose a reason for hiding this comment

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

You seem to have copied the procedure name from elsewhere. Given this is a standalone piece of code now, a new name is in order. Perhaps upgrade_status_action ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@olcbean yes, even i was thinking of the same but was a bit reluctant as was not sure of what complications this would introduce in system elsewhere. will change that. Thanks!

@olcbean
Copy link
Contributor

olcbean commented Mar 17, 2018

@milan15 at this stage, your part is complete :)
Generally after you open a PR, several people will review it and ( maybe after a few iterations ) it will make its way into the codebase.

olcbean
olcbean approved these changes Mar 21, 2018
View reviewed changes
Copy link
Contributor

@olcbean olcbean left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@javanna javanna added the :Core/Infra/REST API REST infrastructure and utilities label Mar 22, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@javanna
Copy link
Member

javanna commented Mar 22, 2018

test this please

@javanna javanna self-assigned this Mar 22, 2018
@javanna javanna changed the title REST : Split RestUpgradeAction. targets to close #29062 REST : Split RestUpgradeAction into two actions Mar 22, 2018
@javanna
Copy link
Member

javanna commented Mar 22, 2018

retest this please

@javanna javanna merged commit 8328b9c into elastic:master Mar 23, 2018
@javanna
Copy link
Member

javanna commented Mar 23, 2018

thanks @milan15 !!

javanna pushed a commit that referenced this pull request Mar 23, 2018
@milan15 @javanna
REST : Split RestUpgradeAction into two actions (#29124)
60dfa0d 
Closes #29062
martijnvg added a commit that referenced this pull request Mar 26, 2018
@martijnvg
Merge remote-tracking branch 'es/master' into ccr
b56fede 
* 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
@martijnvg
Merge remote-tracking branch 'es/6.x' into ccr-6.x
bda5983 
* 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)
  ...
@olcbean
Copy link
Contributor

olcbean commented Mar 27, 2018

@javanna thanks!

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Mar 27, 2018
@jasontedor
Merge branch 'master' into bump-compiler-to-jdk-10
d4dba9a 
* master: (40 commits)
  Do not optimize append-only if seen normal op with higher seqno (elastic#28787)
  [test] packaging: gradle tasks for groovy tests (elastic#29046)
  Prune only gc deletes below local checkpoint (elastic#28790)
  remove testUnassignedShardAndEmptyNodesInRoutingTable
  elastic#28745: remove extra option in the composite rest tests
  Fold EngineDiskUtils into Store, for better lock semantics (elastic#29156)
  Add file permissions checks to precommit task
  Remove execute mode bit from source files
  Optimize the composite aggregation for match_all and range queries (elastic#28745)
  [Docs] Add rank_eval size parameter k (elastic#29218)
  [DOCS] Remove ignore_z_value parameter link
  Docs: Update docs/index_.asciidoc (elastic#29172)
  Docs: Link C++ client lib elasticlient (elastic#28949)
  [DOCS] Unregister repository instead of deleting it (elastic#29206)
  Docs: HighLevelRestClient#multiSearch (elastic#29144)
  Add Z value support to geo_shape
  Remove type casts in logging in server component (elastic#28807)
  Change BroadcastResponse from ToXContentFragment to ToXContentObject (elastic#28878)
  REST : Split `RestUpgradeAction` into two actions (elastic#29124)
  Add error file docs to important settings
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olcbean olcbean olcbean approved these changes

Assignees

@javanna javanna

Labels
:Core/Infra/REST API REST infrastructure and utilities >enhancement v6.3.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@milan15 @elasticmachine @olcbean @javanna @colings86