AlexYinHan commented on code in PR #27042:
URL: https://github.com/apache/flink/pull/27042#discussion_r2425200547
##########
flink-state-backends/flink-statebackend-forst/src/main/java/org/apache/flink/state/forst/ForStKeyedStateBackend.java:
##########
@@ -624,12 +620,12 @@ public boolean isSafeToReuseKVState() {
@VisibleForTesting
Path getLocalBasePath() {
- return optionsContainer.getLocalBasePath();
+ return optionsContainer.getPathContainer().getLocalBasePath();
Review Comment:
No. pathContainer is final and it should never be null
##########
flink-state-backends/flink-statebackend-forst/src/test/java/org/apache/flink/state/forst/datatransfer/DataTransferStrategyTest.java:
##########
@@ -524,19 +531,76 @@ void testBuildingStrategyAsExpected() throws IOException {
CopyDataTransferStrategy.class);
testRestoreStrategyAsExpected(
- forStFlinkFileSystem, RecoveryClaimMode.CLAIM,
ReusableDataTransferStrategy.class);
+ forStFlinkFileSystem,
+ "/src-dir",
+ "/dst-dir",
+ RecoveryClaimMode.CLAIM,
+ ReusableDataTransferStrategy.class);
+
+ testRestoreStrategyAsExpected(
+ forStFlinkFileSystem,
+ "/src-dir",
+ "/dst-dir",
+ RecoveryClaimMode.NO_CLAIM,
+ CopyDataTransferStrategy.class);
+
+ testRestoreStrategyAsExpected(
+ forStFlinkFileSystem,
+ "/src-dir",
+ "/dst-dir",
+ RecoveryClaimMode.LEGACY,
+ ReusableDataTransferStrategy.class);
+
+ testRestoreStrategyAsExpected(
+ null,
+ "/src-dir",
+ "/dst-dir",
+ RecoveryClaimMode.CLAIM,
+ CopyDataTransferStrategy.class);
+
+ testRestoreStrategyAsExpected(
+ null,
+ "/src-dir",
+ "/dst-dir",
+ RecoveryClaimMode.NO_CLAIM,
+ CopyDataTransferStrategy.class);
+ // Restoring from the same directory indicates a failover scenario,
allowing us to reuse the
+ // files if we are in a disaggregated setup.
testRestoreStrategyAsExpected(
- forStFlinkFileSystem, RecoveryClaimMode.NO_CLAIM,
CopyDataTransferStrategy.class);
+ forStFlinkFileSystem,
+ "/same-dir",
+ "/same-dir",
+ RecoveryClaimMode.CLAIM,
+ ReusableDataTransferStrategy.class);
testRestoreStrategyAsExpected(
- forStFlinkFileSystem, RecoveryClaimMode.LEGACY,
ReusableDataTransferStrategy.class);
+ forStFlinkFileSystem,
+ "/same-dir",
+ "/same-dir",
+ RecoveryClaimMode.NO_CLAIM,
+ ReusableDataTransferStrategy.class);
testRestoreStrategyAsExpected(
- null, RecoveryClaimMode.CLAIM, CopyDataTransferStrategy.class);
+ forStFlinkFileSystem,
+ "/same-dir",
+ "/same-dir",
+ RecoveryClaimMode.LEGACY,
+ ReusableDataTransferStrategy.class);
testRestoreStrategyAsExpected(
- null, RecoveryClaimMode.NO_CLAIM,
CopyDataTransferStrategy.class);
+ null,
+ "/same-dir",
+ "/same-dir",
+ RecoveryClaimMode.CLAIM,
+ CopyDataTransferStrategy.class);
+
+ testRestoreStrategyAsExpected(
Review Comment:
Yeah i've had a thought of that. However, these test cases have already been
simple enough, and parameterizing them would not reduce the amount of code, but
only make them harder to read.
##########
flink-state-backends/flink-statebackend-forst/src/main/java/org/apache/flink/state/forst/sync/ForStSyncKeyedStateBackend.java:
##########
@@ -488,18 +488,13 @@ public void dispose() {
columnFamilyOptions.forEach(IOUtils::closeQuietly);
LOG.info(
- "Closed ForSt State Backend. Cleaning up ForSt local
working directory {}, remote working directory {}.",
- optionsContainer.getLocalBasePath(),
- optionsContainer.getRemoteBasePath());
+ "Closed ForSt State Backend. Cleaning up ForSt: {}.",
+ optionsContainer.getPathContainer());
try {
optionsContainer.clearDirectories();
} catch (Exception ex) {
- LOG.warn(
- "Could not delete ForSt local working directory {},
remote working directory {}.",
- optionsContainer.getLocalBasePath(),
- optionsContainer.getRemoteBasePath(),
- ex);
+ LOG.warn("Could not delete ForSt: {}.",
optionsContainer.getPathContainer(), ex);
Review Comment:
Actually, 'the actual folder that could not be deleted' should be included
in the ```ex```, so i think there is no info lost 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
