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.

Sign up for GitHub

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

Jump to bottom

Add support for user/password auth - Elasticsearch #16524

Merged
merged 5 commits into from
Dec 20, 2022
Merged

Add support for user/password auth - Elasticsearch #16524

merged 5 commits into from
Dec 20, 2022

Conversation

v-jizhang
Copy link
Contributor

Cherry-pick of
trinodb/trino@bd4b3dd

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

Test plan - (Please fill in how you tested your changes)

== RELEASE NOTES ==

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

@v-jizhang v-jizhang force-pushed the elastic_password_2 branch from 09d857b to e017572 Compare July 27, 2021 00:42
@v-jizhang v-jizhang requested a review from aweisberg July 27, 2021 19:52
@v-jizhang v-jizhang mentioned this pull request Jul 27, 2021
Closed
@v-jizhang v-jizhang force-pushed the elastic_password_2 branch 2 times, most recently from e0b6f75 to a5d8bb9 Compare July 27, 2021 21:04
@abmn614
Copy link

abmn614 commented Nov 22, 2021

Why hasn't this PR been submitted yet? Anyone knows some news?

@stale
Copy link

stale bot commented Sep 21, 2022

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the task, make sure you've addressed reviewer comments, and rebase on the latest master. Thank you for your contributions!

@stale stale bot added the stale label Sep 21, 2022
@v-jizhang
Copy link
Contributor Author

Reviewed in #15877

@rschlussel rschlussel requested review from zhenxiao and removed request for aweisberg December 6, 2022 15:56
@stale stale bot removed stale labels Dec 6, 2022
@zhenxiao
Copy link
Collaborator

hi @v-jizhang could you please resolved the conflict?

@v-jizhang v-jizhang requested a review from a team as a code owner December 14, 2022 17:12
@v-jizhang v-jizhang requested a review from presto-oss December 14, 2022 17:12
rschlussel
rschlussel reviewed Dec 14, 2022
View reviewed changes
Copy link
Contributor

@rschlussel rschlussel left a comment

Choose a reason for hiding this comment

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

I'll leave @zhenxiao to do a proper review, but just a couple things I noticed.

presto-main/src/test/java/com/facebook/presto/sql/query/QueryAssertions.java Outdated
@@ -53,6 +54,11 @@ public QueryAssertions(Session session)
runner = new LocalQueryRunner(session);
Copy link
Contributor

Choose a reason for hiding this comment

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

would be good to update this constructor to call the new one
this(new LocalQueryRunner(requireNonNull(session, "session is null")))

presto-elasticsearch/src/test/java/com/facebook/presto/elasticsearch/ElasticsearchLoader.java Outdated
@@ -108,8 +108,13 @@ public void addResults(QueryStatusInfo statusInfo, QueryData data)
throw new UncheckedIOException("Error loading data into Elasticsearch index: " + tableName, e);
}
}
client.bulk(request).actionGet();
client.admin().indices().flush(flushRequest(tableName)).actionGet();
request.setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE);
Copy link
Contributor

Choose a reason for hiding this comment

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

static import IMMEDIATE

...-elasticsearch/src/test/java/com/facebook/presto/elasticsearch/ElasticsearchQueryRunner.java
@@ -120,6 +120,9 @@ public static Session createSession()
public static void main(String[] args)
throws Exception
{
// To start Elasticsearch:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a readme too with any instructions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Working on it.

v-jizhang and others added 4 commits December 14, 2022 15:54
@v-jizhang @martint
Migrate Elasticsearch tests to testcontainers
ceab1a4 
Cherry-pick of
trinodb/trino@bd4b3dd

Co-authored-by: Martin Traverso <mtraverso@gmail.com>
@v-jizhang @martint
Add instructions on how to start Elasticsearch
98fad61 
Cherry-pick of trinodb/trino#3331

Co-authored-by: Martin Traverso <mtraverso@gmail.com>
@v-jizhang @martint
Add a constructor to QueryAssertions
46f026f 
Cherry-pick of
trinodb/trino@7efb49c

Co-authored-by: Martin Traverso <mtraverso@gmail.com>
@v-jizhang @martint
Use assertj for query assertions
7077a02 
Cherry-pick of
trinodb/trino@a381258

Co-authored-by: Martin Traverso <mtraverso@gmail.com>
zhenxiao
zhenxiao approved these changes Dec 15, 2022
View reviewed changes
Copy link
Collaborator

@zhenxiao zhenxiao left a comment

Choose a reason for hiding this comment

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

hi @v-jizhang nice work
looks good. 2 minor things

presto-elasticsearch/src/main/java/com/facebook/presto/elasticsearch/ElasticsearchConfig.java Outdated
@@ -32,7 +32,8 @@
{
public enum Security
{
AWS
AWS,
PASSWORD,
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we remove the , after PASSWORD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

...h/src/test/java/com/facebook/presto/elasticsearch/TestElasticsearchIntegrationSmokeTest.java Outdated
@@ -57,12 +57,15 @@
protected QueryRunner createQueryRunner()
throws Exception
{
elasticsearch = new ElasticsearchServer();
elasticsearch = new ElasticsearchServer("docker.elastic.co/elasticsearch/elasticsearch-oss:6.0.0", ImmutableMap.of());
Copy link
Collaborator

Choose a reason for hiding this comment

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

shall we make it a constant or configurable?
docker.elastic.co/elasticsearch/elasticsearch-oss:6.0.0

Copy link
Collaborator

Choose a reason for hiding this comment

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

kindly ping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made it a constant. Thanks for your reviews.

@v-jizhang v-jizhang force-pushed the elastic_password_2 branch 2 times, most recently from 702ffe7 to 84ee838 Compare December 16, 2022 02:33
@zhenxiao
Copy link
Collaborator

looks good. @v-jizhang could you please take a look at the 2 failing tests?

@zhenxiao
Copy link
Collaborator

nice. thank you, @v-jizhang could you please merge the last commit into previous ones?
I plan to merge this PR after the change

@v-jizhang @martint
Add support for user/password auth
9542b19 
Cherry-pick of trinodb/trino#4165

Co-authored-by: Martin Traverso <mtraverso@gmail.com>
@zhenxiao zhenxiao merged commit 1c4822c into prestodb:master Dec 20, 2022
@wanglinsong wanglinsong mentioned this pull request Jan 12, 2023
Merged
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhenxiao zhenxiao zhenxiao approved these changes

@rschlussel rschlussel rschlussel left review comments

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

Assignees
No one assigned
Labels
awaiting-community-help
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@v-jizhang @abmn614 @zhenxiao @rschlussel @rohanpednekar