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.

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

Kinesis connector #476

Open
wants to merge 11 commits into
base: master
from

Conversation

7 participants
@ankitdixit
Copy link
Member

ankitdixit commented Mar 14, 2019

No description provided.

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

@findepi findepi referenced this pull request Mar 14, 2019

Closed

Kinesis connector #474

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

@ankitdixit

This comment has been minimized.

Copy link
Member Author

ankitdixit commented Mar 18, 2019

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

@findepi

This comment has been minimized.

Copy link
Member

findepi commented Mar 18, 2019

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

@martint
Copy link
Member

martint left a comment

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

Show resolved Hide resolved presto-kinesis/README.md Outdated
Show resolved Hide resolved presto-kinesis/README.md Outdated
Show resolved Hide resolved presto-kinesis/README.md Outdated
Show resolved Hide resolved presto-kinesis/README.md Outdated
Show resolved Hide resolved presto-kinesis/pom.xml
Show resolved Hide resolved ...s/src/main/java/io/prestosql/plugin/kinesis/KinesisConnectorFactory.java
Show resolved Hide resolved ...is/src/main/java/io/prestosql/plugin/kinesis/KinesisConnectorConfig.java Outdated
Show resolved Hide resolved ...s/src/main/java/io/prestosql/plugin/kinesis/KinesisConnectorFactory.java
Show resolved Hide resolved ...-kinesis/src/main/java/io/prestosql/plugin/kinesis/SessionVariables.java Outdated
Show resolved Hide resolved ...-kinesis/src/main/java/io/prestosql/plugin/kinesis/SessionVariables.java Outdated
@electrum
Copy link
Member

electrum left a comment

Some initial comments

Show resolved Hide resolved presto-kinesis/pom.xml
Show resolved Hide resolved presto-kinesis/pom.xml Outdated
Show resolved Hide resolved presto-kinesis/pom.xml Outdated
Show resolved Hide resolved presto-kinesis/pom.xml
Show resolved Hide resolved presto-kinesis/pom.xml Outdated
Show resolved Hide resolved .../main/java/io/prestosql/plugin/kinesis/s3config/S3TableConfigClient.java Outdated
Show resolved Hide resolved ...esis/src/main/resources/META-INF/services/com.facebook.presto.spi.Plugin Outdated
Show resolved Hide resolved ...rc/test/java/io/prestosql/plugin/kinesis/TestKinesisConnectorConfig.java Outdated
Show resolved Hide resolved ...rc/test/java/io/prestosql/plugin/kinesis/TestKinesisConnectorConfig.java Outdated
Show resolved Hide resolved ...in/java/io/prestosql/plugin/kinesis/KinesisTableDescriptionSupplier.java
@takezoe

This comment has been minimized.

Copy link
Member

takezoe commented Mar 22, 2019

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 added some commits Apr 2, 2019

@ankitdixit

This comment has been minimized.

Copy link
Member Author

ankitdixit commented Apr 3, 2019

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

@dain

This comment has been minimized.

Copy link
Member

dain commented Apr 3, 2019

@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

This comment has been minimized.

Copy link
Member Author

ankitdixit commented Apr 5, 2019

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

@dain
Copy link
Member

dain left a comment

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.

Show resolved Hide resolved presto-kinesis/pom.xml Outdated
pom.xml Outdated
<groupId>commons-logging</groupId>
<artifactId>commons-logging</artifactId>
</exclusion>
<!-- <exclusion>

This comment has been minimized.

Copy link
@dain

dain Apr 5, 2019

Member

Remove the commented out code.

Merge branch 'prestosqlmaster' into prestosql-kinesis
* prestosqlmaster: (141 commits)
  Fix generics for ConnectorMetadata applyFilter
  Run AbstractTestDistributedQueries for SQLServer
  Improve type mapping in SQLServer connector
  Update tempto to 164
  Enable AbstractTestDistributedQueries#testInsert for MySQL, PostgreSQL
  Rename test class to match the convention
  Remove bogus condition
  Enable Test{PostgreSql,MySql}DistributedQueries on Travis
  Disable testCommentTable for PostgreSQL, MySQL
  Fix `ALTER TABLE .. RENAME TO ..` for MySQL
  Quote variables in .travis.yml
  Move PostgreSQL and MySQL tests to separate group
  Deprecate Node.getHttpUri and reduce usages of Node.getHostAndPort
  Replace all usages of spi Node with InternalNode
  Rename PrestoNode to InternalNode
  Remove extra HiveBloomFilter class
  Optimize ORC Bloom filter hash
  Make Hive a test dependency for ORC module
  [maven-release-plugin] prepare for next development iteration
  [maven-release-plugin] prepare release 307
  ...

@dain dain requested review from electrum and martint Apr 9, 2019

ankitdixit added some commits Apr 12, 2019

Merge branch 'prestosqlmaster' into prestosql-kinesis
* prestosqlmaster: (49 commits)
  [maven-release-plugin] prepare for next development iteration
  [maven-release-plugin] prepare release 308
  Add 308 release notes
  Document JDBC KerberosServicePrincipalPattern parameter
  Increase table width for JDBC parameter reference
  Do not push limit through full outer join
  Remove derivation of FULL join properties
  Add limit pushdown for JDBC connectors
  Remove table layouts from JDBC connectors
  Improve rendering of JDBC column handles in explain plan
  Fix variable name spelling in QueryBuilder
  Use enforced constraint in EffectivePredicateExtractor
  Improve ORC timestamp reader
  Improve ORC slice dictionary reader
  Improve ORC decimal reader
  Improve ORC byte reader
  Improve ORC long reader
  Improve ORC list and map readers
  Improve ORC slice direct reader
  Improve ORC double and float readers
  ...
Merge branch 'prestosqlmaster' into prestosql-kinesis
* prestosqlmaster:
  Fix code after ConnectorId to CatalogName renamed
  Fail fast when plugin is not a plugin
  Add session to applyLimit and applyFilter
  Support writing of TINYINT type
  Add support for ARRAY type to PostgreSQL connector
  Allow overriding querying column metadata
  Add arrayDimensions to JdbcTypeHandle
  Fix typo in 308 release notes
  Fix string representation of JmxTableHandle
  Remove redundant condition for window frame
  Fix dependency injection for HttpBackupModule
  Indicate progress only if limit is applied
  Resolve tables/schemas case-insensitively in JDBC connectors
  Refactor listing tables in JDBC connector
  Extract mapping between Presto names and target names
  Remove docker container when startup not healthy
@ankitdixit

This comment has been minimized.

Copy link
Member Author

ankitdixit commented Apr 18, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.