Skip to content

Allow multiple hosts for elasticsearch connector#12530

Merged
martint merged 1 commit intotrinodb:masterfrom
zhaoyim:i11889
Aug 25, 2022
Merged

Allow multiple hosts for elasticsearch connector#12530
martint merged 1 commit intotrinodb:masterfrom
zhaoyim:i11889

Conversation

@zhaoyim
Copy link
Copy Markdown
Contributor

@zhaoyim zhaoyim commented May 24, 2022

Description

Is this change a fix, improvement, new feature, refactoring, or other?

improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

No

How would you describe this change to a non-technical end user or system administrator?

Allow multiple hosts for elasticsearch connector (elasticsearch.host property)

Related issues, pull requests, and links

Fixes #11889

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
(X) Documentation issue #12528 is filed, and can be handled later.

Release notes

(x) Release notes entries required with the following suggested text:

# Elasticsearch
* Add support for multiple hosts in `elasticsearch.host` configuration property. ({issue}`12530`)

@cla-bot cla-bot bot added the cla-signed label May 24, 2022
@ebyhr ebyhr removed their request for review May 24, 2022 10:03
@ebyhr
Copy link
Copy Markdown
Member

ebyhr commented May 25, 2022

Quote from the issue:

There is property for sniffing elasticsearch.node-refresh-interval but in many cases, sniffing is not the best choice and a hardcoded list of hosts in elasticsearch.host would work best

I suppose this PR still find nodes by schedule.

@zhaoyim
Copy link
Copy Markdown
Contributor Author

zhaoyim commented May 25, 2022

When the first start trino server, it can try all configed elasticsearch.host hosts to makes sure the es connector worked well, and the schedule nodes can get successfully.
For example:
Before:

  1. elasticsearch.host config one node, but the node is not started
  2. start trino, can NOT connect to es, the scheduler also can NOT get nodes
  3. the es query can NOT run, until the config es node started

After:

  1. elasticsearch.host config several nodes, if one node is started
  2. start trino, can connect to es, the scheduler also can get nodes
  3. the es query can run successfully

@zhaoyim
Copy link
Copy Markdown
Contributor Author

zhaoyim commented Jun 10, 2022

@bitsondatadev Could you help review this pr? Thanks!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be a List<String> not a String. The splitting on , will be done by the configuration system. It shouldn't be done by ElasticsearchClient. Otherwise, it's hard to do any validation and error reporting at startup time.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for review! @martint Did the changes, could you help review again? Thanks in advance!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The above comment means changing the argument to List<String> instead of splitting by yourself.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ebyhr Thanks for clarify! Did the changes. Thanks again!

@zhaoyim zhaoyim force-pushed the i11889 branch 2 times, most recently from 327ed99 to 905a091 Compare June 15, 2022 04:40
@zhaoyim zhaoyim requested a review from martint June 15, 2022 06:09
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This test is unnecessary. It doesn't validate anything about the state of the client or that it works -- just that it can be constructed.

Let's remove it.

Copy link
Copy Markdown
Contributor Author

@zhaoyim zhaoyim Aug 23, 2022

Choose a reason for hiding this comment

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

@martint Thanks for your comments! Remove the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Allow multiple hosts for elasticsearch connector (elasticsearch.host property)

3 participants