[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/1073 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user HuangWHWHW commented on the pull request: https://github.com/apache/flink/pull/1073#issuecomment-138547161 Ah, thank you. I'll force push again. Sorry for making you wait 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 not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user mxm commented on the pull request: https://github.com/apache/flink/pull/1073#issuecomment-138544981 The test failures are unrelated to your changes. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user HuangWHWHW commented on the pull request: https://github.com/apache/flink/pull/1073#issuecomment-138511026 @mxm Hi, can you see why the CI failed? I build success in local. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user HuangWHWHW commented on the pull request: https://github.com/apache/flink/pull/1073#issuecomment-138495322 Many thanks for your time! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user mxm commented on the pull request: https://github.com/apache/flink/pull/1073#issuecomment-138494607 Looks good. Will merge this later on. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user HuangWHWHW commented on the pull request: https://github.com/apache/flink/pull/1073#issuecomment-138491813 @mxm Hi, added the System.lineSeparator(). --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user HuangWHWHW commented on the pull request: https://github.com/apache/flink/pull/1073#issuecomment-138487709 Ah, smart! Didn't notice it before. I'll 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 this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user mxm commented on the pull request: https://github.com/apache/flink/pull/1073#issuecomment-138486624 @HuangWHWHW You can use `System.lineSeparator()` to get the either `\n` or `\r\n` depending on the operating system. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user HuangWHWHW commented on the pull request: https://github.com/apache/flink/pull/1073#issuecomment-137746778 @mxm Hi, I change the "\r\n" to "\n" since it use println in PrintSinkFunction.invoke(); --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/1073#discussion_r38621439
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/PrintSinkFunctionTest.java
---
@@ -97,20 +76,43 @@ public void testPrintSinkStdErr(){
try {
printSink.open(new Configuration());
} catch (Exception e) {
- e.printStackTrace();
+ Assert.fail();
}
printSink.setTargetToStandardErr();
printSink.invoke("hello world!");
assertEquals("Print to System.err", printSink.toString());
- assertEquals("hello world!", stream.result);
+ assertEquals("hello world!\r\n", baos.toString());
printSink.close();
+ stream.close();
}
- @Override
- public void invoke(IN record) {
+ @Test
+ public void testPrintSinkWithPrefix(){
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ PrintStream stream = new PrintStream(baos);
+ System.setOut(stream);
+
+ final StreamingRuntimeContext ctx =
Mockito.mock(StreamingRuntimeContext.class);
+ Mockito.when(ctx.getNumberOfParallelSubtasks()).thenReturn(2);
+ Mockito.when(ctx.getIndexOfThisSubtask()).thenReturn(1);
+ PrintSinkFunction printSink = new PrintSinkFunction<>();
+ printSink.setRuntimeContext(ctx);
+ try {
+ printSink.open(new Configuration());
+ } catch (Exception e) {
+ Assert.fail();
+ }
+ printSink.setTargetToStandardErr();
+ printSink.invoke("hello world!");
+
+ assertEquals("Print to System.err", printSink.toString());
+ assertEquals("2> hello world!\r\n", baos.toString());
--- End diff --
Same compatibility issue here.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/1073#discussion_r38621376
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/PrintSinkFunctionTest.java
---
@@ -73,21 +51,22 @@ public void testPrintSinkStdOut(){
try {
printSink.open(new Configuration());
} catch (Exception e) {
- e.printStackTrace();
+ Assert.fail();
}
printSink.setTargetToStandardOut();
printSink.invoke("hello world!");
assertEquals("Print to System.out", printSink.toString());
- assertEquals("hello world!", stream.result);
+ assertEquals("hello world!\r\n", baos.toString());
--- End diff --
This will work on Windows (`\r\n` for newline) but not on Unix systems
(`\n` for newline).
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/1073#discussion_r38621410
--- Diff:
flink-staging/flink-streaming/flink-streaming-core/src/test/java/org/apache/flink/streaming/api/functions/PrintSinkFunctionTest.java
---
@@ -97,20 +76,43 @@ public void testPrintSinkStdErr(){
try {
printSink.open(new Configuration());
} catch (Exception e) {
- e.printStackTrace();
+ Assert.fail();
}
printSink.setTargetToStandardErr();
printSink.invoke("hello world!");
assertEquals("Print to System.err", printSink.toString());
- assertEquals("hello world!", stream.result);
+ assertEquals("hello world!\r\n", baos.toString());
--- End diff --
Same compatibility issue here.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user HuangWHWHW commented on the pull request: https://github.com/apache/flink/pull/1073#issuecomment-137289902 @mxm @StephanEwen Hi,I squashed commits into one but it seems that the CI doesn't run. Any other way to do? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user HuangWHWHW commented on the pull request: https://github.com/apache/flink/pull/1073#issuecomment-136918945 @mxm @StephanEwen Hi, I fix the PrintStream. And if I use the ByteArrayOutputStream baos = new ByteArrayOutputStream();, it will append "\r\n" in the end of print. Does it matter? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user HuangWHWHW commented on the pull request: https://github.com/apache/flink/pull/1073#issuecomment-136916269 @mxm Much thanks! I`ll take a fix. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/1073#issuecomment-136758616
@HuangWHWHW Stephan is talking about something like this instead of the
PrintStreamMock:
```java
ByteArrayOutputStream baos = new ByteArrayOutputStream();
PrintStream captureStream = new PrintStream(baos);
PrintStream original = System.out;
System.setOut(captureStream);
System.out.println("Printing one line");
System.out.println("Another line");
System.setOut(original);
captureStream.close();
Assert.equals("Printing one line\nAnotherline\n", baos.toString());
```
You can see that we're using a `PrintStream` with a `ByteArrayOutputStream`
here to capture the contents that are being printed to standard out.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user HuangWHWHW commented on the pull request: https://github.com/apache/flink/pull/1073#issuecomment-136694516 @StephanEwen Hi, I take changes for all of your comments but this following: "The mock print stream overwrites only one println() function, which makes it susceptible to changes in the sink. It is better to use a regular PrintStream that prints to a StringWriter to collect all contents." I have no ideas to assert the print is correct. So can you tell me in detail? Thank you very much! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user HuangWHWHW commented on the pull request: https://github.com/apache/flink/pull/1073#issuecomment-136686857 ·The mock print stream overwrites only one println() function, which makes it susceptible to changes in the sink. It is better to use a regular PrintStream that prints to a StringWriter to collect all contents.· Hi @StephanEwen If I change this how can I get the printed message? Since I use System.setOut(stream); to solve this 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user HuangWHWHW commented on the pull request: https://github.com/apache/flink/pull/1073#issuecomment-136677416 @StephanEwen Yes, thank you for the comments very much. I`ll take the fix. BTW:There are two PRs of mine that need someone to give some comments: https://github.com/apache/flink/pull/1030 https://github.com/apache/flink/pull/992 Could you take a look? Very sorry if it will spend your time. Thanks a lot! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user StephanEwen commented on the pull request: https://github.com/apache/flink/pull/1073#issuecomment-136667496 @HuangWHWHW I think this test and also the class `PrintSinkFunctionTest` needs some more improvement. Here are a few comments: - Exception during `open()` should not be caught, but should fail the test. This happens at multiple points in the code. - The test contains a class that starts with a lower case letter, which is against the Java style rules. - The test as a whole extends `RichSinkFunction` which is not necessary and seems strange. - The mock print stream overwrites only one `println()` function, which makes it susceptible to changes in the sink. It is better to use a regular `PrintStream` that prints to a `StringWriter` to collect all contents. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user HuangWHWHW commented on the pull request: https://github.com/apache/flink/pull/1073#issuecomment-136361760 @mxm Thank you! BTW:can you take a look about this PR:https://github.com/apache/flink/pull/992? I add a new test in that PR and I`m not sure is it you need? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
Github user mxm commented on the pull request: https://github.com/apache/flink/pull/1073#issuecomment-136357113 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2480][test]add a test for Print Sink wi...
GitHub user HuangWHWHW opened a pull request: https://github.com/apache/flink/pull/1073 [FLINK-2480][test]add a test for Print Sink with prefix I check the coverage of sink and add a test for sink to print with prefix. You can merge this pull request into a Git repository by running: $ git pull https://github.com/HuangWHWHW/flink FLINK-2480-new Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/1073.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 #1073 commit 13ac2aee58c6befbd79532a8a79647014501ea04 Author: HuangWHWHW <404823...@qq.com> Date: 2015-08-29T06:51:02Z [FLINK-2480][test]add a test for Print Sink with prefix --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
