Skip to content

Conversation

@bbonnin
Copy link
Contributor

@bbonnin bbonnin commented Jan 15, 2017

What is this PR for?

Add HTTP client to elasticsearch interpreter.

What type of PR is it?

Feature

Todos

  • - Source code
  • - Tests
  • - License
  • - Docs

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-1821

How should this be tested?

  • Start an Elasticsearch node
  • Configure the elasticsearch interpreter to use http
  • Create queries in a note using elasticsearch

Screenshots (if appropriate)

Questions:

  • Does the licenses files need update? Yes
  • Is there breaking changes for older versions? No
  • Does this needs documentation? Yes

@1ambda
Copy link
Member

1ambda commented Jan 17, 2017

Great contribution for elasticsearch interpreter! This PR enables us to support ES 5.0 easily!

@jongyoul
Copy link
Member

@1ambda Can you review this PR deeply?

@Leemoonsoo
Copy link
Member

@bbonnin Recently we have fixed some flaky test on master branch. Could you try rebase and see if CI becomes green?

@bbonnin
Copy link
Contributor Author

bbonnin commented Jan 23, 2017

After rebase, it looks better now, all is green !

@1ambda
Copy link
Member

1ambda commented Jan 24, 2017

@bbonnin 👍

@jongyoul I will take a look more deeply :)

@Leemoonsoo
Copy link
Member

LGTM @1ambda do you have any comment?

@jaspinderdineout
Copy link

jaspinderdineout commented Jan 27, 2017

When will this be merged? And which release will it be part of?

@1ambda
Copy link
Member

1ambda commented Jan 27, 2017

Works well as described.

  • tested on elasticsearch 2.4.4 (transport, http) and 5.0.0 (http only)
  • code looks nice. well abstracted client classes are shipped with necessary tests.

there some comments. LGTM except that.

private final List<AggWrapper> aggregations = new LinkedList<>();


// public ActionResponse source(String source) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed the commed code lines.

return succeeded;
}

// public String getSource() {
Copy link
Member

Choose a reason for hiding this comment

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

comment code lines

@Leemoonsoo
Copy link
Member

CI failure looks unrelated, since it only removed commented-out code from last CI success.
Thanks @bbonnin for great contribution.
LGTM and merge to master if no further discussions!

@asfgit asfgit closed this in e763b3b Jan 29, 2017
asfgit pushed a commit that referenced this pull request Jan 31, 2017
### What is this PR for?
Add HTTP client to elasticsearch interpreter.

### What type of PR is it?
Feature

### Todos
* [X] - Source code
* [X] - Tests
* [X] - License
* [X] - Docs

### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-1821

### How should this be tested?
* Start an Elasticsearch node
* Configure the elasticsearch interpreter to use http
* Create queries in a note using elasticsearch

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? Yes
* Is there breaking changes for older versions? No
* Does this needs documentation? Yes

Author: Bruno Bonnin <[email protected]>
Author: Bruno Bonnin <[email protected]>

Closes #1902 from bbonnin/master and squashes the following commits:

f5a539e [Bruno Bonnin] Remove commented code lines
86153a8 [Bruno Bonnin] Merge remote-tracking branch 'upstream/master'
2e1bbbd [Bruno Bonnin] Merge remote-tracking branch 'upstream/master'
19e888e [Bruno Bonnin] Remove bad code in test
523d155 [Bruno Bonnin] Replace Java 8 methods
6bcf369 [Bruno Bonnin] Fix issue with id containing special chars (/, #)
4e9812e [Bruno Bonnin] Merge elasticsearch/pom.xml
5a96ae0 [Bruno Bonnin] Merge branch 'master' into master
e2365fb [Bruno Bonnin] Update elasticsearch/pom.xml
28b9805 [Bruno Bonnin] Update img
549db39 [Bruno Bonnin] Add HTTP client to elasticsearch interpreter
f4c5ac3 [Bruno Bonnin] HTTP-based Elasticsearch client

(cherry picked from commit e763b3b)
Signed-off-by: Mina Lee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants