jasperjiaguo commented on code in PR #14883:
URL: https://github.com/apache/pinot/pull/14883#discussion_r1927469280
##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/LocalPinotFS.java:
##########
@@ -193,19 +200,78 @@ private static File toFile(URI uri) {
private static void copy(File srcFile, File dstFile, boolean recursive)
throws IOException {
- if (dstFile.exists()) {
- FileUtils.deleteQuietly(dstFile);
+ boolean oldFileExists = dstFile.exists(); // Automatically set
oldFileExists if the old file exists
+
+ // Step 1: Calculate CRC of srcFile/directory
+ long srcCrc = calculateCrc(srcFile);
+ File backupFile = null;
+
+ if (oldFileExists) {
+ // Step 2: Rename destination file if it exists
+ backupFile = new File(dstFile.getAbsolutePath() + BACKUP +
System.currentTimeMillis());
+ if (!dstFile.renameTo(backupFile)) {
+ throw new IOException("Failed to rename destination file to backup.");
+ }
}
- if (srcFile.isDirectory()) {
- if (recursive) {
- FileUtils.copyDirectory(srcFile, dstFile);
+
+ // If any error occurs during the copy process, we want to restore the
destination file from the backup
+ try {
+ // Step 3: Copy the file or directory
+ if (srcFile.isDirectory()) {
+ if (recursive) {
+ FileUtils.copyDirectory(srcFile, dstFile);
+ } else {
+ throw new IOException(srcFile.getAbsolutePath() + " is a directory
and recursive copy is not enabled.");
+ }
+ } else {
+ FileUtils.copyFile(srcFile, dstFile);
Review Comment:
> I think this is still problematic. For example for the case of refreshing
an existing segment, if controller restarts during the copy operation, the
corrupted segment with dstFile name remains in storage layer, which is not
desirable.
This is true. In this case the tmp file would still be there for the oncall
to backfill. Would not cause data loss.
> IMO if we do ^^, the CRC check is not needed here. If we still have corner
cases that CRC check can help, we can create a periodic task to check CRC.
I think the CRC check is probably inevitable as the copy operation itself is
not reliable either way. The crc is pretty much to ensure the read-after-write
consistency. Periodical check of the CRC would not help? As IIRC we do not
store the CRC of the tar file itself?
> A simpler way is to copy srsFile to a temp file in deep store. Then do a
rename/move from temp file to dstFile. If copy fails for any reason, it's on a
temp file, so there's no harm. If copy passes, then since rename is atomic, we
should be in a good state in both failure or success of rename.
Agreed, I think leaving a bad temp file is better. Let me make the change.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
