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

Adjust RestHighLevelClient method modifiers #27238

Merged

Conversation

javanna
Copy link
Member

@javanna javanna commented Nov 2, 2017

RestHighLevelClient can be subclassed to add support for additional methods, but its public and protected method should be final.

RestHighLevelClient can be subclassed to add support for additional methods, but its public and protected method should be final. Also some methods were protected but they can be just package private for testing purposes, they don't need to be accessed directly by subclasses.
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM.

@javanna
Copy link
Member Author

javanna commented Nov 2, 2017

thanks @jasontedor !

@javanna javanna merged commit 5d7d01b into elastic:master Nov 6, 2017
javanna added a commit that referenced this pull request Nov 6, 2017
RestHighLevelClient can be subclassed to add support for additional methods, but its public and protected methods should be final.
martijnvg added a commit that referenced this pull request Nov 6, 2017
* 6.x:
  test: Break apart the multi type aspect of rolling upgrade tests,
  Upgrade to Jackson 2.8.10 (#27230)
  [Docs] Fix minor paragraph indentation error for multiple Indices params (#25535)
  Fix inconsistencies in the rest api specs for `tasks` (#27163)
  Adjust RestHighLevelClient method modifiers (#27238)
  Add more information on `_failed_to_convert_` exception (#27034)
  Remove ElasticsearchQueryCachingPolicy (#27190)
  Backport the size-based index rollver to v6.1.0
  Add size-based condition to the index rollover API (#27160)
  Remove the single argument Environment constructor (#27235)
  Fix RestGetAction name typo
jasontedor added a commit that referenced this pull request Nov 7, 2017
* master: (25 commits)
  Disable bwc tests in preparation of backporting #26931
  TemplateUpgradeService should only run on the master (#27294)
  Die with dignity while merging
  Fix profiling naming issues (#27133)
  Correctly encode warning headers
  Fixed references to Multi Index Syntax (#27283)
  Add an active Elasticsearch WordPress plugin link (#27279)
  Setting url parts as required to reflect the code base (#27263)
  keys in aggs percentiles need to be in quotes. (#26905)
  Align routing param type with search.json (#26958)
  Update to support bulk updates by query (#27172)
  Remove duplicated SnapshotStatus (#27276)
  add split index reference in indices.asciidoc
  Add ability to split shards (#26931)
  [Docs] Fix minor paragraph indentation error for multiple Indices params (#25535)
  Upgrade to Jackson 2.8.10 (#27230)
  Fix inconsistencies in the rest api specs for `tasks` (#27163)
  Adjust RestHighLevelClient method modifiers (#27238)
  Remove unused parameters in AnalysisRegistry (#27232)
  Add more information on `_failed_to_convert_` exception (#27034)
  ...
jasontedor added a commit to martijnvg/elasticsearch that referenced this pull request Nov 7, 2017
* ccr: (127 commits)
  Disable bwc tests in preparation of backporting elastic#26931
  TemplateUpgradeService should only run on the master (elastic#27294)
  Die with dignity while merging
  Fix profiling naming issues (elastic#27133)
  Correctly encode warning headers
  Fixed references to Multi Index Syntax (elastic#27283)
  Add an active Elasticsearch WordPress plugin link (elastic#27279)
  Setting url parts as required to reflect the code base (elastic#27263)
  keys in aggs percentiles need to be in quotes. (elastic#26905)
  Align routing param type with search.json (elastic#26958)
  Update to support bulk updates by query (elastic#27172)
  Remove duplicated SnapshotStatus (elastic#27276)
  add split index reference in indices.asciidoc
  Add ability to split shards (elastic#26931)
  [Docs] Fix minor paragraph indentation error for multiple Indices params (elastic#25535)
  Upgrade to Jackson 2.8.10 (elastic#27230)
  Fix inconsistencies in the rest api specs for `tasks` (elastic#27163)
  Adjust RestHighLevelClient method modifiers (elastic#27238)
  Remove unused parameters in AnalysisRegistry (elastic#27232)
  Add more information on `_failed_to_convert_` exception (elastic#27034)
  ...
@mveitas
Copy link

mveitas commented Jun 2, 2018

@javanna with these changes to make methods final, it makes it hard to write a wrapper class around the RestHighLevelClient to be able to apply some sort of decorator. What I was hoping to do is create a wrapper around the class to be able to instrument the client and record metrics around the different types of operations, the timing, etc.

Would you be opposed to some sort of interface that the RestHighLevelClient implements? Having an interface would guarantee that any changes to public methods (additions, removals, etc) are accounted for when using some sort of decorator pattern.

@javanna
Copy link
Member Author

javanna commented Jun 4, 2018

hi @mveitas good point, would you mind opening a new issue so we can mark it discuss and discuss it with the rest of the team? Thank you!

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.

5 participants