Support GetFileSize API in FSRandomAccessFile #13676

xingbowang · 2025-06-06T22:41:54Z

Summary:

This change is used to address this issue
#13619
It supports GetFileSize API in FSRandomAccessFile. This allows ReadFooterFromFile to quickly get the file size for file size validation.

Test Plan:
make check

Reviewers:
Peter Dillinger

Subscribers:

Tasks:

Tags:

facebook-github-bot · 2025-06-06T22:44:17Z

@xingbowang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

pdillinger

I would include as many built-in implementations as you can in this PR (git grep 'public FSRandomAccessFile') and add a basic unit test in env_test that simply writes a file of some random size, up to ~100KB, opens it for reads and checks that the written size agrees with all the file size APIs.

We also need to override this function in all the wrapper classes. Basically, if you can't compile the full repository with this function pure virtual instead of defaulting to NotSupported, you should have an explanation for each of the internal cases you have not implemented in this PR.

pdillinger · 2025-06-10T15:45:57Z

include/rocksdb/file_system.h

@@ -909,6 +909,13 @@ class FSRandomAccessFile {
    return IOStatus::NotSupported("Prefetch");
  }

+  // Get the file size. The default implementation is not supported, as some of


I would say

The default implementation returns "not supported" so that user implementations of FSRandomAccessFile do not need to immediately implement this function.

Thanks for the feedback. I will address them.
I also noticed another class RandomAccessFile, which had exact same functions as FSRandomAccessFile. Should I add the new API there as well. I recall you mentioned that it was from a legacy system used in env. The code seems indicate that as well.

Should not be needed for the legacy Env-related class RandomAccessFile. Just adds to the confusion about what should be overridden. #9274

RandomAccessFile is used in LegacyRandomAccessFileWrapper. To make the wrapper work, I had to add the new API GetFileSize in RandomAccessFile. Let me know if it is a concern.

pdillinger · 2025-06-10T15:55:28Z

Also, use unreleased_history/add.sh to add a release note. At this point, the user should care that they have the option to override this new function to make some file metadata queries more efficient. And we could say that this function might be required in the future.

facebook-github-bot · 2025-06-16T20:08:40Z

@xingbowang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-06-16T20:09:31Z

@xingbowang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-06-17T18:09:30Z

@xingbowang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-06-17T22:51:59Z

@xingbowang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2025-06-17T23:09:13Z

@xingbowang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-06-17T23:10:13Z

@xingbowang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2025-06-18T21:45:19Z

@xingbowang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-06-18T22:00:51Z

@xingbowang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-06-18T23:04:30Z

@xingbowang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2025-06-19T13:15:48Z

@xingbowang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-06-19T13:17:43Z

@xingbowang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-06-20T17:28:54Z

@xingbowang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-06-20T17:29:15Z

@xingbowang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

pdillinger

Sorry I typed up these review comments a few days ago and forgot to submit

pdillinger · 2025-06-17T19:18:48Z

env/env_test.cc

+
+  constexpr size_t kSectorSize = 4096;
+  constexpr size_t kNumSectors = 128;
+  constexpr size_t expectedFileSize = kSectorSize * kNumSectors;


Not enough entropy, imho. Could be something random up to something like this size.

Do you mean randomize the number of kNumSectors in each run?

kNumSectors = 64 + (std::rand() % 64);

No, that's not enough entropy. A FileSystem must provide files of any number of bytes (up to some limit). The notion of "sector" seems irrelevant here.

pdillinger · 2025-06-17T19:20:15Z

env/env_test.cc

+    ASSERT_OK(fs->NewRandomAccessFile(fname, FileOptions(), &file, nullptr));
+
+    uint64_t actualFileSize;
+    file->GetFileSize(&actualFileSize);


It is suspicious/wrong that this is not reported by our ASSERT_STATUS_CHECKED CI test. (The return value is ignored and needs to be checked.)

Check with ASSERT_OK

pdillinger · 2025-06-17T19:20:38Z

env/env_test.cc

+};
+
+// Validate GetFileSize API returns the right value.
+// Use the default implementation from Posix File system


It might not be posix depending on platform.

pdillinger · 2025-06-17T19:21:31Z

env/env_test.cc

+  Env* env_;
+};
+
+// Validate GetFileSize API returns the right value.


What about FileSystem::GetFileSize()?

Could you elaborate more about this? Do you mean validate the file size against FileSystem::GetFileSize API?

Something like that. I was just pointing out an inconsistency/incompleteness/ambiguity in what was claimed to be tested vs. actually tested.

pdillinger · 2025-06-17T19:22:32Z

env/env_test.cc

+    std::unique_ptr<FSRandomAccessFile> file;
+    ASSERT_OK(fs->NewRandomAccessFile(fname, FileOptions(), &file, nullptr));
+
+    uint64_t actualFileSize;


APIs can lie or have bugs. I would call this "reportedFileSize".

pdillinger · 2025-06-17T19:25:46Z

include/rocksdb/file_system.h

@@ -1051,6 +1051,14 @@ class FSRandomAccessFile {
  // open.
  virtual Temperature GetTemperature() const { return Temperature::kUnknown; }

+  // Get the file size.


... on an open-for-reading file without re-seeking the file's path in the filesystem.

In other words, API documentation should not only include "what" something is but "why" something exists (if it's potentially redundant).

pdillinger · 2025-06-17T19:31:06Z

table/format.cc 2684

+  uint64_t file_size_from_file_system = 0;
+  Status s;
+  s = file->file()->GetFileSize(&file_size_from_file_system);
+  if (!s.ok()) {


I believe we discussed that it would be OK for implementations not to implement the new function immediately. (Allow NotSupported here.) In fact, we should be testing that scenario in unit tests. We could consider doing that by holding back the env_encryption implementation until we make the function is required. (Justification for that state described well in comments in env_encryption.)

pdillinger · 2025-06-17T19:33:38Z

table/format.cc

+    // manifest. Otherwise it is not guaranteed.
+    return Status::Corruption(
+        "Sst file size mismatch: " + file->file_name() + ". Expected " +
+        std::to_string(expected_file_size) + ", actual size " +


I prefer "reported" to "actual"

facebook-github-bot · 2025-06-23T16:02:28Z

@xingbowang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-06-23T16:24:48Z

@xingbowang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-06-23T19:22:09Z

@xingbowang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Deprecate skip_checking_sst_file_sizes_on_db_open option. Summary: This change is used to address this issue facebook#13619 It supports file size validation in ReadFooterFromFile. In favor of this change, CheckConsistency function and skip_checking_sst_file_sizes_on_db_open flag are deprecated. The CheckConsistency function checks each file size matches what was recorded in manifest during DB open. Meantime, ReadFooterFromFile was called for each file in LoadTables function. Since ReadFooterFromFile always validates file size, the CheckConsistency is redundant. In addtion, CheckConsistency is ecxecuted in a single thread. This could slow down DB open when a network file system is used. Therefore, the flag skip_checking_sst_file_sizes_on_db_open was added to skip this check. After this change, ReadFooterFromFile was executed in parallel through multiple threads. Therefore, the concern of DB open slowness is eliminated, and the flag could be deprecated. There is 2 slight concerns of this change. *If max_open_files is set with smaller value, engine will not open all the files during DB open. This means if there is a corruption on file size, it will not be detected during DB open, but rather at a later time. Since the default is -1, which means open all the files, and it is rarely overridden and a lot of new features rely on it to be -1, the risk is very low. *If FIFO compaction is used, engine could fail to open DB unnecessarily on the corrupted files that would never be used again. However, this is a very rare case as well. The error could still be ignored by setting paranoid_checks operationally. The risk is very low. To remain backward compatibility. The public facing flag was kept and marked as no-op internally. Another change is required to fully remove the flag. Test Plan: make check A new unit test was added to validate file size check API works as expected.

facebook-github-bot · 2025-06-23T19:22:18Z

@xingbowang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-06-23T21:48:31Z

@xingbowang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot added the CLA Signed label Jun 6, 2025

xingbowang assigned pdillinger Jun 6, 2025

pdillinger requested changes Jun 10, 2025

View reviewed changes

xingbowang force-pushed the main branch from 9d241e1 to cee9b60 Compare June 16, 2025 20:08

xingbowang force-pushed the main branch from 2c907e4 to b24c5ca Compare June 17, 2025 18:09

xingbowang force-pushed the main branch from b24c5ca to 59c00a1 Compare June 17, 2025 23:09

xingbowang force-pushed the main branch from 59c00a1 to 833d401 Compare June 18, 2025 21:45

xingbowang force-pushed the main branch from fc5b3f9 to 1318b02 Compare June 19, 2025 13:17

pdillinger requested changes Jun 20, 2025

View reviewed changes

xingbowang force-pushed the main branch from 96b3478 to 1d787f1 Compare June 23, 2025 16:02

xingbowang force-pushed the main branch from 1d787f1 to d1262ee Compare June 23, 2025 16:24

xingbowang force-pushed the main branch from d1262ee to e43e91e Compare June 23, 2025 19:22

Support GetFileSize API in FSRandomAccessFile #13676

Are you sure you want to change the base?

Support GetFileSize API in FSRandomAccessFile #13676

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!