From b6de139df51c655691930f4dc669f218745aba11 Mon Sep 17 00:00:00 2001
From: zczhu <>
Date: Tue, 7 Jun 2022 09:49:31 -0700
Subject: [PATCH] =?UTF-8?q?Handle=20"NotSupported"=20status=20by=20default?=
 =?UTF-8?q?=20implementation=20of=20Close()=20in=20=E2=80=A6=20(#10127)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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: https://github.com/facebook/rocksdb/pull/10127

Reviewed By: ajkr

Differential Revision: D36971112

Pulled By: littlepig2013

fbshipit-source-id: 100f0e6ad1191e1acc1ba6458c566a11724cf466
---
 db/column_family.cc   |  2 --
 db/db_impl/db_impl.cc | 12 +++++++++++-
 db/db_impl/db_impl.h  | 37 +++++++++++++++++++++++++++++++++----
 file/filename.cc      | 13 ++++++++++++-
 4 files changed, 56 insertions(+), 8 deletions(-)

diff --git a/db/column_family.cc b/db/column_family.cc
index bac4c21a6..90c0f3e25 100644
--- 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
index 4dbf89a43..872c6e7a1 100644
--- 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
index 4e192e831..721c73c37 100644
--- 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
index e31dbb681..703167c88 100644
--- 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();