[
https://issues.apache.org/jira/browse/IGNITE-28870?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Oleg Valuyskiy updated IGNITE-28870:
------------------------------------
Description:
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
was:
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
> 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
>
>
> 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)
