Skip to content

Conversation

@rhajek
Copy link
Contributor

@rhajek rhajek commented Feb 11, 2020

What is this PR for?

The goal is to add support for querying InfluxDB 2.x using Flux language in Zeppelin notebook.

InfluxDB 2.0 (beta) docs
https://v2.docs.influxdata.com/v2.0/

Flux language docs:
https://docs.influxdata.com/flux/

What type of PR is it?

Feature

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

linked in docs/interpreter/influxdb.md

## Build

```
mvn -pl zeppelin-interpreter,zeppelin-display,influxdb -DskipTests package
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use mvn -pl influxdb -am -DskipTests package, influxdb depends on zeppelin-interpreter-shaded, so use -am would be easier. BTW, does influxdb interpreter depends on zeppelin-display ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The interpreter does not depends on zeppelin-display, I just copied it from another interpreter. It is very basic interpreter now. In future we would like to add syntax highlighting and code complete.

<dependency>
<groupId>com.influxdb</groupId>
<artifactId>influxdb-client-java</artifactId>
<version>${influxdb.client.version}</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the license of this dependency ? You need to put it in LICENCE file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MIT, I added license to zeppelin-distribution/src/bin_license/LICENSE

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 also added RxJava (Apache2 license) (used by InfluxDB client library)

<dependency>
<groupId>com.squareup.okhttp3</groupId>
<artifactId>mockwebserver</artifactId>
<version>${dependency.okhttp3.version}</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same licence issue as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apache 2, I added license to zeppelin-distribution/src/bin_license/LICENSE

influxdb/pom.xml Outdated
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<configuration>
<skip>false</skip>
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to enable it. We are planning to enable it for other modules as well.

@zjffdu
Copy link
Contributor

zjffdu commented Feb 26, 2020

@rhajek Could you do a rebase and trigger the travis CI again ?

@rhajek
Copy link
Contributor Author

rhajek commented Feb 26, 2020

It looks like there are some breaking changes, InfluxDBInterpreter.java:[28,39] cannot find symbol symbol: class BaseZeppelinContext, I will look on that.

@zjffdu
Copy link
Contributor

zjffdu commented Feb 29, 2020

@rhajek This is due to code refactoring which we just rename BaseZeppelinContext to ZeppelinContext

@zjffdu
Copy link
Contributor

zjffdu commented Mar 6, 2020

ping @rhajek Any update ?

@rhajek
Copy link
Contributor Author

rhajek commented Mar 6, 2020

Oh sorry, Instead of rebase I merged 'apache:master' into my fork 'rhajek:master'.

Now this PR should be ready for merge.

@timhallinflux
Copy link

@zjffdu how does this look now? Good to go?

@zjffdu
Copy link
Contributor

zjffdu commented Mar 13, 2020

@rajeshkp Could you paste the travis CI link ?

@rhajek
Copy link
Contributor Author

rhajek commented Mar 19, 2020

@rajeshkp Could you paste the travis CI link ?

Any updates ? Can I help with something ?

@zjffdu
Copy link
Contributor

zjffdu commented Mar 19, 2020

Sorry, I ping the wrong id, @rhajek Could you rebase this PR and paste the travis CI link ?

@rhajek
Copy link
Contributor Author

rhajek commented Mar 20, 2020

@zjffdu
Copy link
Contributor

zjffdu commented Mar 23, 2020

@rhajek There's several failure, may be due to flaky test. Could you rerun the build ?

@rhajek
Copy link
Contributor Author

rhajek commented Apr 2, 2020

I rebased and manually tested InfluxDB interpreter in zeppelin UI. Also mvn -pl influxdb -am test is passing. Travis build is also ok.

image

Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

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

LGTM...

@zjffdu what do you think?

static final String EMPTY_COLUMN_VALUE = "";

InfluxDBClient client;
QueryApi queryApi;
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to mark it as volatile ?

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'am not sure, I marked both variables as private and volatile.

@rhajek
Copy link
Contributor Author

rhajek commented Apr 28, 2020

I rebased PR, Travis build also looks ok.

@alexott
Copy link
Contributor

alexott commented Apr 28, 2020

LGTM

1 similar comment
@zjffdu
Copy link
Contributor

zjffdu commented Apr 28, 2020

LGTM

@asfgit asfgit closed this in 94300b3 May 1, 2020
asfgit pushed a commit that referenced this pull request May 1, 2020
### What is this PR for?
The goal is to add support for querying InfluxDB 2.x using Flux language in Zeppelin notebook.

InfluxDB 2.0 (beta) docs
https://v2.docs.influxdata.com/v2.0/

Flux language docs:
https://docs.influxdata.com/flux/

### What type of PR is it?
Feature

### What is the Jira issue?
* Open an issue on Jira https://issues.apache.org/jira/browse/ZEPPELIN-4602

### How should this be tested?
* First time? Setup Travis CI as described on https://zeppelin.apache.org/contribution/contributions.html#continuous-integration
* Strongly recommended: add automated unit tests for any new or changed behavior
* Outline any manual steps to test the PR here.

### Screenshots (if appropriate)
linked in docs/interpreter/influxdb.md

Author: Robert Hajek <[email protected]>

Closes #3640 from rhajek/master and squashes the following commits:

206848e [Robert Hajek] [ZEPPELIN-4602] updated influxdb client libraries to 1.7.0, added support for InfluxDB 1.8, interpreter code cleanup
34cd9ed [Robert Hajek] [ZEPPELIN-4602] BaseZeppelinContext replaced
8955205 [Robert Hajek] [ZEPPELIN-4602] Updated README.md, removed specific maven-checkstyle-plugin configuration
04b7e28 [Robert Hajek] [ZEPPELIN-4602] Added licences
b6b9a48 [Robert Hajek] [ZEPPELIN-4602] Added initial version of InfluxDB interpreter

(cherry picked from commit 94300b3)
Signed-off-by: Alex Ott <[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.

4 participants