Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/658#issuecomment-103819433
I think the PR is good to merge.
---
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 StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/658#issuecomment-103883348
Looks good.
Will merge this in my next batch...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/658#issuecomment-101185563
That is a good point. It is a bit tricky, though, since the skip method
does not guarantee efficient skipping. It also reserves the right to skip less
(and return
Github user sekruse commented on the pull request:
https://github.com/apache/flink/pull/658#issuecomment-101215521
I see your point and double-checked it with the Java doc. I will adjust the
seek method accordingly.
---
If your project is set up for it, you can reply to this email
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/658#issuecomment-101024456
Looks good. One comment, though:
- The position tracking in the stream wrapper is a nice idea, but since
the `skip()` method is anyways only a hint (it may skip
Github user sekruse commented on the pull request:
https://github.com/apache/flink/pull/658#issuecomment-101064479
I wanted to integrate the gz support with the decorateStream method,
therefore I was waiting for this PR to be merged.
I can of course skip the seek, but I thought
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/658#issuecomment-101026234
I think Sebastian has already filed a JIRA for adding gz read support.
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/658#discussion_r29920729
--- Diff:
flink-core/src/main/java/org/apache/flink/api/common/io/InputStreamFSInputWrapper.java
---
@@ -0,0 +1,60 @@
+package
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/658#issuecomment-100129770
Thank you for this pull request. I'll review it now.
The first thing I saw is that the build has failed:
```
[INFO]
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/658#issuecomment-100132056
The change looks good, its not completely consistent with our coding
guidelines. But a few runs of mvn install -DskipTests will resolve this ;)
(also, the
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/658#discussion_r29920868
--- Diff:
flink-core/src/test/java/org/apache/flink/api/common/io/FileInputFormatTest.java
---
@@ -18,24 +18,20 @@
package
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/658#discussion_r29920838
--- Diff:
flink-core/src/main/java/org/apache/flink/api/common/io/InputStreamFSInputWrapper.java
---
@@ -0,0 +1,60 @@
+package
Github user sekruse commented on the pull request:
https://github.com/apache/flink/pull/658#issuecomment-100240346
Alright, I incorporated all the feedback. The build seems to be passing now.
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/658#issuecomment-100245298
+1 to merge.
---
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
14 matches
Mail list logo
