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

Introduce Delegating ActionListener Wrappers #40129

Conversation

original-brownbear
Copy link
Member

  • Dry up use cases of ActionListener that simply pass through the response or exception to another listener

* Dry up use cases of ActionListener that simply pass through the response or exception to another listener
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@jaymode jaymode self-requested a review March 27, 2019 15:45
@original-brownbear
Copy link
Member Author

Jenkins run elasticsearch-ci/bwc
Jenkins run elasticsearch-ci/default-distro

Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

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

Overall, I like this change because I know that it is pretty common to delegate at least one aspect of a listener. The thing I do not like is that we lose some context since this refactoring uses (l, r) and it makes the reader have to look into what l and r actually are. Can you preserve the response variable name like getResponse? Then maybe replace l with delegatedListener or something else?

@original-brownbear
Copy link
Member Author

@jaymode thanks for taking a look fixed the naming as requested in 1f0a804 :)

Copy link
Member

@jaymode jaymode 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 for the quick iteration @original-brownbear

@original-brownbear
Copy link
Member Author

@jaymode np + thanks for the review!

@original-brownbear original-brownbear merged commit f49436d into elastic:master Apr 5, 2019
@original-brownbear original-brownbear deleted the delegating-action-listener branch April 5, 2019 15:46
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 6, 2019
* master: (63 commits)
  Suppress lease background sync failures if stopping (elastic#40902)
  [DOCS] Added settings page for ILM. (elastic#40880)
  [Docs] Remove extraneous text (elastic#40914)
  Move test classes to test root in Painless (elastic#40873)
  Fix date index name processor default date_formats (elastic#40915)
  Source additional files correctly in elasticsearch-cli (elastic#40890)
  Allow AVX-512 on JDK 11+ (elastic#40828)
  [Docs] Change example to show col headers (elastic#40822)
  Update apache httpclient to version 4.5.8 (elastic#40875)
  Update monitoring-kibana.json (elastic#40899)
  Introduce Delegating ActionListener Wrappers (elastic#40129)
  Deprecate old transport settings (elastic#40821)
  Add Kibana application privileges for monitoring and ml reserved roles (elastic#40651)
  Use Writeable for TransportReplAction derivatives (elastic#40894)
  Add test for HTTP and Transport TLS on basic license (elastic#40714)
  Remove unneded cluster config from test (elastic#40856)
  Make Fuzziness reject illegal values earlier (elastic#33511)
  Remove test-only customisation from TransReplAct (elastic#40863)
  Fix dense/sparse vector limit documentation (elastic#40852)
  Make -try xlint warning disabled by default. (elastic#40833)
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 6, 2019
* master: (77 commits)
  Suppress lease background sync failures if stopping (elastic#40902)
  [DOCS] Added settings page for ILM. (elastic#40880)
  [Docs] Remove extraneous text (elastic#40914)
  Move test classes to test root in Painless (elastic#40873)
  Fix date index name processor default date_formats (elastic#40915)
  Source additional files correctly in elasticsearch-cli (elastic#40890)
  Allow AVX-512 on JDK 11+ (elastic#40828)
  [Docs] Change example to show col headers (elastic#40822)
  Update apache httpclient to version 4.5.8 (elastic#40875)
  Update monitoring-kibana.json (elastic#40899)
  Introduce Delegating ActionListener Wrappers (elastic#40129)
  Deprecate old transport settings (elastic#40821)
  Add Kibana application privileges for monitoring and ml reserved roles (elastic#40651)
  Use Writeable for TransportReplAction derivatives (elastic#40894)
  Add test for HTTP and Transport TLS on basic license (elastic#40714)
  Remove unneded cluster config from test (elastic#40856)
  Make Fuzziness reject illegal values earlier (elastic#33511)
  Remove test-only customisation from TransReplAct (elastic#40863)
  Fix dense/sparse vector limit documentation (elastic#40852)
  Make -try xlint warning disabled by default. (elastic#40833)
  ...
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Apr 25, 2019
* Introduce Delegating ActionListener Wrappers
* Dry up use cases of ActionListener that simply pass through the response or exception to another listener
original-brownbear added a commit that referenced this pull request Apr 25, 2019
* Introduce Delegating ActionListener Wrappers
* Dry up use cases of ActionListener that simply pass through the response or exception to another listener
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
* Introduce Delegating ActionListener Wrappers
* Dry up use cases of ActionListener that simply pass through the response or exception to another listener
@jakelandis jakelandis removed the v8.0.0 label Jul 26, 2021
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.

4 participants