[
https://issues.apache.org/jira/browse/IGNITE-28870?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18095669#comment-18095669
]
Ignite TC Bot commented on IGNITE-28870:
----------------------------------------
{panel:title=Branch: [pull/13358/head] Base: [master] : No blockers
found!|borderStyle=dashed|borderColor=#ccc|titleBGColor=#D6F7C1}{panel}
{panel:title=Branch: [pull/13358/head] Base: [master] : New Tests
(8)|borderStyle=dashed|borderColor=#ccc|titleBGColor=#D6F7C1}
{color:#00008b}Snapshots 7{color} [[tests
8|https://ci2.ignite.apache.org/viewLog.html?buildId=9201595]]
* {color:#013220}IgniteSnapshotTestSuite7:
IgniteSnapshotManagerSelfTest.testSnapshotTmpRootIsRemovedIfNotEmptyOnError[encryption=false,
onlyPrimay=true] - PASSED{color}
* {color:#013220}IgniteSnapshotTestSuite7:
IgniteSnapshotManagerSelfTest.testSnapshotTmpRootIsNotRemovedIfNotEmptyOnSuccess[encryption=false,
onlyPrimay=true] - PASSED{color}
* {color:#013220}IgniteSnapshotTestSuite7:
IgniteSnapshotManagerSelfTest.testSnapshotTmpRootIsRemovedIfNotEmptyOnError[encryption=false,
onlyPrimay=false] - PASSED{color}
* {color:#013220}IgniteSnapshotTestSuite7:
IgniteSnapshotManagerSelfTest.testSnapshotTmpRootIsNotRemovedIfNotEmptyOnSuccess[encryption=false,
onlyPrimay=false] - PASSED{color}
* {color:#013220}IgniteSnapshotTestSuite7:
IgniteSnapshotManagerSelfTest.testSnapshotTmpRootIsRemovedIfNotEmptyOnError[encryption=true,
onlyPrimay=true] - PASSED{color}
* {color:#013220}IgniteSnapshotTestSuite7:
IgniteSnapshotManagerSelfTest.testSnapshotTmpRootIsNotRemovedIfNotEmptyOnSuccess[encryption=true,
onlyPrimay=true] - PASSED{color}
* {color:#013220}IgniteSnapshotTestSuite7:
IgniteSnapshotManagerSelfTest.testSnapshotTmpRootIsRemovedIfNotEmptyOnError[encryption=true,
onlyPrimay=false] - PASSED{color}
* {color:#013220}IgniteSnapshotTestSuite7:
IgniteSnapshotManagerSelfTest.testSnapshotTmpRootIsNotRemovedIfNotEmptyOnSuccess[encryption=true,
onlyPrimay=false] - PASSED{color}
{panel}
[TeamCity *--> Run :: All*
Results|https://ci2.ignite.apache.org/viewLog.html?buildId=9201603&buildTypeId=IgniteTests24Java8_RunAll]
{color:#ffffff}tcbot-analysis-comment chainBuildId=9201603
rerunBuildIds=9202192{color}
> Restore snapshot temporary files cleanup logic
> ----------------------------------------------
>
> Key: IGNITE-28870
> URL: https://issues.apache.org/jira/browse/IGNITE-28870
> Project: Ignite
> Issue Type: Bug
> Reporter: Oleg Valuyskiy
> Assignee: Oleg Valuyskiy
> Priority: Minor
> Labels: ise
> Attachments: SnapshotTmpDirCleanupReproducer.patch
>
> Time Spent: 50m
> Remaining Estimate: 0h
>
> h3. Problem
> After the changes introduced in IGNITE-24130 / PR #11896, the temporary
> snapshot files cleanup logic was accidentally changed. Previously, snapshot
> cleanup used two different directories:
> {code:java}
> U.delete(sft.tempFileTree().nodeStorage());
> // Delete snapshot directory if no other files exists.
> try {
> if (U.fileCount(sft.tempFileTree().root().toPath()) == 0 || err != null)
> U.delete(sft.tempFileTree().root().toPath());
> }
> catch (IOException e) {
> log.error("Snapshot directory doesn't exist [snpName=" + snpName + ",
> dir=" + sft.tempFileTree().root() + ']');
> }
> {code}
> These two directories represent different levels of the temporary snapshot
> file tree. For example:
> * sft.tempFileTree().root() -
> <work_dir>/db/<node_storage>/snp/<snapshot_name>
> * sft.tempFileTree().nodeStorage() -
> <work_dir>/db/<node_storage>/snp/<snapshot_name>/db/<node_storage>
> So the original cleanup logic was:
> * remove the node-specific temporary snapshot storage directory
> * then check the temporary root directory of the current snapshot
> * remove the root directory only if it becomes empty
> * remove the root directory regardless of its contents if snapshot
> processing finished with an error, because the temporary state of the current
> snapshot is considered invalid
> In other words, the original logic intentionally worked with two different
> filesystem levels.
> h3. Regression
> After IGNITE-24130 / PR #11896, the cleanup logic was refactored into a
> helper method:
> {code:java}
> public static void removeTmpSnapshotFiles(SnapshotFileTree sft, boolean err,
> IgniteLogger log) {
> NodeFileTree tmpFt = sft.tempFileTree();
> ```
> removeTmpDir(tmpFt.root(), err, log);
> for (File tmpDrStorage : tmpFt.extraStorages().values())
> removeTmpDir(tmpDrStorage.getParentFile(), err, log);
> ```
> }
> {code}
> And the helper currently accepts only one directory:
> {code:java}
> private static void removeTmpDir(File dir, boolean err, IgniteLogger log) {
> U.delete(dir);
> ```
> // Delete snapshot directory if no other files exists.
> try {
> if (U.fileCount(dir.toPath()) == 0 || err)
> U.delete(dir.toPath());
> }
> catch (IOException e) {
> log.error("Snapshot directory doesn't exist [snpName=" +
> dir.getName() + ", dir=" + dir.getParentFile() + ']');
> }
> ```
> }
> {code}
> This accidentally collapses the original two-level cleanup logic into a
> single directory. As a result, the code now:
> * removes the snapshot temporary root directory
> * then tries to count files in the same directory
> * then tries to delete the same directory again
> For example, instead of deleting:
> {code:java}
> <work_dir>/db/<node_storage>/snp/<snapshot_name>/db/<node_storage>{code}
> and then checking:
> {code:java}
> <work_dir>/db/<node_storage>/snp/<snapshot_name>{code}
> the current implementation deletes:
> {code:java}
> <work_dir>/db/<node_storage>/snp/<snapshot_name>{code}
> and then calls *U.fileCount(...)* for the same already deleted path. This
> leads to a false ERROR in the log after a successful snapshot creation:
> {code:java}
> Snapshot directory doesn't exist [snpName=<snapshot_name>,
> dir=<work_dir>/db/<node_storage>/snp]{code}
> (this is how the issue was identified in the first place, reproducer:
> [^SnapshotTmpDirCleanupReproducer.patch])
> Thus, because of the cahnges introduced in IGNITE-24130 the original cleanup
> logic was changed. The success path became more aggressive: the whole
> snapshot temporary root is removed before checking whether it still contains
> any unexpected files. Previously, such a non-empty root was not removed on
> the success path.
> h3. Expected behavior
> The cleanup logic should restore the original two-level behavior:
> * remove the node-specific temporary snapshot storage directory
> * then check the snapshot temporary root directory
> * remove the root directory if it becomes empty
> * remove the root directory regardless of its contents if snapshot
> processing finished with an error
> * do not call *U.fileCount(...)* on the directory that has just been deleted
> * do not log an error when the temporary directory has already been
> successfully removed
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
