You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by delding <gi...@git.apache.org> on 2016/07/13 03:33:53 UTC
[GitHub] flink pull request #2233: [FLINK-2125][streaming] Delimiter change from char...
GitHub user delding opened a pull request:
https://github.com/apache/flink/pull/2233
[FLINK-2125][streaming] Delimiter change from char to string
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](http://flink.apache.org/how-to-contribute.html).
In addition to going through the list, please provide a meaningful description of your changes.
- [ ] General
- The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
- The pull request addresses only one issue
- Each commit in the PR has a meaningful commit message (including the JIRA id)
- [ ] Documentation
- Documentation has been added for new functionality
- Old documentation affected by the pull request has been updated
- JavaDoc for public methods has been added
- [ ] Tests & Build
- Functionality added by the pull request is covered by tests
- `mvn clean verify` has been executed successfully locally or a Travis build has passed
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/delding/flink master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2233.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2233
----
commit 7c3491719edfb99a4809951de34758548397e833
Author: erli ding <ed...@us.ibm.com>
Date: 2016-07-13T03:29:34Z
[FLINK-2125][streaming] Delimiter change from char to string
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2233: [FLINK-2125][streaming] Delimiter change from char...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/2233
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2233: [FLINK-2125][streaming] Delimiter change from char to str...
Posted by delding <gi...@git.apache.org>.
Github user delding commented on the issue:
https://github.com/apache/flink/pull/2233
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2233: [FLINK-2125][streaming] Delimiter change from char to str...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2233
Only had 2 small comments, otherwise this looks good to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2233: [FLINK-2125][streaming] Delimiter change from char...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2233#discussion_r70614465
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunction.java ---
@@ -96,19 +96,21 @@ public void run(SourceContext<String> ctx) throws Exception {
socket.connect(new InetSocketAddress(hostname, port), CONNECTION_TIMEOUT_TIME);
BufferedReader reader = new BufferedReader(new InputStreamReader(socket.getInputStream()));
- int data;
- while (isRunning && (data = reader.read()) != -1) {
- // check if the string is complete
- if (data != delimiter) {
- buffer.append((char) data);
- }
- else {
+ char[] cbuf = new char[8192];
+ int byteRead;
+ while (isRunning && (byteRead = reader.read(cbuf)) != -1) {
+ buffer.append(cbuf, 0, byteRead);
+ int delimPos;
+ while (buffer.length() >= delimiter.length() && (delimPos = buffer.indexOf(delimiter)) != -1) {
+ String token = buffer.substring(0, delimPos);
// truncate trailing carriage return
- if (delimiter == '\n' && buffer.length() > 0 && buffer.charAt(buffer.length() - 1) == '\r') {
- buffer.setLength(buffer.length() - 1);
+ if (delimiter.equals("\n") && token.endsWith("\r")) {
+ token = token.substring(0, token.length() - 1);
+ }
+ if (!token.isEmpty()) {
+ ctx.collect(token);
--- End diff --
This changes existing behavior. Empty strings are filtered out, which wasn't done before.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2233: [FLINK-2125][streaming] Delimiter change from char to str...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2233
merging
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2233: [FLINK-2125][streaming] Delimiter change from char to str...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2233
Because this breaks the public API, it would be good to do the following:
Add a new method to the `StreamExecutionEnvironment`, rather than changing the old method. Tag that new method as `@PublicEvolving`.
Take the old method, delegate to the new method, and mark it as `@Deprecated`. Also add a proper deprecation comment.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2233: [FLINK-2125][streaming] Delimiter change from char...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2233#discussion_r70614809
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunction.java ---
@@ -96,19 +96,21 @@ public void run(SourceContext<String> ctx) throws Exception {
socket.connect(new InetSocketAddress(hostname, port), CONNECTION_TIMEOUT_TIME);
BufferedReader reader = new BufferedReader(new InputStreamReader(socket.getInputStream()));
- int data;
- while (isRunning && (data = reader.read()) != -1) {
- // check if the string is complete
- if (data != delimiter) {
- buffer.append((char) data);
- }
- else {
+ char[] cbuf = new char[8192];
+ int byteRead;
--- End diff --
This variable should be named `bytesRead` .
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2233: [FLINK-2125][streaming] Delimiter change from char to str...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2233
@StephanEwen The socketTextStream methods are already marked as `@PublicEvolving`, i thought changing those was allowed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2233: [FLINK-2125][streaming] Delimiter change from char to str...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2233
Yes, `@PublicEvolving` allows us to change things, but it is still nice for users if we give them a smooth transition :-)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2233: [FLINK-2125][streaming] Delimiter change from char to str...
Posted by delding <gi...@git.apache.org>.
Github user delding commented on the issue:
https://github.com/apache/flink/pull/2233
Hi @zentol , If you agree with @StephanEwen 's suggestion, I will update this PR in his way.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2233: [FLINK-2125][streaming] Delimiter change from char to str...
Posted by delding <gi...@git.apache.org>.
Github user delding commented on the issue:
https://github.com/apache/flink/pull/2233
This is my first pull request to Flink. Any comments would be very appreciative.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2233: [FLINK-2125][streaming] Delimiter change from char to str...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2233
ok, go ahead then.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2233: [FLINK-2125][streaming] Delimiter change from char to str...
Posted by delding <gi...@git.apache.org>.
Github user delding commented on the issue:
https://github.com/apache/flink/pull/2233
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---