mcvsubbu commented on code in PR #14883:
URL: https://github.com/apache/pinot/pull/14883#discussion_r1926380426
##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/LocalPinotFS.java:
##########
@@ -193,19 +200,76 @@ 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);
+ 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);
+ }
+
+ // Step 4: Verify CRC of copied file
+ long dstCrc = calculateCrc(dstFile);
+ if (srcCrc != dstCrc) {
+
+ throw new IOException("CRC mismatch: source and destination files are
not identical.");
+ }
+ } catch (IOException e) {
+ // Restore destination file from backup if copy fails
+ if (oldFileExists) {
+ if (!dstFile.delete() || !backupFile.renameTo(dstFile)) {
+ throw new IOException("Failed to restore destination file from
backup after failed copy.");
+ }
} else {
- // Throws Exception on failure
- throw new IOException(srcFile.getAbsolutePath() + " is a directory and
recursive copy is not enabled.");
+ dstFile.delete();
+ }
+ throw e;
+ }
+
+ if (oldFileExists) {
Review Comment:
One thing that may go south is access to the remote file system. In that
case, the corrected step will also fail, right? In that case, the only recourse
is to field the exception and raise a metric and then fix it when an alert
sounds. Is that correct? In that case, you may want to raise a specific
(defined by Pinot) exception?
--
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
