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

Apache Flink module #92

Merged
merged 2 commits into from
Jan 19, 2016
Merged

Apache Flink module #92

merged 2 commits into from
Jan 19, 2016

Conversation

gesundkrank
Copy link
Contributor

This module adds support for Apache Flink. Works and looks a lot like the Spark module. But instead of the BeamRDD it contains BeamSink.

Had some struggle with the Flink dependency imports since they are not following the Scala naming conventions.

Would be great to get some feedback on this and even greater to get it merge.

.builder((eventMap: SimpleEvent) => new DateTime())
.curator(curator)
.discoveryPath(discoveryPath)
.location(DruidLocation(indexService, firehosePattern, dataSource))
Copy link
Member

Choose a reason for hiding this comment

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

firehosePattern is no longer needed and most users will find it easier to not worry about it. Could you remove that from the example?

val aggregators = Seq(new LongSumAggregatorFactory("baz", "baz"))

DruidBeams
.builder((eventMap: SimpleEvent) => new DateTime())
Copy link
Member

Choose a reason for hiding this comment

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

Using new DateTime for the timestamper is going to lead to mysterious drops (when tranquility thinks an event is ok and reports it sent, but druid silently drops it). I guess you copied this from the spark example, so we should fix both. I don't think that has to happen as part of this PR though.

Filed #93 to remember to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Just copied the Spark example. And I think it makes sense to fix both examples at the same time. Won't fix it in this PR.

@gianm
Copy link
Member

gianm commented Jan 15, 2016

@gesundkrank thanks for the contribution! Could you please take a look at the comments on the patch, squash your commits (see https://github.com/druid-io/druid/blob/master/CONTRIBUTING.md), and fill out the Druid CLA at http://druid.io/community/cla.html?

@gesundkrank
Copy link
Contributor Author

@gianm Thank you very much for your review! I squashed and commited the fixups.
My CTO sent you the Druid CLA a few hours ago.

@gianm
Copy link
Member

gianm commented Jan 19, 2016

👍 looks good!

@gesundkrank I don't see you in the CLA list- do you fall under a corporate CLA? If so which one?

@gesundkrank
Copy link
Contributor Author

Yes. My company is mbr targeting.

@xvrl
Copy link
Member

xvrl commented Jan 19, 2016

@gesundkrank thank you, received the CLA

gianm added a commit that referenced this pull request Jan 19, 2016
@gianm gianm merged commit 2d6804e into druid-io:master Jan 19, 2016
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.

None yet

3 participants