[BAHIR-274] Add Flink InfluxDBv2.0 Connector #114

1p4pk · 2021-03-10T19:56:15Z

Hi @AHeise,

@Shark, @raminqaf and myself have prepared the first draft.

We have one question that we directly commented in the code.

Best,
Felix, Ramin & Leon

...ain/java/org/apache/flink/streaming/connectors/influxdb/sink/commiter/InfluxDBCommitter.java

eskabetxe

Thanks @AHeise, @Shark and @raminqaf for the contribution :)

I have added a few comments.

flink-connector-influxdb2/pom.xml

...luxdb2/src/main/java/org/apache/flink/streaming/connectors/influxdb/common/InfluxParser.java

pom.xml

eskabetxe · 2021-03-11T08:17:47Z

pom.xml

@@ -75,6 +75,7 @@
    <module>flink-connector-akka</module>
    <module>flink-connector-flume</module>
    <module>flink-connector-influxdb</module>
+    <module>flink-connector-influxdb2</module>


why create another module?
the first one could not be updated?

Unfortunately not. Since there are breaking changes in InfluxDB 2.x. Even the java client is written from scratch and it's not compatible with version 1.7 anymore. More information here. We created a separate module to avoid a situation where other users still rely on InfluxDB 1.x connectors.

should we maintain the 1.7 version? its 10 versions down from last one

I guess yes, because we implemented influxDB 2.X which is still quite new. I guess a lot of people still use the old api.

...c/main/java/org/apache/flink/streaming/connectors/influxdb/source/InfluxDBSourceBuilder.java

flink-connector-influxdb2/pom.xml

AHeise

Thank you very much for your contributions. The new CDC source should be interesting for many IoT use cases. A couple of bigger issues:

Exception handling should be more explicit. In most cases, it should be simply getting rid of SneakyThrows and use IOException in the interfaces.
Parsing of lines seem to be suboptimal.
Some important javadocs are missing, especially on the user-facing issues.
Please also double-check if you could hide some classes by making them package-private. In this way, the user will only see a few Influx classes in their autocompletion. For example, all the split/enumerator serializers should be non-public.
Ditch Properties in favor of Configuration unless there is a good reason to use that.

I'm very happy with the test coverage already and I think the tests are well-structured such that adding new tests later should be very easy.

flink-connector-influxdb2/README.md

flink-connector-influxdb2/src/main/resources/log4j.properties

...2/src/test/java/org/apache/flink/streaming/connectors/InfluxDBSourceIntegrationTestCase.java

...or-influxdb2/src/test/java/org/apache/flink/streaming/connectors/util/InfluxDBContainer.java

...uxdb2/src/test/java/org/apache/flink/streaming/connectors/util/InfluxDBTestDeserializer.java

...nector-influxdb2/src/test/java/org/apache/flink/streaming/connectors/util/RetentionUnit.java

AHeise

Already looking very good. What's left:

double-check the time unit of DataPoint.
remove spotless from pom
remove log4js.properties (leave it in test/)
fix test (on Travis)
Wait for approval/merge by Bahir committer.

...influxdb2/src/main/java/org/apache/flink/streaming/connectors/influxdb/common/DataPoint.java

...rc/main/java/org/apache/flink/streaming/connectors/influxdb/source/http/WriteAPIHandler.java

flink-connector-influxdb2/src/main/resources/log4j2.properties

1p4pk · 2021-03-18T18:55:21Z

Hi @rmetzger,

could you also review this commit?

Thank you.

Best,
Leon

eskabetxe

LGTM, minor changes requested

flink-connector-influxdb2/pom.xml

...influxdb2/src/main/java/org/apache/flink/streaming/connectors/influxdb/common/DataPoint.java

eskabetxe · 2021-03-30T17:15:53Z

pom.xml

@@ -75,6 +75,7 @@
    <module>flink-connector-akka</module>
    <module>flink-connector-flume</module>
    <module>flink-connector-influxdb</module>
+    <module>flink-connector-influxdb2</module>


should we maintain the 1.7 version? its 10 versions down from last one

...java/org/apache/flink/streaming/connectors/influxdb/sink/writer/InfluxDBPointSerializer.java

rmetzger · 2021-04-07T11:53:17Z

I don't think we can add the presentation in flink-connector-influxdb2/media/benchmarks.pdf to bahir.
Effectively, we are releasing the pdf under the ASL 2.0, but I'm pretty sure that the pdf contains pictures which we haven't created outselves.

rmetzger · 2021-04-07T11:53:39Z

Maybe host the PDF somewhere else and link to it?

...luxdb2/src/main/java/org/apache/flink/streaming/connectors/influxdb/common/InfluxParser.java

bkahloon · 2021-04-19T23:25:03Z

Hi @1p4pk , @Shark, @raminqaf

I was just wondering if the source connector would be usable via the Flink SQL api. If there isn't a direct integration available with this PR, what work do you suspect would be needed.

Thank you for your help.

1p4pk · 2021-04-20T06:59:19Z

Hi @bkahloon,

my first guess would be no, as this connector implements the DataStream source API. However, based on the documentation, you need to implement the Table API & SQL user-defined source and sink API for that.

@AHeise should know better, correct me if I am mistaken.

Best,
Leon

1p4pk · 2021-04-20T07:00:14Z

@AHeise we added the last suggestions, but I could only re-request a review from one person. Can you have a look again?

rmetzger

+1 to merge this PR

Thanks a lot for your contribution!

rmetzger · 2021-04-29T12:41:45Z

...c/main/java/org/apache/flink/streaming/connectors/influxdb/source/InfluxDBSourceBuilder.java

+public final class InfluxDBSourceBuilder<OUT> {
+
+    private InfluxDBDataPointDeserializer<OUT> deserializationSchema;
+    // Configurations


not: this comment is not really adding value

Delete it before merging?

rmetzger · 2021-04-29T12:41:59Z

flink-connector-influxdb2/license-header

+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */


what's the benefit of this file

Can be deleted. Was used for spotless::apply.

rmetzger · 2021-04-29T12:49:31Z

I'll merge this PR in the next 24 hours (I want to leave some time because I'm not merging things that frequently here in bahir)

rmetzger · 2021-05-03T06:23:56Z

Thanks for the additional approval @eskabetxe. I'll merge this now!

rmetzger · 2021-05-03T06:28:56Z

For your next contribution to Bahir, please create a Jira ticket: https://issues.apache.org/jira/browse/BAHIR-274

rmetzger · 2021-05-03T06:37:55Z

@1p4pk Could you squash all your commits into one, prefixed with "[BAHIR-274] .." ?
I tried squashing your change, but it seems that you've merged stuff from master in the your branch in between your work. I tried rebasing to master, but that caused conflicts, which I don't have time to resolve right now.

Just force push the cleaned up branch here, then I'll merge it.

Sorry for the back and forth.

1p4pk · 2021-05-11T11:24:45Z

@rmetzger squashed.

rmetzger · 2021-05-11T11:27:51Z

Thanks a lot. It seems that there are a number of conflicts with master. Could you resolve them?

1p4pk · 2021-05-11T11:29:09Z

Yes @raminqaf squashed all commits ever done in this repo. He is trying to fix it...

AHeise · 2021-05-11T11:43:07Z

Probably easiest to (re)start from the latest unsquashed commit 03219bf55f6de444dff62422f29062b7a9c1451c.

raminqaf · 2021-05-11T16:27:47Z

@rmetzger Everything should be ready and set for the merge now! 🚀 only one small thing:
I noticed that the Travis CI Pipeline is failing (for both environments, scala 2.11 & 2.12) due to three tests in the flume connector. Here is a screenshot of the Travis logs:

I found a quick fix for it: I just updated to the newest version of testcontainers, and it seems that this fixes the problem. I created a PR for this issue. The pipeline passes successfully. I didn't know how to create an issue on Jira but maybe you can help me out on that :) Also, I couldn't add anyone to the reviewers on my PR.

rmetzger · 2021-05-20T11:54:50Z

Thanks a lot for the hotfix. Could you rebase to the latest master to see if the build now passes?

Co-authored-by: Leon Papke <leonpapke@gmx.de> Co-authored-by: Felix Seidel <felix@seidel.me>

raminqaf · 2021-05-21T08:14:46Z

@rmetzger sry for all the continuous pushes... there were a bunch of conflicts and branch hell going on.. everything should be in place and correct now!

rmetzger · 2021-05-25T18:11:07Z

Thanks a lot for adjusting!

Merging PR.

raminqaf reviewed Mar 10, 2021

View reviewed changes

...ain/java/org/apache/flink/streaming/connectors/influxdb/sink/commiter/InfluxDBCommitter.java Outdated Show resolved Hide resolved

eskabetxe suggested changes Mar 11, 2021

View reviewed changes

AHeise suggested changes Mar 12, 2021

View reviewed changes

AHeise reviewed Mar 18, 2021

View reviewed changes

1p4pk marked this pull request as ready for review March 24, 2021 14:44

1p4pk requested a review from eskabetxe March 24, 2021 15:11

eskabetxe suggested changes Mar 30, 2021

View reviewed changes

eskabetxe reviewed Mar 30, 2021

View reviewed changes

...java/org/apache/flink/streaming/connectors/influxdb/sink/writer/InfluxDBPointSerializer.java Outdated Show resolved Hide resolved

rmetzger reviewed Apr 7, 2021

View reviewed changes

...luxdb2/src/main/java/org/apache/flink/streaming/connectors/influxdb/common/InfluxParser.java Outdated Show resolved Hide resolved

1p4pk requested a review from eskabetxe April 20, 2021 06:52

lresende requested a review from rmetzger April 24, 2021 07:53

rmetzger approved these changes Apr 29, 2021

View reviewed changes

eskabetxe approved these changes Apr 29, 2021

View reviewed changes

raminqaf force-pushed the dev branch from 03219bf to b0db9b6 Compare May 11, 2021 11:20

1p4pk changed the title ~~InfluxDBv2.0 Connector~~ [BAHIR-274] Add Flink InfluxDBv2.0 Connector May 11, 2021

raminqaf force-pushed the dev branch from b0db9b6 to 693d6aa Compare May 11, 2021 12:31

raminqaf force-pushed the dev branch 3 times, most recently from f44ad5e to 9315335 Compare May 11, 2021 13:14

raminqaf force-pushed the dev branch 2 times, most recently from 1fbe8d6 to 2d4e331 Compare May 21, 2021 08:07

[BAHIR-274] Add Flink InfluxDBv2.0 Connector

716e449

Co-authored-by: Leon Papke <leonpapke@gmx.de> Co-authored-by: Felix Seidel <felix@seidel.me>

raminqaf force-pushed the dev branch from 2d4e331 to 716e449 Compare May 21, 2021 08:10

rmetzger merged commit 809a588 into apache:master May 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BAHIR-274] Add Flink InfluxDBv2.0 Connector #114

[BAHIR-274] Add Flink InfluxDBv2.0 Connector #114

1p4pk commented Mar 10, 2021 •

edited

eskabetxe left a comment

eskabetxe Mar 11, 2021

raminqaf Mar 11, 2021 •

edited

eskabetxe Mar 30, 2021

1p4pk Apr 20, 2021

AHeise left a comment

AHeise left a comment

1p4pk commented Mar 18, 2021

eskabetxe left a comment

eskabetxe Mar 30, 2021

rmetzger commented Apr 7, 2021

rmetzger commented Apr 7, 2021

bkahloon commented Apr 19, 2021 •

edited

1p4pk commented Apr 20, 2021

1p4pk commented Apr 20, 2021

rmetzger left a comment

rmetzger Apr 29, 2021

1p4pk Apr 30, 2021

rmetzger Apr 29, 2021

1p4pk Apr 30, 2021

rmetzger commented Apr 29, 2021

rmetzger commented May 3, 2021

rmetzger commented May 3, 2021

rmetzger commented May 3, 2021

1p4pk commented May 11, 2021 •

edited

rmetzger commented May 11, 2021

1p4pk commented May 11, 2021

AHeise commented May 11, 2021

raminqaf commented May 11, 2021 •

edited

rmetzger commented May 20, 2021

raminqaf commented May 21, 2021

rmetzger commented May 25, 2021

[BAHIR-274] Add Flink InfluxDBv2.0 Connector #114

[BAHIR-274] Add Flink InfluxDBv2.0 Connector #114

Conversation

1p4pk commented Mar 10, 2021 • edited

eskabetxe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raminqaf Mar 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AHeise left a comment

Choose a reason for hiding this comment

AHeise left a comment

Choose a reason for hiding this comment

1p4pk commented Mar 18, 2021

eskabetxe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmetzger commented Apr 7, 2021

rmetzger commented Apr 7, 2021

bkahloon commented Apr 19, 2021 • edited

1p4pk commented Apr 20, 2021

1p4pk commented Apr 20, 2021

rmetzger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmetzger commented Apr 29, 2021

rmetzger commented May 3, 2021

rmetzger commented May 3, 2021

rmetzger commented May 3, 2021

1p4pk commented May 11, 2021 • edited

rmetzger commented May 11, 2021

1p4pk commented May 11, 2021

AHeise commented May 11, 2021

raminqaf commented May 11, 2021 • edited

rmetzger commented May 20, 2021

raminqaf commented May 21, 2021

rmetzger commented May 25, 2021

1p4pk commented Mar 10, 2021 •

edited

raminqaf Mar 11, 2021 •

edited

bkahloon commented Apr 19, 2021 •

edited

1p4pk commented May 11, 2021 •

edited

raminqaf commented May 11, 2021 •

edited