-
Couldn't load subscription status.
- Fork 86
Adds elastic-transport client support #223
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
Adds elastic-transport client support #223
Conversation
…art using newer ES ruby client.
|
|
||
| def get_transport_client_class | ||
| require "elasticsearch/transport/transport/http/manticore" | ||
| require_relative "elasticsearch/patches/_elasticsearch_transport_http_manticore" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review note:
_elasticsearch_transport_http_manticoresets
set user-agent and accept-encoding headers for the es-transport-client versions between 7.2 and 7.16. It seems to me we can remove this but I have kept so far as there are active 7.x users;_elasticsearch_transport_connections_selectorhas a round-robin load balancer logic for the es-client versions <7.2.0
I have kept them for now but it looks to me we can remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good opportunity to be more explicit about the versions of es ruby this plugin supports, so instead of:
s.add_runtime_dependency 'elasticsearch', '>= 7.17.9'
we could do
s.add_runtime_dependency 'elasticsearch', '>= 7.17.9', '< 9'
as we haven't tested with 9 at all.
| end | ||
| end | ||
|
|
||
| def get_transport_client_class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add a comment here explaining what's happening
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added.
…ments on the logics to support both elasticsearch and elastic transport gems.
Co-authored-by: João Duarte <[email protected]>
Yes, this is good point! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…-transport-client Adds elastic-transport client support. (cherry picked from commit b920701)
Fix the unit test failures caused by #223
Merge pull request #223 from mashhurs/support-elastic-transport-client
Adds elastic-transport client support where opens LS-core a way to start using newer ES ruby client.
FYI: failed Test plugin docs CI is not related to this change.