Kinesis connector #476

ankitdixit · 2019-03-14T11:26:37Z

No description provided.

ankitdixit · 2019-03-18T05:58:07Z

@martint @findepi Can you guys please review it or point me to people who could?

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisConnectorFactory.java

findepi · 2019-03-18T08:00:31Z

@ankitdixit i'd love to review this, but i don't have enough bandwidth now. 😞

martint

Some high level comments from a quick scan of the code.

presto-kinesis/README.md

presto-kinesis/pom.xml

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisConnectorFactory.java

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisConnectorConfig.java

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisConnectorFactory.java

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/SessionVariables.java

electrum

Some initial comments

presto-kinesis/pom.xml

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/s3config/S3TableConfigClient.java

presto-kinesis/src/main/resources/META-INF/services/com.facebook.presto.spi.Plugin

presto-kinesis/src/test/java/io/prestosql/plugin/kinesis/TestKinesisConnectorConfig.java

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisTableDescriptionSupplier.java

takezoe · 2019-03-22T17:31:33Z

Is it acceptable to include KCL (amazon-kinesis-client)? KCL is licensed under Amazon Software License and its compatibility with Apache License had been argued in some other open source projects.

Probably it's OK if the connector will be distributed as source code and users build by themselves.

ankitdixit · 2019-04-03T11:36:50Z

@martint @electrum I have addressed comments, please have a look again.

dain · 2019-04-03T21:36:45Z

@takezoe and @ankitdixit: We looked over the license, and it seems ok for a connector. We will need to update the NOTICE file, the README file, and we will add a banner in the docs for this connector to let users know the license has additional restrictions. We will handle theses changes, so you don't need to do anything extra for this.

ankitdixit · 2019-04-05T06:13:38Z

@dain @electrum Can you please go through this PR again and let me know the next set of comments

dain

The build is failing due to the old version number in the new presto-kinesis/pom.xml file. Can you update that an resubmit so that Travis runs?

Also, I just skimmed over the code, and I think you missed a bunch of comments from @electrum.

presto-kinesis/pom.xml

pom.xml

ankitdixit · 2019-04-18T11:29:18Z

@martint @electrum I have addressed missed comments, can you guys please check this again

ankitdixit · 2019-04-24T04:41:43Z

@electrum @martint ping

martint

Some more comments.

BTW, did you forget to push some updates or is github getting confused with the merge commits? It doesn't seem to be showing updates based on earlier review comments.

Can you rebase on top of master and get rid of the merge commits to see if that fixes it?

presto-kinesis/pom.xml

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisClientManager.java

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisColumnHandle.java

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisConfig.java

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisConnector.java

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisConnectorFactory.java

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisConnector.java

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisConnectorFactory.java

ankitdixit · 2019-05-08T12:03:08Z

@martint @dain @electrum Sorry for the delayed response, I was traveling.
I have addressed the comments

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisConnector.java

ankitdixit · 2019-05-16T11:08:50Z

@electrum @martint Please have a look at this PR again

martint

@ankitdixit, it looks like some comments that are marked as resolved are still not addressed. Did some changes get lost when you squashed the commits?

martint · 2019-05-24T17:33:07Z

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisConfig.java

+        return iterFromTimestamp;
+    }
+
+    @Config("kinesis.iter-from-timestamp")


This still has the old name. Can you update?

martint · 2019-05-24T17:33:31Z

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisConfig.java

+        return iterOffsetSeconds;
+    }
+
+    @Config("kinesis.iter-offset-seconds")


ankitdixit · 2019-05-27T06:28:49Z

@ankitdixit, it looks like some comments that are marked as resolved are still not addressed. Did some changes get lost when you squashed the commits?

Yes, seems i missed them while squashing, fixed now.

electrum

Still reviewing, but some initial comments

presto-kinesis/README.md

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisMetadata.java

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisPlugin.java

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/s3config/S3TableConfigClient.java

presto-kinesis/src/test/java/io/prestosql/plugin/kinesis/util/MockKinesisClient.java

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisPlugin.java

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisRecordSet.java

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisSessionProperties.java

kokosing · 2019-08-07T07:46:17Z

@ankitdixit Have you managed to address all comments you got?

ankitdixit · 2019-08-16T11:23:18Z

@martint @electrum @kokosing I have gone through all the conversations and have tried to resolve all the comments
Here are 3 reccomendations which I have not incorporated yet with reasons:

Using latest SDK version :
Using latest version causes exceptions like: com.amazonaws.services.kinesis.model.InvalidArgumentException: The timestampInMillis parameter cannot be greater than the currentTimestampInMillis. timestampInMillis: 1565614647413000, currentTimestampInMillis: 1565701047548 (Service: AmazonKinesis; Status Code: 400; Error Code: InvalidArgumentException; Request ID: fdd2d515-614a-01d7-a81e-60e2343526fb)
And because of old version removing System.setProperty("com.amazonaws.sdk.disableCbor", "true”); causes: com.amazonaws.AmazonClientException: Unable to marshall request to JSON: com.fasterxml.jackson.dataformat.cbor.CBORGenerator.getOutputContext()Lcom/fasterxml/jackson/core/json/JsonWriteContext;
Kinesis connector #476 (comment)
KinesisColumnHandle implements DecoderColumnHandle, hence the getters still need to return String (for example), we can clean this up separately changing decoder module interfaces (common across redshift, kinesis and kafka).
In KinesisPlugin, still exposing a method to expose ClientProvider as tests in TestRecordAccess.java needs access to ClientProvider to add data to stream from within the test.
I could not come up with a better way to write tests there.

martint

Some additional comments. Also, it looks like you missed some comments from @electrum.

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisConfig.java

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisConnectorFactory.java

presto-docs/src/main/sphinx/connector/kinesis.rst

presto-kinesis/src/test/java/io/prestosql/plugin/kinesis/util/TestUtils.java

martint

I have a bunch of minor stylistic comments. For anything else, let's create a separate issue to track them and we can tackle them after we merge this first version.

The only issues I've found that we should address before merging are:

the "statefulness" of ConnectorFactory, which can cause problems when the connector is instantiated multiple times. If you'd like to chat about this more, please ping me on Slack.
some errors are being swallowed, which cause result in silent and subtle issues.

presto-docs/src/main/sphinx/connector/kinesis.rst

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisRecordSet.java

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisSessionProperties.java

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisShardCheckpointer.java

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/s3config/S3TableConfigClient.java

martint · 2019-09-24T13:48:44Z

Made a couple of minor adjustments and merged it. Thanks!

cla-bot bot added the cla-signed label Mar 14, 2019

findepi mentioned this pull request Mar 14, 2019

Kinesis connector #474

Closed

findepi changed the title ~~Adding kinesis connector~~ Kinesis connector Mar 14, 2019

Praveen2112 reviewed Mar 18, 2019

View reviewed changes

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisConnectorFactory.java Outdated Show resolved Hide resolved

martint reviewed Mar 18, 2019

View reviewed changes

electrum reviewed Mar 18, 2019

View reviewed changes

dain reviewed Apr 5, 2019

View reviewed changes

presto-kinesis/pom.xml Outdated Show resolved Hide resolved

pom.xml Outdated Show resolved Hide resolved

dain requested review from electrum and martint April 9, 2019 20:48

martint reviewed Apr 25, 2019

View reviewed changes

ankitdixit closed this May 8, 2019

ankitdixit reopened this May 8, 2019

electrum reviewed May 9, 2019

View reviewed changes

presto-kinesis/src/main/java/io/prestosql/plugin/kinesis/KinesisConnector.java Outdated Show resolved Hide resolved

ankitdixit force-pushed the prestosql-kinesis branch from 4efa077 to 848dafd Compare May 22, 2019 05:05

martint reviewed May 24, 2019

View reviewed changes

ankitdixit force-pushed the prestosql-kinesis branch from 848dafd to fa6b9b0 Compare May 27, 2019 06:26

dain requested review from martint and electrum June 1, 2019 02:03

ankitdixit force-pushed the prestosql-kinesis branch from 35b4465 to 3fd0c3e Compare June 3, 2019 05:13

electrum reviewed Jun 10, 2019

View reviewed changes

ankitdixit force-pushed the prestosql-kinesis branch 5 times, most recently from 9c9019d to 5a7baf5 Compare June 27, 2019 09:35

ankitdixit force-pushed the prestosql-kinesis branch 3 times, most recently from 6be40c2 to bfedeb9 Compare August 16, 2019 11:18

martint requested changes Aug 26, 2019

View reviewed changes

ankitdixit force-pushed the prestosql-kinesis branch 2 times, most recently from 7118f81 to 4e044aa Compare September 9, 2019 18:00

ankitdixit force-pushed the prestosql-kinesis branch from 4e044aa to 3d01675 Compare September 19, 2019 05:52

martint requested changes Sep 19, 2019

View reviewed changes

martint assigned ankitdixit Sep 19, 2019

ankitdixit force-pushed the prestosql-kinesis branch 3 times, most recently from 674026c to 10dfef1 Compare September 24, 2019 10:59

Kinesis connector

1bb6309

ankitdixit force-pushed the prestosql-kinesis branch from 10dfef1 to 1bb6309 Compare September 24, 2019 11:49

martint closed this Sep 24, 2019

martint added this to the 320 milestone Sep 24, 2019

martint mentioned this pull request Sep 24, 2019

Release notes for 320 #1570

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kinesis connector #476

Kinesis connector #476

ankitdixit commented Mar 14, 2019

ankitdixit commented Mar 18, 2019

findepi commented Mar 18, 2019

martint left a comment

electrum left a comment

takezoe commented Mar 22, 2019

ankitdixit commented Apr 3, 2019

dain commented Apr 3, 2019

ankitdixit commented Apr 5, 2019

dain left a comment

ankitdixit commented Apr 18, 2019

ankitdixit commented Apr 24, 2019

martint left a comment

ankitdixit commented May 8, 2019

ankitdixit commented May 16, 2019

martint left a comment

martint May 24, 2019

martint May 24, 2019

ankitdixit commented May 27, 2019

electrum left a comment

kokosing commented Aug 7, 2019

ankitdixit commented Aug 16, 2019 •

edited

Loading

martint left a comment

martint left a comment

martint commented Sep 24, 2019

Kinesis connector #476

Kinesis connector #476

Conversation

ankitdixit commented Mar 14, 2019

ankitdixit commented Mar 18, 2019

findepi commented Mar 18, 2019

martint left a comment

Choose a reason for hiding this comment

electrum left a comment

Choose a reason for hiding this comment

takezoe commented Mar 22, 2019

ankitdixit commented Apr 3, 2019

dain commented Apr 3, 2019

ankitdixit commented Apr 5, 2019

dain left a comment

Choose a reason for hiding this comment

ankitdixit commented Apr 18, 2019

ankitdixit commented Apr 24, 2019

martint left a comment

Choose a reason for hiding this comment

ankitdixit commented May 8, 2019

ankitdixit commented May 16, 2019

martint left a comment

Choose a reason for hiding this comment

martint May 24, 2019

Choose a reason for hiding this comment

martint May 24, 2019

Choose a reason for hiding this comment

ankitdixit commented May 27, 2019

electrum left a comment

Choose a reason for hiding this comment

kokosing commented Aug 7, 2019

ankitdixit commented Aug 16, 2019 • edited Loading

martint left a comment

Choose a reason for hiding this comment

martint left a comment

Choose a reason for hiding this comment

martint commented Sep 24, 2019

ankitdixit commented Aug 16, 2019 •

edited

Loading