Rename and recreate un-initialized cycle file instead of overwriting #1314
Conversation
alamar
requested review from
nicktindall,
peter-lawrey,
JerryShea and
rogersimmons
alamar
force-pushed
the
Queue1313
branch
2 times, most recently
from
d11c99c
to
8011411
Compare
| @@ -70,6 +67,7 @@ | |||
| public class SingleChronicleQueue extends AbstractCloseable implements RollingChronicleQueue { | |||
|
|
|||
| public static final String SUFFIX = ".cq4"; | |||
| public static final String DISCARD_FILE_SUFFIX = ".discard"; | |||
There was a problem hiding this comment.
I would suggest a sequence or timestamp-based decoration is a better/more flexible option
There was a problem hiding this comment.
Timestamp-based decoraton means we may copy a single cycle an arbitrary number of times, which in case of high performance system might as well be tens of thousands times which will exhaust inodes and-or current directory files limit. That's why I advise we use predictable discard name and avoid this issue entirely. If it's not successful after first try it's probably too dangerous to continue - what do you think @rogersimmons ?
There was a problem hiding this comment.
Yes, I agree with the concern, and it is something we should be in a position to mitigate.
Having a sequence/time-based decoration helps rather than impedes that, and at the same time gives us more control options in future if needed
There was a problem hiding this comment.
I don't know how we could mitigate this right now, whereas the current solution is indeed limited but is predictable and completely understood, and we can reason about its behavior.
I suggest that maybe we add time-based decoration together with the change that protects about creating the same cycle file unbounded number of times.
| @@ -108,7 +106,7 @@ public void appropriateExceptionIsThrownWhenLockCannotBeAcquiredForRecovery() th | |||
| assertTrue(readingDocument.isPresent()); | |||
| } | |||
| } | |||
| assertThrows(IOException.class, tailer::readingDocument); | |||
| assertEquals(false, tailer.readingDocument().isPresent()); | |||
There was a problem hiding this comment.
perhaps rename the test also (now an exception is not thrown)
| // Rename un-acquirable segment file to make sure no data is lost to try and recreate the segment | ||
| private boolean backupCycleFile(File cycleFile) { | ||
| File cycleFileDiscard = new File(cycleFile.getParentFile(), cycleFile.getName() + DISCARD_FILE_SUFFIX); | ||
| boolean success = cycleFile.renameTo(cycleFileDiscard); |
There was a problem hiding this comment.
Is it possible that multiple processes get here and think they moved the existing file (i.e is the move atomic), and would that cause chaos later if they both attempted to create the replacement file? I notice Files.move allows you to specify an atomic move (and define the behaviour when an existing file is there), not sure if that would be more appropriate here.
There was a problem hiding this comment.
Only one of these process succeeds and this one gets to recreate the file from scratch - other ones retry opening this file without createIfAbsent flag. I find the behavior of File.renameTo() more consistent than the described atomic move, but when we rename file in the current directory it should almost always be atomic yes or no operation.
|
Kudos, SonarCloud Quality Gate passed! |
closes #1313