Re: [PR] [FLINK-35460][state] Adjust read size when ByteBuffer size is larger than file size for ForSt [flink]
masteryhx closed pull request #24847: [FLINK-35460][state] Adjust read size when ByteBuffer size is larger than file size for ForSt URL: https://github.com/apache/flink/pull/24847 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-35460][state] Adjust read size when ByteBuffer size is larger than file size for ForSt [flink]
zoltar9264 commented on code in PR #24847:
URL: https://github.com/apache/flink/pull/24847#discussion_r1615709313
##
flink-state-backends/flink-statebackend-forst/src/main/java/org/apache/flink/state/forst/fs/ByteBufferReadableFSDataInputStream.java:
##
@@ -99,7 +104,23 @@ public int readFully(ByteBuffer bb) throws IOException {
public int readFully(long position, ByteBuffer bb) throws Exception {
if (bb == null) {
throw new NullPointerException();
-} else if (bb.remaining() == 0) {
+} else if (position >= totalFileSize) {
+throw new IllegalArgumentException(
+String.format(
+"position [%s] is larger or equals than
totalFileSize [%s]",
Review Comment:
```suggestion
"position [%s] is larger than or equals to
totalFileSize [%s]",
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-35460][state] Adjust read size when ByteBuffer size is larger than file size for ForSt [flink]
masteryhx commented on code in PR #24847:
URL: https://github.com/apache/flink/pull/24847#discussion_r1615702417
##
flink-state-backends/flink-statebackend-forst/src/main/java/org/apache/flink/state/forst/fs/ByteBufferReadableFSDataInputStream.java:
##
@@ -99,7 +104,23 @@ public int readFully(ByteBuffer bb) throws IOException {
public int readFully(long position, ByteBuffer bb) throws Exception {
if (bb == null) {
throw new NullPointerException();
-} else if (bb.remaining() == 0) {
+} else if (position > totalFileSize) {
+throw new IllegalArgumentException(
+String.format(
+"position [%s] is larger than totalFileSize [%s]",
+position, totalFileSize));
+}
+
+// Avoid bb.remaining() exceeding the file size limit.
+bb.limit(
Review Comment:
It just follws `ByteBufferReadable` as we set `limit` before calling it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-35460][state] Adjust read size when ByteBuffer size is larger than file size for ForSt [flink]
masteryhx commented on code in PR #24847:
URL: https://github.com/apache/flink/pull/24847#discussion_r1615703735
##
flink-state-backends/flink-statebackend-forst/src/main/java/org/apache/flink/state/forst/fs/ByteBufferReadableFSDataInputStream.java:
##
@@ -99,7 +104,23 @@ public int readFully(ByteBuffer bb) throws IOException {
public int readFully(long position, ByteBuffer bb) throws Exception {
if (bb == null) {
throw new NullPointerException();
-} else if (bb.remaining() == 0) {
+} else if (position > totalFileSize) {
+throw new IllegalArgumentException(
Review Comment:
It just check some conditions before calling `ByteBufferReadable`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-35460][state] Adjust read size when ByteBuffer size is larger than file size for ForSt [flink]
zoltar9264 commented on code in PR #24847:
URL: https://github.com/apache/flink/pull/24847#discussion_r1615683994
##
flink-state-backends/flink-statebackend-forst/src/main/java/org/apache/flink/state/forst/fs/ByteBufferReadableFSDataInputStream.java:
##
@@ -99,7 +104,23 @@ public int readFully(ByteBuffer bb) throws IOException {
public int readFully(long position, ByteBuffer bb) throws Exception {
if (bb == null) {
throw new NullPointerException();
-} else if (bb.remaining() == 0) {
+} else if (position > totalFileSize) {
Review Comment:
It should be ?
position >= totalFileSize
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-35460][state] Adjust read size when ByteBuffer size is larger than file size for ForSt [flink]
zoltar9264 commented on code in PR #24847:
URL: https://github.com/apache/flink/pull/24847#discussion_r1615676559
##
flink-state-backends/flink-statebackend-forst/src/main/java/org/apache/flink/state/forst/fs/ByteBufferReadableFSDataInputStream.java:
##
@@ -99,7 +104,23 @@ public int readFully(ByteBuffer bb) throws IOException {
public int readFully(long position, ByteBuffer bb) throws Exception {
if (bb == null) {
throw new NullPointerException();
-} else if (bb.remaining() == 0) {
+} else if (position > totalFileSize) {
+throw new IllegalArgumentException(
+String.format(
+"position [%s] is larger than totalFileSize [%s]",
+position, totalFileSize));
+}
+
+// Avoid bb.remaining() exceeding the file size limit.
+bb.limit(
Review Comment:
Same to previous comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-35460][state] Adjust read size when ByteBuffer size is larger than file size for ForSt [flink]
zoltar9264 commented on code in PR #24847:
URL: https://github.com/apache/flink/pull/24847#discussion_r1615675293
##
flink-state-backends/flink-statebackend-forst/src/main/java/org/apache/flink/state/forst/fs/ByteBufferReadableFSDataInputStream.java:
##
@@ -99,7 +104,23 @@ public int readFully(ByteBuffer bb) throws IOException {
public int readFully(long position, ByteBuffer bb) throws Exception {
if (bb == null) {
throw new NullPointerException();
-} else if (bb.remaining() == 0) {
+} else if (position > totalFileSize) {
+throw new IllegalArgumentException(
Review Comment:
Sorry I just noticed the changed file `ByteBufferReadableFSDataInputStream`
is not implement `ByteBufferReadable`, ignore this comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-35460][state] Adjust read size when ByteBuffer size is larger than file size for ForSt [flink]
zoltar9264 commented on code in PR #24847:
URL: https://github.com/apache/flink/pull/24847#discussion_r1615605846
##
flink-state-backends/flink-statebackend-forst/src/main/java/org/apache/flink/state/forst/fs/ByteBufferReadableFSDataInputStream.java:
##
@@ -99,7 +104,23 @@ public int readFully(ByteBuffer bb) throws IOException {
public int readFully(long position, ByteBuffer bb) throws Exception {
if (bb == null) {
throw new NullPointerException();
-} else if (bb.remaining() == 0) {
+} else if (position > totalFileSize) {
+throw new IllegalArgumentException(
Review Comment:
The interface doc of ByteBufferReadable say "Implementations should treat
0-length requests as legitimate, and must not signal an error upon their
receipt.", It seems this change brake that ?
##
flink-state-backends/flink-statebackend-forst/src/main/java/org/apache/flink/state/forst/fs/ByteBufferReadableFSDataInputStream.java:
##
@@ -99,7 +104,23 @@ public int readFully(ByteBuffer bb) throws IOException {
public int readFully(long position, ByteBuffer bb) throws Exception {
if (bb == null) {
throw new NullPointerException();
-} else if (bb.remaining() == 0) {
+} else if (position > totalFileSize) {
+throw new IllegalArgumentException(
+String.format(
+"position [%s] is larger than totalFileSize [%s]",
+position, totalFileSize));
+}
+
+// Avoid bb.remaining() exceeding the file size limit.
+bb.limit(
Review Comment:
The interface doc of ByteBufferReadable mention “Callers should use
byteBuffer. limit(...) to control the size of the desired read” and "After a
successful call, byteBuffer. position() will be advanced by the number of bytes
read and byteBuffer. limit() will be unchanged." ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-35460][state] Adjust read size when ByteBuffer size is larger than file size for ForSt [flink]
flinkbot commented on PR #24847: URL: https://github.com/apache/flink/pull/24847#issuecomment-2132753830 ## CI report: * 05d220aee0adf0a608f0f638a940ad6c139ac708 UNKNOWN Bot commands The @flinkbot bot supports the following commands: - `@flinkbot run azure` re-run the last Azure build -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [FLINK-35460][state] Adjust read size when ByteBuffer size is larger than file size for ForSt [flink]
masteryhx opened a new pull request, #24847: URL: https://github.com/apache/flink/pull/24847 ## What is the purpose of the change Adjust read size when ByteBuffer size is larger than file size to avoid bb.remaining() exceeding the file size limit. ## Brief change log - Adjust read size in ByteBufferReadableFSDataInputStream#readFully when ByteBuffer size is larger than file size ## Verifying this change - Add case ForStFlinkFileSystemTest#testReadExceedingFileSize ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): (yes / **no**) - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes / **no**) - The serializers: (yes / no / don't know) - The runtime per-record code paths (performance sensitive): (yes / **no** / don't know) - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / **no** / don't know) - The S3 file system connector: (yes / **no** / don't know) ## Documentation - Does this pull request introduce a new feature? (yes / **no**) - If yes, how is the feature documented? (**not applicable** / docs / JavaDocs / not documented) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
