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 Integration #2885

Closed
wants to merge 11 commits into from
Closed

Conversation

shubham166
Copy link

Issue : Kinesis connector #2366
Presto Connector to Amazon Kinesis.

@electrum
Copy link
Contributor

electrum commented May 8, 2015

Thanks for sending this! There seems to be a lot of duplication with the Kafka connector, especially around the decoding logic for rows/fields. How much of this could we pull out into a shared module? I think this could be useful for other connectors, too.

@shubham166
Copy link
Author

Yes, decoder logic for Kafka and Kinesis is almost same. It would be a really great idea to pull the decoder module to shared module, which can be used for other connectors in future.

@snarayanank2
Copy link

There is also an open PR for redis connector. Might be worth doing refactoring as a separate PR?

@losipiuk
Copy link
Contributor

looking into this one

@snarayanank2
Copy link

The latest version of the code is available here. Feel free to fork / send patches there.

Here's an AWS blogpost.

cumbersome and makes it difficult to write SQL queries.

A table definition file consists of a JSON definition for a table. The
name of the file can be arbitrary but must end in ``.json``.
Copy link
Contributor

Choose a reason for hiding this comment

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

must end with .json.

@losipiuk
Copy link
Contributor

General note:
There is a lot of codestyle violation errors. Just run mvn install.
If you are using IntelliJ there is codestyle config available here: https://github.com/airlift/codestyle

@losipiuk
Copy link
Contributor

Please rebase to master if possible - there were some changes in the spi.

import com.google.inject.name.Named;

public class KinesisMetadata
extends ReadOnlyConnectorMetadata
Copy link
Contributor

Choose a reason for hiding this comment

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

ReadOnlyConnectorMetdata is now gone and replaced with default methods in ConnectorMetadata interface.


<build>
<plugins>
<plugin>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why you need this plugin definition. It is defined in parent already.

@martint
Copy link
Contributor

martint commented Oct 14, 2015

The latest version of the code is available here. Feel free to fork / send patches there

@snarayanank2, this code was originally contributed by someone in your team, right? Are you still interested in getting it in given that you have a separate project to develop it? If not, we can just close this pull request.

@snarayanank2
Copy link

@martint - I'm not affiliated with Qubole anymore. @stagraqubole would be the right person to answer your question.

@shubhamtagra
Copy link

@martint we would like to have this in trunk. This pull request now needs quite a few changes to utilize the new presto-record-decoder module and to take care of comments by @losipiuk.

The original author has also moved on and we cannot use this to pull request for further changes. We can close this one and in sometime we will open a new request that handles the suggested changes.

@martint
Copy link
Contributor

martint commented Oct 15, 2015

@stagraqubole Sounds good. Let's do that, then.

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.

7 participants