[RFC] Add new property in IOOptions to skip recursing through directories and list only files during GetChildren. #10668

akankshamahajan15 · 2022-09-13T16:26:58Z

Summary: Add new property "do_not_recurse" in IOOptions for underlying file system to skip iteration of directories during DB::Open if there are no sub directories and list only files.
By default this property is set to false. This property is set true currently in the code where RocksDB is sure only files are needed during DB::Open.

Provided support in PosixFileSystem to use "do_not_recurse".

TestPlan:

Existing tests

facebook-github-bot · 2022-09-13T16:27:20Z

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

mrambacher

Seems like it would make sense to make the PosixFileSystem and other builtin FS also support this property, and to add associated tests. The tests should also support setting the value of the property to something that is not valid ("hello") and see what happens.

db/db_impl/db_impl_files.cc

facebook-github-bot · 2022-09-26T17:06:34Z

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

facebook-github-bot · 2022-09-26T17:10:14Z

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

facebook-github-bot · 2022-09-26T17:17:54Z

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

facebook-github-bot · 2022-09-26T19:17:53Z

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

pdillinger

Property bag seems a little weird, though if this is just a super-niche optimization, probably OK. Possible alternative: add a GetChildrenForFiles() function that may exclude directories from the list, with default implementation calling GetChildren().

A tricky part for testing (either with my proposal or what you have here) is that implementations are allowed to include directories, and would our unit tests a regression where we are confused by a directory being in the list? One possibility is to include them sometimes in DEBUG builds. But it would be weird to decide randomly. How about using one of the variant debug builds, such as ROCKSDB_ASSERT_STATUS_CHECKED or ROCKSDB_MODIFY_NPHASH, to use the old behavior in PosixFileSystem?

env/fs_posix.cc

facebook-github-bot · 2022-09-26T20:43:38Z

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

include/rocksdb/file_system.h

pdillinger · 2022-09-27T16:52:07Z

include/rocksdb/file_system.h

@@ -119,7 +133,9 @@ struct IOOptions {
        prio(IOPriority::kIOLow),
        rate_limiter_priority(Env::IO_TOTAL),
        type(IOType::kUnknown),
-        force_dir_fsync(force_dir_fsync_) {}
+        force_dir_fsync(force_dir_fsync_) {
+    SetProperty("list_files_only", "false");


Umm, I think this is going to be a non-trivial, unnecessary CPU consumer. Usually the threshold for negligible is .01%, and we already spend well over that in IOOptions::*, mostly in the destructor. And this would give it more work to do. (Internal detail in P533216605)

db/db_impl/db_impl.cc

facebook-github-bot · 2022-09-27T22:27:03Z

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

facebook-github-bot · 2022-09-27T22:28:30Z

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

akankshamahajan15 · 2022-09-27T22:47:19Z

A tricky part for testing (either with my proposal or what you have here) is that implementations are allowed to include directories, and would our unit tests a regression where we are confused by a directory being in the list? One possibility is to include them sometimes in DEBUG builds. But it would be weird to decide randomly. How about using one of the variant debug builds, such as ROCKSDB_ASSERT_STATUS_CHECKED or ROCKSDB_MODIFY_NPHASH, to use the old behavior in PosixFileSystem?

Sure. Let me try ROCKSDB_ASSERT_STATUS_CHECKED to use the old behavior in PosixFileSystem.

facebook-github-bot · 2022-09-28T15:06:22Z

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

facebook-github-bot · 2022-09-28T17:02:14Z

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

include/rocksdb/file_system.h

Summary: Add new property in IOOptions for underlying file system to skip iteration of directories during DB::Open if there are no sub directories. Test Plan: Reviewers: Subscribers: Tasks: Tags:

…SERT_STATUS_CHECKED Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

facebook-github-bot · 2022-10-02T17:14:28Z

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

facebook-github-bot · 2022-10-02T17:39:06Z

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

anand1976

LGTM

pdillinger · 2022-10-03T20:38:56Z

db/db_impl/db_impl.cc

@@ -4588,8 +4592,12 @@ Status DestroyDB(const std::string& dbname, const Options& options,
  // Reset the logger because it holds a handle to the
  // log file and prevents cleanup and directory removal
  soptions.info_log.reset();
+  IOOptions io_opts;


Did you intend to set the flag here? It's not clear.

pdillinger · 2022-10-03T20:42:49Z

include/rocksdb/file_system.h

@@ -112,14 +112,19 @@ struct IOOptions {
  // fsync, set this to force the fsync
  bool force_dir_fsync;

+  // Can be used by underlying file systems to skip recursing through sub
+  // directories and list only files in GetChildren API.
+  bool do_not_recurse;


This is a bad name. GetChildren is never recursive. It never returns children of children, only direct children. The question is whether to filter out directories (which would preclude wrapping GetChildren in recursive listing logic, but the name here is "do not recurse").

Also, AFAIK for backward compatibility and generality, this is only intended as a hint, not a strong requirement to filter out directories.

My GetChildrenForFiles() suggestion avoids extra "bloat" on IOOptions (fields only affecting one operation) and should make it easier to "do the right thing" rather than require an explicit IOOptions every time you want just the files in a directory. (See possible omission I pointed out in your code.) Also avoids the problem of "negative naming" on boolean variables. See e.g. https://www.serendipidata.com/posts/naming-guidelines-for-boolean-variables

facebook-github-bot added the CLA Signed label Sep 13, 2022

mrambacher reviewed Sep 16, 2022

View reviewed changes

db/db_impl/db_impl_files.cc Outdated Show resolved Hide resolved

db/db_impl/db_impl_files.cc Outdated Show resolved Hide resolved

db/db_impl/db_impl_files.cc Outdated Show resolved Hide resolved

akankshamahajan15 requested a review from anand1976 September 21, 2022 20:22

akankshamahajan15 force-pushed the pass_new_property branch from 706e43a to 307cf5b Compare September 26, 2022 17:06

akankshamahajan15 force-pushed the pass_new_property branch from 307cf5b to f19c9bd Compare September 26, 2022 17:10

akankshamahajan15 changed the title ~~Add new property in IOOptions to skip directories during GetChildren.~~ Add new property in IOOptions to skip directories and list only files during GetChildren. Sep 26, 2022

pdillinger reviewed Sep 26, 2022

View reviewed changes

env/fs_posix.cc Outdated Show resolved Hide resolved

akankshamahajan15 force-pushed the pass_new_property branch from 04cab05 to d959655 Compare September 26, 2022 20:43

pdillinger reviewed Sep 27, 2022

View reviewed changes

anand1976 reviewed Sep 27, 2022

View reviewed changes

db/db_impl/db_impl.cc Outdated Show resolved Hide resolved

akankshamahajan15 force-pushed the pass_new_property branch from d959655 to df18b62 Compare September 27, 2022 22:27

akankshamahajan15 force-pushed the pass_new_property branch from df18b62 to 53e733b Compare September 27, 2022 22:28

akankshamahajan15 requested review from pdillinger and anand1976 September 27, 2022 22:29

akankshamahajan15 force-pushed the pass_new_property branch from 53e733b to eb4e081 Compare September 28, 2022 15:06

akankshamahajan15 changed the title ~~Add new property in IOOptions to skip directories and list only files during GetChildren.~~ [RFC] Add new property in IOOptions to skip directories and list only files during GetChildren. Sep 28, 2022

anand1976 reviewed Sep 28, 2022

View reviewed changes

include/rocksdb/file_system.h Outdated Show resolved Hide resolved

akankshamahajan15 added 2 commits October 2, 2022 10:13

Add new property in IOOptions to skip directories during GetChildren.

512724b

Summary: Add new property in IOOptions for underlying file system to skip iteration of directories during DB::Open if there are no sub directories. Test Plan: Reviewers: Subscribers: Tasks: Tags:

Use list_files_only in PosixFileSystem::GetChildren

598258a

akankshamahajan15 added 3 commits October 2, 2022 10:13

Addressed comments to include "list_files_only" as bool in IOOptions

c9e329d

Test older version of Posix GetChildren API in debug build ROCKSDB_AS…

55c1be6

…SERT_STATUS_CHECKED Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Addressed comments and update history.md

b0601dd

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

akankshamahajan15 force-pushed the pass_new_property branch from eb4e081 to b0601dd Compare October 2, 2022 17:14

akankshamahajan15 changed the title ~~[RFC] Add new property in IOOptions to skip directories and list only files during GetChildren.~~ [RFC] Add new property in IOOptions to skip recursing through directories and list only files during GetChildren. Oct 2, 2022

akankshamahajan15 requested a review from anand1976 October 2, 2022 17:39

anand1976 approved these changes Oct 3, 2022

View reviewed changes

facebook-github-bot closed this in ae0f9c3 Oct 3, 2022

pdillinger reviewed Oct 3, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Add new property in IOOptions to skip recursing through directories and list only files during GetChildren. #10668

[RFC] Add new property in IOOptions to skip recursing through directories and list only files during GetChildren. #10668

akankshamahajan15 commented Sep 13, 2022 •

edited

Loading

facebook-github-bot commented Sep 13, 2022

mrambacher left a comment

facebook-github-bot commented Sep 26, 2022

facebook-github-bot commented Sep 26, 2022

facebook-github-bot commented Sep 26, 2022

facebook-github-bot commented Sep 26, 2022

pdillinger left a comment

facebook-github-bot commented Sep 26, 2022

pdillinger Sep 27, 2022

facebook-github-bot commented Sep 27, 2022

facebook-github-bot commented Sep 27, 2022

akankshamahajan15 commented Sep 27, 2022

facebook-github-bot commented Sep 28, 2022

facebook-github-bot commented Sep 28, 2022

facebook-github-bot commented Oct 2, 2022

facebook-github-bot commented Oct 2, 2022

anand1976 left a comment

pdillinger Oct 3, 2022

pdillinger Oct 3, 2022

[RFC] Add new property in IOOptions to skip recursing through directories and list only files during GetChildren. #10668

[RFC] Add new property in IOOptions to skip recursing through directories and list only files during GetChildren. #10668

Conversation

akankshamahajan15 commented Sep 13, 2022 • edited Loading

facebook-github-bot commented Sep 13, 2022

mrambacher left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 26, 2022

facebook-github-bot commented Sep 26, 2022

facebook-github-bot commented Sep 26, 2022

facebook-github-bot commented Sep 26, 2022

pdillinger left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 26, 2022

pdillinger Sep 27, 2022

Choose a reason for hiding this comment

facebook-github-bot commented Sep 27, 2022

facebook-github-bot commented Sep 27, 2022

akankshamahajan15 commented Sep 27, 2022

facebook-github-bot commented Sep 28, 2022

facebook-github-bot commented Sep 28, 2022

facebook-github-bot commented Oct 2, 2022

facebook-github-bot commented Oct 2, 2022

anand1976 left a comment

Choose a reason for hiding this comment

pdillinger Oct 3, 2022

Choose a reason for hiding this comment

pdillinger Oct 3, 2022

Choose a reason for hiding this comment

akankshamahajan15 commented Sep 13, 2022 •

edited

Loading