XComp commented on code in PR #23424:
URL: https://github.com/apache/flink/pull/23424#discussion_r1346146391
##########
flink-runtime/src/main/java/org/apache/flink/runtime/highavailability/HighAvailabilityServices.java:
##########
@@ -212,19 +213,53 @@ default LeaderRetrievalService
getClusterRestEndpointLeaderRetriever() {
void close() throws Exception;
/**
- * Closes the high availability services (releasing all resources) and
deletes all data stored
- * by these services in external stores.
+ * Deletes all data stored by high availability services in external
stores.
*
- * <p>After this method was called, the any job or session that was
managed by these high
+ * <p>After this method was called, any job or session that was managed by
these high
* availability services will be unrecoverable.
*
* <p>If an exception occurs during cleanup, this method will attempt to
continue the cleanup
* and report exceptions only after all cleanup steps have been attempted.
*
- * @throws Exception Thrown, if an exception occurred while closing these
services or cleaning
- * up data stored by them.
+ * @throws Exception Thrown, if an exception occurred while cleaning up
data stored by them.
*/
- void closeAndCleanupAllData() throws Exception;
+ void cleanupAllData() throws Exception;
+
+ /**
+ * Closes the high availability services (releasing all resources) and
optionally deletes all
+ * data stored by these services in external stores.
+ *
+ * <p>After this method was called, any job or session that was managed by
these high
+ * availability services will be unrecoverable.
+ *
+ * <p>If an exception occurs during cleanup or close, this method will
attempt to continue the
+ * cleanup or close and report exceptions only after all cleanup and close
steps have been
+ * attempted.
+ *
+ * @param cleanupData Whether cleanup of data stored by the high
availability services should be
+ * attempted.
+ * @throws Exception Thrown, if an exception occurred while cleaning up
data stored by the high
+ * availability services.
Review Comment:
```suggestion
* Calls {@link #cleanupAllData()} (if {@code true} is passed as a
parameter) before calling {@link #close()} on this instance. Any error that
appeared during the cleanup will be propagated after calling {@code close()}.
```
nit: I'm not a big fan of duplicating documentation because it makes
maintaining the documentation harder. What do you think of the proposal above?
##########
flink-runtime/src/main/java/org/apache/flink/runtime/highavailability/HighAvailabilityServices.java:
##########
@@ -212,19 +213,53 @@ default LeaderRetrievalService
getClusterRestEndpointLeaderRetriever() {
void close() throws Exception;
/**
- * Closes the high availability services (releasing all resources) and
deletes all data stored
- * by these services in external stores.
+ * Deletes all data stored by high availability services in external
stores.
*
- * <p>After this method was called, the any job or session that was
managed by these high
+ * <p>After this method was called, any job or session that was managed by
these high
* availability services will be unrecoverable.
*
* <p>If an exception occurs during cleanup, this method will attempt to
continue the cleanup
* and report exceptions only after all cleanup steps have been attempted.
*
- * @throws Exception Thrown, if an exception occurred while closing these
services or cleaning
- * up data stored by them.
+ * @throws Exception Thrown, if an exception occurred while cleaning up
data stored by them.
Review Comment:
```suggestion
* @throws Exception if an error occurred while cleaning up data stored
by them.
```
nit: just to fix using "throw" twice
##########
flink-runtime/src/test/java/org/apache/flink/runtime/highavailability/AbstractHaServicesTest.java:
##########
@@ -97,7 +98,8 @@ void
testCloseAndCleanupAllDataDoesNotDeleteBlobsIfCleaningUpHADataFails() throw
},
ignored -> {});
-
assertThatThrownBy(haServices::closeAndCleanupAllData).isInstanceOf(FlinkException.class);
+
assertThatThrownBy(haServices::cleanupAllData).isInstanceOf(FlinkException.class);
+ haServices.close();
Review Comment:
```suggestion
assertThatThrownBy(() ->
haServices.closeWithOptionalCleanup(true)).isInstanceOf(FlinkException.class);
```
This improves the test coverage (i.e. it would check the newly added default
implementation).
##########
flink-runtime/src/test/java/org/apache/flink/runtime/highavailability/AbstractHaServicesTest.java:
##########
@@ -66,13 +66,14 @@ void
testCloseAndCleanupAllDataDeletesBlobsAfterCleaningUpHAData() throws Except
() ->
closeOperations.offer(CloseOperations.HA_CLEANUP),
ignored -> {});
- haServices.closeAndCleanupAllData();
+ haServices.cleanupAllData();
+ haServices.close();
Review Comment:
```suggestion
haServices.closeWithOptionalCleanup(true);
```
This improves the test coverage (i.e. it would check the newly added default
implementation).
##########
flink-runtime/src/test/java/org/apache/flink/runtime/highavailability/nonha/embedded/EmbeddedHaServicesTest.java:
##########
@@ -57,7 +57,8 @@ public void setupTest() {
@After
public void teardownTest() throws Exception {
if (embeddedHaServices != null) {
- embeddedHaServices.closeAndCleanupAllData();
+ embeddedHaServices.cleanupAllData();
+ embeddedHaServices.close();
Review Comment:
```suggestion
embeddedHaServices.closeAndOptionalCleanup(true);
```
nit: This is up to you. Calling the newly added default implementation (here
and in the other tests) would make the diff smaller.
--
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
