Handle "NotSupported" status by default implementation of Close() in … (

#10127) Summary: The default implementation of Close() function in Directory/FSDirectory classes returns `NotSupported` status. However, we don't want operations that worked in older versions to begin failing after upgrading when run on FileSystems that have not implemented Directory::Close() yet. So we require the upper level that calls Close() function should properly handle "NotSupported" status instead of treating it as an error status. Pull Request resolved: #10127 Reviewed By: ajkr Differential Revision: D36971112 Pulled By: littlepig2013 fbshipit-source-id: 100f0e6ad1191e1acc1ba6458c566a11724cf466
facebook · Jun 7, 2022 · b6de139 · b6de139
1 parent 3ee6c9b
commit b6de139
Show file tree

Hide file tree

Showing 4 changed files with 56 additions and 8 deletions.
diff --git a/db/column_family.cc b/db/column_family.cc
@@ -683,8 +683,6 @@ ColumnFamilyData::~ColumnFamilyData() {
         s = data_dir_ptr->Close(IOOptions(), nullptr);
         if (!s.ok()) {
           // TODO(zichen): add `Status Close()` and `CloseDirectories()
-          ROCKS_LOG_WARN(ioptions_.logger, "Ignoring error %s",
-                         s.ToString().c_str());
           s.PermitUncheckedError();
         }
       }

diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc
@@ -4408,7 +4408,17 @@ Status DBImpl::RenameTempFileToOptionsFile(const std::string& file_name) {
                                        DirFsyncOptions(options_file_name));
     }
     if (s.ok()) {
-      s = dir_obj->Close(IOOptions(), nullptr);
+      Status temp_s = dir_obj->Close(IOOptions(), nullptr);
+      // The default Close() could return "NotSupproted" and we bypass it
+      // if it is not impelmented. Detailed explanations can be found in
+      // db/db_impl/db_impl.h
+      if (!temp_s.ok()) {
+        if (temp_s.IsNotSupported()) {
+          temp_s.PermitUncheckedError();
+        } else {
+          s = temp_s;
+        }
+      }
     }
   }
   if (s.ok()) {

diff --git a/db/db_impl/db_impl.h b/db/db_impl/db_impl.h
@@ -116,8 +116,23 @@ class Directories {
   IOStatus Close(const IOOptions& options, IODebugContext* dbg) {
     // close all directories for all database paths
     IOStatus s = IOStatus::OK();
+    IOStatus temp_s = IOStatus::OK();
+
+    // The default implementation for Close() in Directory/FSDirectory class
+    // "NotSupported" status, the upper level interface should be able to
+    // handle this error so that Close() does not fail after upgrading when
+    // run on FileSystems that have not implemented `Directory::Close()` or
+    // `FSDirectory::Close()` yet
+
     if (db_dir_) {
-      s = db_dir_->Close(options, dbg);
+      temp_s = db_dir_->Close(options, dbg);
+      if (!temp_s.ok()) {
+        if (temp_s.IsNotSupported()) {
+          temp_s.PermitUncheckedError();
+        } else {
+          s = temp_s;
+        }
+      }
     }
 
     if (!s.ok()) {
@@ -126,6 +141,13 @@ class Directories {
 
     if (wal_dir_) {
       s = wal_dir_->Close(options, dbg);
+      if (!temp_s.ok()) {
+        if (temp_s.IsNotSupported()) {
+          temp_s.PermitUncheckedError();
+        } else {
+          s = temp_s;
+        }
+      }
     }
 
     if (!s.ok()) {
@@ -135,14 +157,21 @@ class Directories {
     if (data_dirs_.size() > 0 && s.ok()) {
       for (auto& data_dir_ptr : data_dirs_) {
         if (data_dir_ptr) {
-          s = data_dir_ptr->Close(options, dbg);
-          if (!s.ok()) {
-            return s;
+          temp_s = data_dir_ptr->Close(options, dbg);
+          if (!temp_s.ok()) {
+            if (temp_s.IsNotSupported()) {
+              temp_s.PermitUncheckedError();
+            } else {
+              return temp_s;
+            }
           }
         }
       }
     }
 
+    // Mark temp_s as checked when temp_s is still the initial status
+    // (IOStatus::OK(), not checked yet)
+    temp_s.PermitUncheckedError();
     return s;
   }
 

diff --git a/file/filename.cc b/file/filename.cc
@@ -443,8 +443,19 @@ Status SetIdentityFile(Env* env, const std::string& dbname,
     s = dir_obj->FsyncWithDirOptions(IOOptions(), nullptr,
                                      DirFsyncOptions(identify_file_name));
   }
+
+  // The default Close() could return "NotSupported" and we bypass it
+  // if it is not impelmented. Detailed explanations can be found in
+  // db/db_impl/db_impl.h
   if (s.ok()) {
-    s = dir_obj->Close(IOOptions(), nullptr);
+    Status temp_s = dir_obj->Close(IOOptions(), nullptr);
+    if (!temp_s.ok()) {
+      if (temp_s.IsNotSupported()) {
+        temp_s.PermitUncheckedError();
+      } else {
+        s = temp_s;
+      }
+    }
   }
   if (!s.ok()) {
     env->DeleteFile(tmp).PermitUncheckedError();