-
Notifications
You must be signed in to change notification settings - Fork 13k
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
[FLINK-2125][streaming] Delimiter change from char to string #2233
Conversation
This is my first pull request to Flink. Any comments would be very appreciative. |
token = token.substring(0, token.length() - 1); | ||
} | ||
if (!token.isEmpty()) { | ||
ctx.collect(token); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes existing behavior. Empty strings are filtered out, which wasn't done before.
Only had 2 small comments, otherwise this looks good to me. |
Hi @zentol , thanks for your comments. I have updated this PR. Please let me know if there are further improvements that need to be done. |
Because this breaks the public API, it would be good to do the following: Add a new method to the Take the old method, delegate to the new method, and mark it as |
@StephanEwen The socketTextStream methods are already marked as |
Hi @zentol , If you agree with @StephanEwen 's suggestion, I will update this PR in his way. |
Yes, |
ok, go ahead then. |
9c220e2
to
de2b11d
Compare
Hi, I have updated the PR as @StephanEwen suggested, adding two new methods named socketTextStream that take string delimiter as parameter and delegating the old method to the new one |
merging |
Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the How To Contribute guide.
In addition to going through the list, please provide a meaningful description of your changes.
mvn clean verify
has been executed successfully locally or a Travis build has passed