You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/11/20 13:48:16 UTC
[GitHub] [flink] 1996fanrui opened a new pull request, #21353: [FLINK-30100][checkpointing] Remove the unused CheckpointFailureReason
1996fanrui opened a new pull request, #21353:
URL: https://github.com/apache/flink/pull/21353
## What is the purpose of the change
Some CheckpointFailureReasons have not been used for a long time and should be removed. E.g:
- TOO_MANY_CONCURRENT_CHECKPOINTS
- CHECKPOINT_DECLINED_TASK_NOT_CHECKPOINTING
- CHECKPOINT_DECLINED_ALIGNMENT_LIMIT_EXCEEDED
- JOB_FAILURE
## Brief change log
- The first commit: Remove some unused exceptions and refactor some code of CheckpointCoordinatorTest
- The second commit: Remove deprecated getCheckpointId of PendingCheckpoint
- The third commit: Remove the unused CheckpointFailureReason
## Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
## 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? (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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] 1996fanrui commented on pull request #21353: [FLINK-30100][checkpointing] Remove the unused CheckpointFailureReason
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on PR #21353:
URL: https://github.com/apache/flink/pull/21353#issuecomment-1327485152
@pnowojski Thanks for your review!
--
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
[GitHub] [flink] 1996fanrui commented on pull request #21353: [FLINK-30100][checkpointing] Remove the unused CheckpointFailureReason
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on PR #21353:
URL: https://github.com/apache/flink/pull/21353#issuecomment-1321952773
Hi @pnowojski please help take a look, thanks. It's also a cleanup 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
[GitHub] [flink] 1996fanrui commented on a diff in pull request #21353: [FLINK-30100][checkpointing] Remove the unused CheckpointFailureReason
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on code in PR #21353:
URL: https://github.com/apache/flink/pull/21353#discussion_r1030484528
##########
flink-runtime/src/test/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinatorTest.java:
##########
@@ -3837,14 +3829,6 @@ public void testBaseLocationsNotInitialized() throws Exception {
.addJobVertex(jobVertexID)
.setTransitToRunning(false)
.build(EXECUTOR_RESOURCE.getExecutor());
- CheckpointCoordinator checkpointCoordinator =
- new CheckpointCoordinatorBuilder()
- .setCheckpointCoordinatorConfiguration(
- CheckpointCoordinatorConfiguration.builder()
- .setCheckpointInterval(Long.MAX_VALUE)
- .build())
- .setCheckpointStorage(new FsStateBackend(checkpointDir.toURI()))
- .build(graph);
Review Comment:
You are right, It's my mistake. I see this checkpointCoordinator isn't used, so removed it.
For the convenience of review, friendly reminder:
- It's recovered in the first commit, and I removed the `CheckpointCoordinator checkpointCoordinator =` due to it's not necessary.
- I did some extra work in this PR during fix this comment. I migrate checkpoint coordinator related tests to junit5 in the 3rd commit.
- The second and fourth commits are not updated.
--
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
[GitHub] [flink] pnowojski merged pull request #21353: [FLINK-30100][checkpointing] Remove the unused CheckpointFailureReason
Posted by GitBox <gi...@apache.org>.
pnowojski merged PR #21353:
URL: https://github.com/apache/flink/pull/21353
--
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
[GitHub] [flink] pnowojski commented on a diff in pull request #21353: [FLINK-30100][checkpointing] Remove the unused CheckpointFailureReason
Posted by GitBox <gi...@apache.org>.
pnowojski commented on code in PR #21353:
URL: https://github.com/apache/flink/pull/21353#discussion_r1030298472
##########
flink-runtime/src/test/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinatorTest.java:
##########
@@ -3837,14 +3829,6 @@ public void testBaseLocationsNotInitialized() throws Exception {
.addJobVertex(jobVertexID)
.setTransitToRunning(false)
.build(EXECUTOR_RESOURCE.getExecutor());
- CheckpointCoordinator checkpointCoordinator =
- new CheckpointCoordinatorBuilder()
- .setCheckpointCoordinatorConfiguration(
- CheckpointCoordinatorConfiguration.builder()
- .setCheckpointInterval(Long.MAX_VALUE)
- .build())
- .setCheckpointStorage(new FsStateBackend(checkpointDir.toURI()))
- .build(graph);
Review Comment:
I'm not sure but it looks like intention of this code was to start `CheckpointCoordinator` and check if the directory has not been created. So after removing of those lines this test becomes a no-op
--
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
[GitHub] [flink] flinkbot commented on pull request #21353: [FLINK-30100][checkpointing] Remove the unused CheckpointFailureReason
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #21353:
URL: https://github.com/apache/flink/pull/21353#issuecomment-1321139432
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e78d6e4e1695ef569198892fee7198479d95425c",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "e78d6e4e1695ef569198892fee7198479d95425c",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e78d6e4e1695ef569198892fee7198479d95425c 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
[GitHub] [flink] 1996fanrui commented on a diff in pull request #21353: [FLINK-30100][checkpointing] Remove the unused CheckpointFailureReason
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on code in PR #21353:
URL: https://github.com/apache/flink/pull/21353#discussion_r1030484528
##########
flink-runtime/src/test/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinatorTest.java:
##########
@@ -3837,14 +3829,6 @@ public void testBaseLocationsNotInitialized() throws Exception {
.addJobVertex(jobVertexID)
.setTransitToRunning(false)
.build(EXECUTOR_RESOURCE.getExecutor());
- CheckpointCoordinator checkpointCoordinator =
- new CheckpointCoordinatorBuilder()
- .setCheckpointCoordinatorConfiguration(
- CheckpointCoordinatorConfiguration.builder()
- .setCheckpointInterval(Long.MAX_VALUE)
- .build())
- .setCheckpointStorage(new FsStateBackend(checkpointDir.toURI()))
- .build(graph);
Review Comment:
You are right, It's my mistake. I see this checkpointCoordinator isn't used, so removed it.
It's covered, and I removed the `CheckpointCoordinator checkpointCoordinator =` due to it's not necessary.
BTW, I did some extra work in this PR during fix this comment.
I migrate checkpoint coordinator related tests to junit5 in the 3rd commit.
--
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