[
https://issues.apache.org/jira/browse/IGNITE-28870?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Oleg Valuyskiy updated IGNITE-28870:
------------------------------------
Description:
After the changes introduced in IGNITE-24130 / PR #11896, Ignite may log the
following ERROR after a snapshot is successfully created:
{code:java}
Snapshot directory doesn't exist [snpName=<snapshot_name>,
dir=<work_dir>/db/<node_storage>/snp]{code}
The snapshot itself may be successfully created in the configured snapshot
directory. The error is logged during cleanup of temporary snapshot files.
Reproducer: [^SnapshotTmpDirCleanupReproducer.patch]
h3. Original cleanup logic
Before IGNITE-24130 / PR #11896, snapshot cleanup used two different temporary
directories ({*}SnapshotFutureTask#onDone{*}):
{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 filesystem levels:
* 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 logic was:
* remove the node-specific temporary snapshot storage directory;
* then check the temporary root directory of the current snapshot;
* if the root directory has no other files left, remove it as well;
* if snapshot processing finished with an error, remove the temporary root
directory regardless of its contents.
h3. Regression after IGNITE-24130 / PR #11896
After the data region storage path support changes, 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}
The helper currently receives 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. The method now:
* removes the snapshot temporary root directory;
* then tries to count files in that same directory;
* then tries to delete the same directory again.
So after:
{code:java}
U.delete(dir);{code}
the following call may fail with NoSuchFileException:
{code:java}
U.fileCount(dir.toPath()){code}
This results in a false ERROR in the log even though the snapshot temporary
directory has already been successfully removed.
h3. Expected behavior
The cleanup logic should preserve the original two-level semantics:
* remove the node-specific temporary snapshot storage directory;
* check the snapshot temporary root directory;
* remove the root directory if it becomes empty;
* remove the root directory 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 directory has already been removed successfully.
was:
After the changes introduced in IGNITE-24130 / PR #11896, Ignite may log the
following ERROR after a snapshot is successfully created:
{code:java}
Snapshot directory doesn't exist [snpName=<snapshot_name>,
dir=<work_dir>/db/<node_storage>/snp]{code}
The snapshot itself may be successfully created in the configured snapshot
directory. The error is logged during cleanup of temporary snapshot files.
Reproducer: [^SnapshotTmpDirCleanupReproducer.patch]
h3. Original cleanup logic
Before IGNITE-24130 / PR #11896, snapshot cleanup used two different temporary
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 filesystem levels:
* 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 logic was:
* remove the node-specific temporary snapshot storage directory;
* then check the temporary root directory of the current snapshot;
* if the root directory has no other files left, remove it as well;
* if snapshot processing finished with an error, remove the temporary root
directory regardless of its contents.
h3. Regression after IGNITE-24130 / PR #11896
After the data region storage path support changes, 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}
The helper currently receives 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. The method now:
* removes the snapshot temporary root directory;
* then tries to count files in that same directory;
* then tries to delete the same directory again.
So after:
{code:java}
U.delete(dir);{code}
the following call may fail with NoSuchFileException:
{code:java}
U.fileCount(dir.toPath()){code}
This results in a false ERROR in the log even though the snapshot temporary
directory has already been successfully removed.
h3. Expected behavior
The cleanup logic should preserve the original two-level semantics:
* remove the node-specific temporary snapshot storage directory;
* check the snapshot temporary root directory;
* remove the root directory if it becomes empty;
* remove the root directory 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 directory has already been removed successfully.
> False ERROR is logged during snapshot temporary directory cleanup
> -----------------------------------------------------------------
>
> 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
>
>
> After the changes introduced in IGNITE-24130 / PR #11896, Ignite may log the
> following ERROR after a snapshot is successfully created:
> {code:java}
> Snapshot directory doesn't exist [snpName=<snapshot_name>,
> dir=<work_dir>/db/<node_storage>/snp]{code}
> The snapshot itself may be successfully created in the configured snapshot
> directory. The error is logged during cleanup of temporary snapshot files.
> Reproducer: [^SnapshotTmpDirCleanupReproducer.patch]
> h3. Original cleanup logic
> Before IGNITE-24130 / PR #11896, snapshot cleanup used two different
> temporary directories ({*}SnapshotFutureTask#onDone{*}):
> {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 filesystem levels:
> * 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 logic was:
> * remove the node-specific temporary snapshot storage directory;
> * then check the temporary root directory of the current snapshot;
> * if the root directory has no other files left, remove it as well;
> * if snapshot processing finished with an error, remove the temporary root
> directory regardless of its contents.
> h3. Regression after IGNITE-24130 / PR #11896
> After the data region storage path support changes, 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}
> The helper currently receives 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. The method now:
> * removes the snapshot temporary root directory;
> * then tries to count files in that same directory;
> * then tries to delete the same directory again.
> So after:
> {code:java}
> U.delete(dir);{code}
> the following call may fail with NoSuchFileException:
> {code:java}
> U.fileCount(dir.toPath()){code}
> This results in a false ERROR in the log even though the snapshot temporary
> directory has already been successfully removed.
> h3. Expected behavior
> The cleanup logic should preserve the original two-level semantics:
> * remove the node-specific temporary snapshot storage directory;
> * check the snapshot temporary root directory;
> * remove the root directory if it becomes empty;
> * remove the root directory 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 directory has already been removed
> successfully.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
