[FLINK-2125][streaming] Delimiter change from char to string #1077
Conversation
| SocketTextStreamFunction source = new SocketTextStreamFunction("", 0, '\n', 0); | ||
| Field field = SocketTextStreamFunction.class.getDeclaredField("isRunning"); | ||
| field.setAccessible(true); | ||
| field.set(source, true); |
There was a problem hiding this comment.
I do not think it is good to use reflection since I did something the same as you before and they told don`t do this.
Why not set up a socket server in your test?
|
Hi, |
|
I changed unit tests as HuangWHWHW suggested, added a simple test server socket and refactored test methods. |
| private static final String[] fakeServerResponses = content.split("\\."); | ||
|
|
||
| private class TestServer implements Runnable{ | ||
| private int port = 44444; |
There was a problem hiding this comment.
It is not suitable to use a static port since sometimes it will get failed if this port is already used.
You can call serverSocket = new ServerSocket(0); to set up a socket server with a free port.
|
I changed static port and added assertions for full content |
|
Hi, sorry for reply late. |
|
@ogokal, please ping me when you have rebased and updated the PR and I will have a look. |
| } else if (data != '\r') { // ignore carriage return | ||
| buffer.append((char) data); | ||
| buffer.append(charBuffer, 0, readCount); | ||
| String[] splits = buffer.toString().split(delimiter); |
There was a problem hiding this comment.
String.split() and String.replace() create new String objects which must be garbage collected. This adds quite some overhead, because these functions are called very often. The previous implementation was operating on a byte-level and avoiding the creation of new objects. It would be good if we could preserve this behavior.
|
Hi @ogokal, do you plan to update this PR? If not, could you please close it? Thank you. |
|
OK, I will close this PR for now. |
This closes apache#1247 -- PRs closed due to inactivity This closes apache#1077
This closes apache#1247 -- PRs closed due to inactivity This closes apache#1077
I tried to change based on the previous comments. I hope it is sufficient enough.