You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2024/02/08 08:03:50 UTC
[PR] [WIP][CORE] Refactor `refill()` method to `isExhausted()` in `NioBufferedFileInputStream` [spark]
LuciferYang opened a new pull request, #45068:
URL: https://github.com/apache/spark/pull/45068
### What changes were proposed in this pull request?
The proposed changes in this pr involve refactoring the `refill()` method in the `NioBufferedFileInputStream` class to `isExhausted()`. All calls to `!refill()` within the class have been updated to call `isExhausted()` instead.
### Why are the changes needed?
The changes are needed to improve the readability and understandability of the code. Currently, the `refill()` method in `NioBufferedFileInputStream` is always invoked in a negated context (`!refill()`), which can be confusing and counter-intuitive.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Pass GitHub Actions
### Was this patch authored or co-authored using generative AI tooling?
No
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47006][CORE] Refactor `refill()` method to `isExhausted()` in `NioBufferedFileInputStream` [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang closed pull request #45068: [SPARK-47006][CORE] Refactor `refill()` method to `isExhausted()` in `NioBufferedFileInputStream`
URL: https://github.com/apache/spark/pull/45068
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47006][CORE] Refactor `refill()` method to `isExhausted()` in `NioBufferedFileInputStream` [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #45068:
URL: https://github.com/apache/spark/pull/45068#issuecomment-1936813946
Close this one
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][CORE] Refactor `refill()` method to `isExhausted()` in `NioBufferedFileInputStream` [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #45068:
URL: https://github.com/apache/spark/pull/45068#issuecomment-1933543961
Test first
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47006][CORE] Refactor `refill()` method to `isExhausted()` in `NioBufferedFileInputStream` [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #45068:
URL: https://github.com/apache/spark/pull/45068#discussion_r1484198113
##########
core/src/main/java/org/apache/spark/io/NioBufferedFileInputStream.java:
##########
@@ -66,32 +66,30 @@ private boolean refill() throws IOException {
nRead = fileChannel.read(byteBuffer);
}
byteBuffer.flip();
- if (nRead < 0) {
- return false;
- }
+ return nRead >= 0;
}
return true;
}
@Override
public synchronized int read() throws IOException {
- if (!refill()) {
- return -1;
+ if (refill()) {
Review Comment:
@mridulm I agree with your viewpoint. Currently, I have transformed the code from checking `!refill()` to checking `refill()`, and adjusted the corresponding handling logic (I have not yet updated the PR description). Do you consider this to be acceptable? If not, I will close this PR.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org