Add support for user/password auth - Elasticsearch #15877

v-jizhang · 2021-03-26T17:00:22Z

Cherry-pick of trinodb/trino#4165,
trinodb/trino#2591 and
trinodb/trino#3331

Co-authored-by: Martin Traverso mtraverso@gmail.com

== RELEASE NOTES ==

ElasticSearch Connector Changes
* Add support for Elasticsearch user and password authentication.
   :issue:`15909`

aweisberg

Thanks for working on this.

So I am generally speaking +1 on what I see in terms of the functional changes, but there is some additional backporting and simplification we can do in the tests (only use high level rest client, remove embedded elastic search server) and I would like to block the PR on including those as well.

pom.xml

aweisberg · 2021-04-07T21:43:53Z

presto-elasticsearch/src/test/java/com/facebook/presto/elasticsearch/ElasticsearchLoader.java

Why are we omitting the refresh policy from Trino? trinodb/trino@bd4b3dd#diff-d16680601d6ec03b7147a7dc5eb1178e73a5c2725a40c0f60c920048aea0e217R110

Should not be omitted, added. Thanks

aweisberg · 2021-04-07T21:45:26Z

...-elasticsearch/src/test/java/com/facebook/presto/elasticsearch/ElasticsearchQueryRunner.java

We should remove EmbeddedElasticsearchNode like they did in Trino if we can.

Yes, I'll do it in another PR.

aweisberg · 2021-04-07T23:14:06Z

@zhenxiao adding you for review since you have done a lot of ES work.

aweisberg · 2021-04-07T23:27:29Z

I mixed up the two links I meant trinodb/trino@bbe718d

zhenxiao

looks good. a few minor things

zhenxiao · 2021-04-08T06:32:25Z

presto-elasticsearch/src/test/java/com/facebook/presto/elasticsearch/ElasticsearchServer.java

shall we make /usr/share/elasticsearch/config/ constant?

Sure, thanks.

zhenxiao · 2021-04-08T06:33:24Z

...lasticsearch/src/test/java/com/facebook/presto/elasticsearch/TestPasswordAuthentication.java

how about password as PASSWORD?

Let's keep it the same as Trino

zhenxiao · 2021-04-08T06:34:03Z

...lasticsearch/src/test/java/com/facebook/presto/elasticsearch/TestPasswordAuthentication.java

make elasticsearch:7.8.0 a constant, or 7.8.0 a version constant?

Sure. I'll change it. Thanks

aweisberg · 2021-04-08T14:28:16Z

@zhenxiao how much is consistency worth with Trino? A lot of these choices for constants are just what Trino did.

v-jizhang · 2021-04-08T15:59:50Z

Thanks for working on this.

So I am generally speaking +1 on what I see in terms of the functional changes, but there is some additional backporting and simplification we can do in the tests (only use high level rest client, remove embedded elastic search server) and I would like to block the PR on including those as well.

That should be done in another PR and it's on my list.

aweisberg · 2021-04-09T14:33:17Z

I don't think it should be a separate PR just because it introduces the additional complexity in this PR instead of doing it the way Trino did it? We are mixing together two change sets from Trino and not taking the entirety of both so we end up with this mixed result.

v-jizhang · 2021-04-12T15:25:41Z

I don't think it should be a separate PR just because it introduces the additional complexity in this PR instead of doing it the way Trino did it? We are mixing together two change sets from Trino and not taking the entirety of both so we end up with this mixed result.

I'll port testcontainers changes and get rid of embedded Elasticsearch node and make this PR simpler. Thanks

aweisberg

Looks this is relying on EmbeddedElasticSearchNode. We should get rid of the duplication between that and the older RestClient.

v-jizhang · 2021-05-04T17:48:11Z

Looks this is relying on EmbeddedElasticSearchNode. We should get rid of the duplication between that and the older RestClient.

Yes, I'll rebase and get rid of it after #15937

aweisberg

Can you squash/fixup so it is the three core commits. Add password authentication. Remove embedded elastic search server, and add the comment from Martin? You don't need to change the order just make it roughly analogous to what happened in Trino.

Also add co-authors to the latter two commits.

aweisberg · 2021-05-19T17:20:45Z

presto-elasticsearch/pom.xml

Why do we end up needing JNA? I don't see it as a dependency in Trino.

Presto has that dependency:
Require upper bound dependencies error for net.java.dev.jna:jna:5.2.0 paths to dependency are:
+-com.facebook.presto:presto-elasticsearch:0.254-SNAPSHOT
+-org.testcontainers:testcontainers:1.15.2
+-org.rnorth.visible-assertions:visible-assertions:2.1.2
+-net.java.dev.jna:jna:5.2.0
and
+-com.facebook.presto:presto-elasticsearch:0.254-SNAPSHOT
+-org.testcontainers:testcontainers:1.15.2
+-com.github.docker-java:docker-java-transport-zerodep:3.2.7
+-net.java.dev.jna:jna:5.5.0

aweisberg · 2021-05-19T17:28:52Z

...-elasticsearch/src/test/java/com/facebook/presto/elasticsearch/ElasticsearchQueryRunner.java

Why remove setNodeCount?

Trino removed that and other counts in trinodb/trino#3267. I should do it in another PR. Added it back. Thanks

aweisberg · 2021-05-24T20:40:02Z

OK, well if you can get the commit history cleaned up and get a passing test run I can approve it :-)

Cherry-pick of trinodb/trino#4165 and trinodb/trino#2591 Co-authored-by: Martin Traverso <mtraverso@gmail.com>

v-jizhang · 2021-05-25T21:04:20Z

OK, well if you can get the commit history cleaned up and get a passing test run I can approve it :-)

Done. Please review.

aweisberg

We don't really want to squash all the commits together into a single commit. That erased the history and merged together unrelated changes. We just want to squash the extra modification commits used to be there https://github.com/prestodb/presto/commits/d3013b66e4c904a08fae8faf72cf8128bef2f2d7

Is it possible to restore it without it being a herculean task? Ideally if we cherry-pick three commits we end up with the same three commits on our end.

v-jizhang · 2021-07-19T16:30:58Z

We don't really want to squash all the commits together into a single commit. That erased the history and merged together unrelated changes. We just want to squash the extra modification commits used to be there https://github.com/prestodb/presto/commits/d3013b66e4c904a08fae8faf72cf8128bef2f2d7

Is it possible to restore it without it being a herculean task? Ideally if we cherry-pick three commits we end up with the same three commits on our end.

I didn't find a way to restore the commits. If you like, I can redo it. Thanks

v-jizhang · 2021-07-27T19:54:12Z

Replaced by #16524.

v-jizhang force-pushed the elastic_password branch from 5170a36 to c1f9773 Compare March 26, 2021 20:39

aweisberg self-requested a review April 5, 2021 16:38

v-jizhang mentioned this pull request Apr 6, 2021

Elasticsearch Connector does not support username and password #15909

Open

aweisberg suggested changes Apr 7, 2021

View reviewed changes

v-jizhang force-pushed the elastic_password branch from 7573450 to 1b11c8f Compare April 7, 2021 23:04

v-jizhang requested a review from aweisberg April 7, 2021 23:05

aweisberg requested a review from zhenxiao April 7, 2021 23:13

zhenxiao reviewed Apr 8, 2021

View reviewed changes

aweisberg suggested changes May 3, 2021

View reviewed changes

v-jizhang force-pushed the elastic_password branch from e6c13f9 to e2bc9e4 Compare May 17, 2021 17:47

v-jizhang requested a review from aweisberg May 17, 2021 23:09

aweisberg suggested changes May 19, 2021

View reviewed changes

v-jizhang requested a review from aweisberg May 20, 2021 16:27

Add support for user/password auth - Elasticsearch

31f5ff4

Cherry-pick of trinodb/trino#4165 and trinodb/trino#2591 Co-authored-by: Martin Traverso <mtraverso@gmail.com>

v-jizhang force-pushed the elastic_password branch from d3013b6 to 31f5ff4 Compare May 25, 2021 18:28

aweisberg suggested changes Jun 25, 2021

View reviewed changes

v-jizhang requested a review from aweisberg July 20, 2021 15:11

v-jizhang marked this pull request as draft July 26, 2021 18:22

v-jizhang closed this Jul 27, 2021

v-jizhang mentioned this pull request Nov 30, 2022

Add support for user/password auth - Elasticsearch #16524

Merged

Add support for user/password auth - Elasticsearch #15877

Add support for user/password auth - Elasticsearch #15877

Uh oh!

Conversation

v-jizhang commented Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aweisberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweisberg commented Apr 7, 2021

Uh oh!

aweisberg commented Apr 7, 2021

Uh oh!

zhenxiao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweisberg commented Apr 8, 2021

Uh oh!

v-jizhang commented Apr 8, 2021

Uh oh!

aweisberg commented Apr 9, 2021

Uh oh!

v-jizhang commented Apr 12, 2021

Uh oh!

aweisberg left a comment

Choose a reason for hiding this comment

Uh oh!

v-jizhang commented May 4, 2021

Uh oh!

aweisberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweisberg commented May 24, 2021

Uh oh!

v-jizhang commented May 25, 2021

Uh oh!

aweisberg left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v-jizhang commented Jul 19, 2021

Uh oh!

v-jizhang commented Jul 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

v-jizhang commented Mar 26, 2021 •

edited

Loading

aweisberg left a comment •

edited

Loading