You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "WencongLiu (via GitHub)" <gi...@apache.org> on 2023/12/19 03:26:47 UTC
[PR] [FLINK-33502][runtime] The preparation step of ScheduledSubpartitionReader should not shut down the process [flink]
WencongLiu opened a new pull request, #23954:
URL: https://github.com/apache/flink/pull/23954
## What is the purpose of the change
*The preparation step of ScheduledSubpartitionReader should not shut down the process.*
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no)
- The serializers: (no)
- The runtime per-record code paths (performance sensitive): (no)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
- The S3 file system connector: (no)
## Documentation
- Does this pull request introduce a new feature? (no)
- If yes, how is the feature documented? (not applicable)
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33502][network] Refactor DiskIOScheduler to handle exceptions more gracefully and improve error handling logic [flink]
Posted by "reswqa (via GitHub)" <gi...@apache.org>.
reswqa commented on code in PR #23954:
URL: https://github.com/apache/flink/pull/23954#discussion_r1433587339
##########
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/hybrid/tiered/tier/disk/DiskIOScheduler.java:
##########
@@ -206,9 +206,9 @@ private int readBuffersFromFile() {
Queue<MemorySegment> buffers;
try {
buffers = allocateBuffers();
- } catch (Exception exception) {
- failScheduledReaders(scheduledReaders, exception);
- LOG.error("Failed to request buffers for data reading.", exception);
+ } catch (Exception e) {
Review Comment:
Could we catch only `TimeoutException` here, other exceptions should be unexpected.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33502][network] Refactor DiskIOScheduler to handle exceptions more gracefully and improve error handling logic [flink]
Posted by "reswqa (via GitHub)" <gi...@apache.org>.
reswqa commented on code in PR #23954:
URL: https://github.com/apache/flink/pull/23954#discussion_r1433628981
##########
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/hybrid/tiered/tier/disk/DiskIOScheduler.java:
##########
@@ -206,9 +206,9 @@ private int readBuffersFromFile() {
Queue<MemorySegment> buffers;
try {
buffers = allocateBuffers();
- } catch (Exception exception) {
- failScheduledReaders(scheduledReaders, exception);
- LOG.error("Failed to request buffers for data reading.", exception);
+ } catch (Exception e) {
Review Comment:
Alright, `BatchShuffleReadBufferPool#requestBuffers()` abuses the declaration of ambiguous exception. It should be limited to it's own home, but this indeed out of the scope of 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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33502][network] Refactor DiskIOScheduler to handle exceptions more gracefully and improve error handling logic [flink]
Posted by "reswqa (via GitHub)" <gi...@apache.org>.
reswqa commented on code in PR #23954:
URL: https://github.com/apache/flink/pull/23954#discussion_r1433628981
##########
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/hybrid/tiered/tier/disk/DiskIOScheduler.java:
##########
@@ -206,9 +206,9 @@ private int readBuffersFromFile() {
Queue<MemorySegment> buffers;
try {
buffers = allocateBuffers();
- } catch (Exception exception) {
- failScheduledReaders(scheduledReaders, exception);
- LOG.error("Failed to request buffers for data reading.", exception);
+ } catch (Exception e) {
Review Comment:
Alright, `BatchShuffleReadBufferPool#requestBuffers()` abuses the declaration of ambiguous exception. It should be limited to it's own home, but this indeed out of the scope of this PR. So let's keep as is.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33502][network] Refactor DiskIOScheduler to handle exceptions more gracefully and improve error handling logic [flink]
Posted by "reswqa (via GitHub)" <gi...@apache.org>.
reswqa merged PR #23954:
URL: https://github.com/apache/flink/pull/23954
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33502][runtime] The preparation step of ScheduledSubpartitionReader should not shut down the process [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #23954:
URL: https://github.com/apache/flink/pull/23954#issuecomment-1862054502
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "ba611f6691b0383e2861828d2e3d968d1d4aa74d",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "ba611f6691b0383e2861828d2e3d968d1d4aa74d",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* ba611f6691b0383e2861828d2e3d968d1d4aa74d UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33502][network] Refactor DiskIOScheduler to handle exceptions more gracefully and improve error handling logic [flink]
Posted by "WencongLiu (via GitHub)" <gi...@apache.org>.
WencongLiu commented on code in PR #23954:
URL: https://github.com/apache/flink/pull/23954#discussion_r1433606853
##########
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/hybrid/tiered/tier/disk/DiskIOScheduler.java:
##########
@@ -206,9 +206,9 @@ private int readBuffersFromFile() {
Queue<MemorySegment> buffers;
try {
buffers = allocateBuffers();
- } catch (Exception exception) {
- failScheduledReaders(scheduledReaders, exception);
- LOG.error("Failed to request buffers for data reading.", exception);
+ } catch (Exception e) {
Review Comment:
The `allocateBuffers()` method will call `BatchShuffleReadBufferPool#requestBuffers()`, which throws `Exception`in the method declaration.
Modifying the `BatchShuffleReadBufferPool#requestBuffers()` is a little unnecessary. Catching the `Exception` is required here.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org