You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by StefanRRichter <gi...@git.apache.org> on 2017/05/30 15:09:38 UTC
[GitHub] flink pull request #4019: [FLINK-6776] [runtime] Use skip instead of seek fo...
GitHub user StefanRRichter opened a pull request:
https://github.com/apache/flink/pull/4019
[FLINK-6776] [runtime] Use skip instead of seek for small forward rep…
…ositioning in DFS streams
Reading checkpoint meta data and finding key-groups in restores sometimes require to seek in input streams. Currently, we always use a seek, even for small position changes. As small true seeks are far more expensive than small reads/skips, we should just skip over small gaps instead of performing the seek.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/StefanRRichter/flink FLINK-6776
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4019.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 #4019
----
commit 6251b2d68fa567bf7b04fca56bd74ef77bcb3114
Author: Stefan Richter <s....@data-artisans.com>
Date: 2017-05-30T15:08:26Z
[FLINK-6776] [runtime] Use skip instead of seek for small forward repositioning in DFS streams
----
---
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 #4019: [FLINK-6776] [runtime] Use skip instead of seek for small...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/4019
Updated with comments.
---
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 #4019: [FLINK-6776] [runtime] Use skip instead of seek fo...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/4019#discussion_r121632647
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/fs/hdfs/HadoopDataInputStream.java ---
@@ -89,4 +99,14 @@ public long skip(long n) throws IOException {
public org.apache.hadoop.fs.FSDataInputStream getHadoopInputStream() {
return fsDataInputStream;
}
+
+ public void forceSeek(long seekPos) throws IOException {
--- End diff --
I agree that doc wouldn't hurt. This class as a whole was rather undocumented, but it is also internal and user will only interact through`FSDataInputStream`, which is not exposing those methods. Can write something anyways :)
---
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 #4019: [FLINK-6776] [runtime] Use skip instead of seek fo...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4019#discussion_r121627666
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/fs/hdfs/HadoopDataInputStream.java ---
@@ -89,4 +99,14 @@ public long skip(long n) throws IOException {
public org.apache.hadoop.fs.FSDataInputStream getHadoopInputStream() {
return fsDataInputStream;
}
+
+ public void forceSeek(long seekPos) throws IOException {
--- End diff --
Java docs missing.
---
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 #4019: [FLINK-6776] [runtime] Use skip instead of seek fo...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter closed the pull request at:
https://github.com/apache/flink/pull/4019
---
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 #4019: [FLINK-6776] [runtime] Use skip instead of seek fo...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4019#discussion_r121631955
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/fs/hdfs/HadoopDataInputStream.java ---
@@ -31,11 +31,15 @@
*/
public final class HadoopDataInputStream extends FSDataInputStream {
+ /** Minimum amount of bytes to skip forward before we issue a seek instead of discarding read */
+ private static final int MIN_SKIP_BYTES = 1024 * 1024;
--- End diff --
This makes sense. Wanna add this as a comment to the code?
---
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 #4019: [FLINK-6776] [runtime] Use skip instead of seek fo...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on a diff in the pull request:
https://github.com/apache/flink/pull/4019#discussion_r121630151
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/fs/hdfs/HadoopDataInputStream.java ---
@@ -31,11 +31,15 @@
*/
public final class HadoopDataInputStream extends FSDataInputStream {
+ /** Minimum amount of bytes to skip forward before we issue a seek instead of discarding read */
+ private static final int MIN_SKIP_BYTES = 1024 * 1024;
--- End diff --
Right now, this is a purely "magic" number. The optimum should depend on the dfs and the underlying fs. For now, this number is chosen "big enough" to provide improvements for smaller seeks, and "small enough" to avoid disadvantages over real seeks. While the minimum should be the page size, a true optimum per system would be the amounts of bytes the can be consumed within seektime. Unfortunately, seektime is not constant and devices as well as dfs potentially also use read buffers and read-ahead. In the long run this value could become configurable, but for now I have simply chosen a conservative, relatively small value that should bring safe improvements for small skips in meta data, that would hurt the most.
---
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 #4019: [FLINK-6776] [runtime] Use skip instead of seek fo...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4019#discussion_r121627538
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/fs/hdfs/HadoopDataInputStream.java ---
@@ -31,11 +31,15 @@
*/
public final class HadoopDataInputStream extends FSDataInputStream {
+ /** Minimum amount of bytes to skip forward before we issue a seek instead of discarding read */
+ private static final int MIN_SKIP_BYTES = 1024 * 1024;
--- End diff --
How have you come up with this number?
---
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 #4019: [FLINK-6776] [runtime] Use skip instead of seek for small...
Posted by StefanRRichter <gi...@git.apache.org>.
Github user StefanRRichter commented on the issue:
https://github.com/apache/flink/pull/4019
Merged in 9141379f6d.
---
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 #4019: [FLINK-6776] [runtime] Use skip instead of seek fo...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4019#discussion_r121627730
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/fs/hdfs/HadoopDataInputStream.java ---
@@ -89,4 +99,14 @@ public long skip(long n) throws IOException {
public org.apache.hadoop.fs.FSDataInputStream getHadoopInputStream() {
return fsDataInputStream;
}
+
+ public void forceSeek(long seekPos) throws IOException {
+ fsDataInputStream.seek(seekPos);
+ }
+
+ public void skipFully(long bytes) throws IOException {
--- End diff --
JavaDocs
---
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.
---