Github user HuangWHWHW commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r38906129
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r38909392
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
---
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-138518846
@mxm
Hi, I added the ArgumentCaptor to the test and removed the unwanted code.
---
If your project is set up for it, you can reply to this email and have your
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-138608134
Nice :) 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
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/992
---
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
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-138268476
@mxm
@StephanEwen
Hi, very sorry for bothering.
I got the CI passed.
Is there any new comment or this can be merged?
---
If your project is set up
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-137745160
@mxm
Ok, I make the CI rerun.
Any new comment?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well.
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-137372443
I think there is an issue with Travis at the moment. Could you force push
to this branch again?
---
If your project is set up for it, you can reply to this email and have
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-136910004
@mxm @StephanEwen
Hi, I just update one commit yesterday.
And I found that a few PRs got the same trouble yesterday.
Is there any issue in CI?
---
If
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r38409485
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/main/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunction.java
---
Github user HuangWHWHW commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r38414236
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/main/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunction.java
---
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-136754340
@HuangWHWHW Thank you for addressing my comments. Could you please squash
your commits and force push to this branch again?
---
If your project is set up for it, you can
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-136739055
I am not sure why the CI is not retesting this.
Can you try to squash your commits into one commit and force-push this
branch? This always triggers CI for
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-136285250
@mxm
Hi, max.
Any comment about my new changes?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-135429969
@mxm
Hi,
I add a test that first send a message to the SocketTextStreamFunction and
this is success.
Then I close the server let the
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-135351424
While merging your pull request I noticed that the
`SocketTextStreamFunction` actually does not wait the time specified in
`CONNCTION_RETRY_SLEEP` but immediately tries to
Github user HuangWHWHW commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37963338
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37959211
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/main/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunction.java
---
@@
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-134902558
@HuangWHWHW Thanks for your changes. Adding reflection calls to the testing
codes is not good practice and makes the code hard to maintain.
---
If your project is set up
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37959366
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
---
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-135262313
Hi,@mxm
any new 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
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37879401
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/main/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunction.java
---
@@
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-134797100
@mxm
Hi,I chage the CONNECTION_RETRY_SLEEP to static final int
CONNECTION_RETRY_SLEEP = 1000;
But I have no idea to straightly changing the
Github user HuangWHWHW commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37843364
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
Github user HuangWHWHW commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37845490
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-134526309
Hi, I take a new change that decrease the sleep time and clear the error
for each test cases.
But I have no idea to control the value of retry to increase.
---
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37844142
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
---
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37856724
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
---
Github user HuangWHWHW commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37937505
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/main/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunction.java
---
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-134493928
@mxm
It doesn`t matter.
I`ll take a new change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user HuangWHWHW commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37861790
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-134570319
Hi, I decreased both the waiting time and the retry times since it will
still cost over 10 seconds if only the waiting time is decreased due to the
Github user HuangWHWHW commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37860526
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37860912
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
---
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-134567337
Looks good to merge if we further adjust the waiting time of the tests.
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user HuangWHWHW commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37858063
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37859069
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
---
Github user HuangWHWHW commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37860800
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
Github user HuangWHWHW commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37864215
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37748868
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
---
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37749287
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
---
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-134199343
@HuangWHWHW Sorry for keeping you waiting. I've made some more comments.
Otherwise, I think this looks ready to be merged.
---
If your project is set up for it, you can
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37749598
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
---
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37748844
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
---
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37748808
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
---
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37762121
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-132982117
Hi Max,
I am very sorry to bothered you.
I fixed some of my PRs and was waiting for your reply for days.
Otherwise, as your advice, I added a change about
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-133238272
@tillrohrmann
Thank you for the info :-)
---
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
Github user tillrohrmann commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-132999197
@HuangWHWHW, Max is currently on vacations. He'll be back next week. I'm
sure that he'll get back to you then :-)
---
If your project is set up for it, you can
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-130999638
Hi , I did a new change.
---
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
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r36955259
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/main/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunction.java
---
@@
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r36955189
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/main/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunction.java
---
@@
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r36955316
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/main/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunction.java
---
@@
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r36955334
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/main/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunction.java
---
@@
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r36955221
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/main/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunction.java
---
@@
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-130644950
`StringBuilder` is only for single-threaded while `StringBuffer` enables
multi-thread access. If you use `StringBuffer` in a single-threaded scenario it
has worse
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-130588652
Thanks for your changes. I think we should use `read()` instead of
`readLine()` because we are using a custom delimiter and not necessarily \n
(newline symbol). The danger
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-130524804
@mxm
Hi, I fixed the StringBuffer and add the test.
Take a look whether it`s correct.
Thank you!
---
If your project is set up for it, you can reply to
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-130626843
Hi Max,
I fixed all as your reviews.
And I retained the change of StringBuffer to StringBuilder.
There is a question that as I see the StringBuilder just do
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-130664351
@mxm
Otherwise, I found the SocketClientSink didn`t have the retry.
Is it necessary to get a retry?
---
If your project is set up for it, you can reply to
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r36973279
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
---
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r36973254
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunctionTest.java
---
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r36973430
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/main/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunction.java
---
@@
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r36973377
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/main/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunction.java
---
@@
Github user HuangWHWHW commented on a diff in the pull request:
https://github.com/apache/flink/pull/992#discussion_r37042816
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/main/java/org/apache/flink/streaming/api/functions/source/SocketTextStreamFunction.java
---
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-130663511
Ok, I add two cases(retry 10 and 0) since I thought retry 1 time just same
as 10.
And would you please take a look with another two
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-130672880
Otherwise, I found the SocketClientSink didn`t have the retry.
Is it necessary to get a retry?
Yes, that might be an issue but let's keep it separate from our
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-130217420
@mxm
Hi, thank you for suggestions.
I will try to follow your suggestions and improve the test.
I understand almost of yours and I also read the Class
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-130213092
@HuangWHWHW `read()` method of the `BufferedReader` object returns `-1` in
case the end of the stream has been reached.
A couple of things I noticed apart from the
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-130222078
Actually point 3 is not so bad because we're using a buffered reader that
fills the buffer and does not read a character from the socket on every call to
`read()`.
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-130206658
@mxm
@StephanEwen
Hi, I do a test for this today and I got another problem.
The SocketTextStreamFunction use BufferedReader.read() to get the buffer
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-130228505
Hi, there are two more questions:
1.In using StringBuilder, does it mean that we should use
BufferedReader.readLine() instead of BufferedReader.read()?
2.Could
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-130237642
1.In using StringBuilder, does it mean that we should use
BufferedReader.readLine() instead of BufferedReader.read()?
Reading by character is the way to go if we
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-130311909
Thank you!
I`ll try again.
---
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
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-129864531
@HuangWHWHW `retryForever` is just a convenience variable for `maxRetry
0`. Your fix is correct because the loop will only execute if `maxRetry 0`
and thus not execute at
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-129875331
@mxm
Ok, I`ll add a test.
There is a little difficult that I can`t get the retry times in test since
the retry is a local variable.
So can I add a
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-129571942
This fix looks valid.
Can it be included in an extended test for the socket function? Something
that validates that the function properly tries to reconnect?
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-129667322
@StephanEwen
Hi,yes,I plan to add a test for it.
However, the test may be failed since the retryForever in the flink-master
is also unworked currently.
Github user zentol commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-128309202
If you think it was necessary why was your first step to remove it's
usage...
---
If your project is set up for it, you can reply to this email and have your
reply
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-128322151
Hah
Sorry, this thought was generated after this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
GitHub user HuangWHWHW opened a pull request:
https://github.com/apache/flink/pull/992
[FLINK-2490][FIX]Remove the retryForever check in function streamFromâ¦
In the class SocketTextStreamFunction, the var retryForever only be set in
the line this.retryForever = maxRetry
Github user HuangWHWHW commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-128307751
Yes, I understand you.
But I think the retryForever is necessary.
Maybe there is a bug that make the retryForever not working.
I`ll get another fix after
Github user zentol commented on the pull request:
https://github.com/apache/flink/pull/992#issuecomment-128301542
if you remove that check, retryForever is unused and can be removed
completely.
---
If your project is set up for it, you can reply to this email and have your
reply
83 matches
Mail list logo
