Skip to content

Conversation

rmetzger
Copy link
Contributor

I removed the FileSinkFunctionByMillis and removed all the millis arguments on the writing functions.

The whole "buffering" and "flushing" functionality was broken: Elements were kept in an ArrayList and send to the OutputFormat on "flush()". However, the flush was not really called periodically. It was only checked when new records arrived. So when a stream is not having elements for a certain time, the last few elements would just stay in the list until new elements arrive again.

@mbalassi
Copy link
Contributor

With the RollingHDFSSink this functionality is not needed any more and as you suggested apparently was a misleading implmentation anyway. I like your not to the docs. 👍
Could you add [api-breaking] to the commit msg though please, when merging?

Copy link
Contributor

Choose a reason for hiding this comment

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

May be worth adding that this means usually "at-least-once" , but may also mean data loss in cases where the output formats buffer data and do not immediately persist it.

@StephanEwen
Copy link
Contributor

Code looks good. I would like to update the docs to include a bit more info (like in the inline comment) and at least refer to the addSink(...) method, for fault tolerant sinks.

@tillrohrmann
Copy link
Contributor

Changes look good to me

@rmetzger rmetzger force-pushed the flink3296 branch 2 times, most recently from a2716ea to 205a79a Compare February 12, 2016 14:42
@rmetzger
Copy link
Contributor Author

Thank you for the review. I've addressed the comments and rebased the change.

Once travis has passed, I'll merge it!

@aljoscha
Copy link
Contributor

I think the writeOutputFormat method name could be misleading. To me it implies writing the OutputFormat not writing something by using the OutputFormat.

@rmetzger
Copy link
Contributor Author

I renamed the method to writeUsingOutputFormat and rebased to current master.

@rmetzger rmetzger force-pushed the flink3296 branch 2 times, most recently from 6d9a0cf to 008a1a7 Compare February 16, 2016 11:32
@rmetzger
Copy link
Contributor Author

I'll merge the PR.

@asfgit asfgit closed this in 2714aaf Feb 16, 2016
@rmetzger rmetzger deleted the flink3296 branch February 16, 2016 15:02
subhankarb pushed a commit to subhankarb/flink that referenced this pull request Mar 17, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling df49d5b on rmetzger:flink3296 into ** on apache:master**.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants