[feature] #1149: Restrict block number to 1 million per dir #2194

SamHSmith · 2022-05-11T12:16:31Z

Description of the Change

Take what was the previous file name, prepend with zeroes, then interpret that as dir + file name. So block files are ex 2/003, 0/032 et cetera.

Issue

#1149

Benefits

Most operating systems will freak out if there are too many files in a directory. This solves that.

Possible Drawbacks

We still iterate over all existing files to figure out which block file we want to edit/create. I think this needs to change in the long run. We should using a simple formula map from block number to block file number. That way you only touch the the file you want to operate on. That is a seperate issue from #1149 though, so the point is irrelevant for this PR.

codecov · 2022-05-11T12:26:59Z

Codecov Report

Merging #2194 (a395ba5) into iroha2-dev (49407ff) will increase coverage by 0.11%.
The diff coverage is 93.33%.

❗ Current head a395ba5 differs from pull request most recent head e47e4ec. Consider uploading reports for the commit e47e4ec to get more accurate results

@@              Coverage Diff               @@
##           iroha2-dev    #2194      +/-   ##
==============================================
+ Coverage       63.83%   63.95%   +0.11%     
==============================================
  Files             127      127              
  Lines           23592    23692     +100     
==============================================
+ Hits            15060    15152      +92     
- Misses           8532     8540       +8

Impacted Files	Coverage Δ
core/src/kura.rs	`93.17% <93.33%> (-0.21%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49407ff...e47e4ec. Read the comment docs.

core/src/kura.rs

Arjentix · 2022-05-20T15:44:05Z

core/src/kura.rs

@@ -463,15 +505,39 @@ async fn storage_files_base_indices<IO: DiskIO>(
    path: &Path,
    io: &IO,
 ) -> Result<BTreeSet<NonZeroU64>> {
-    let bases = io
+    let dirs: Vec<(String, IO::Dir)> = io
        .read_dir(path.to_path_buf())
        .await?
        .filter_map(|item| async {


Consider rewriting like this:

let dirs: Vec<(String, IO::Dir)> = io .read_dir(path.to_path_buf()) .await? .filter(Result::ok) .map(|item| async { let dir_name = item.to_string_lossy().into_owned(); match io.read_dir(path.join(dir_name.clone())).await { Ok(items_stream) => Some((dir_name, items_stream)), Err(_) => None, } }) .collect() .await;

Don't you think it looks prettier? I'm talking about using filter and map instead of filter_map.

AFAIK we should use filter_map when we will apply an operation to parameter regardless of its value. Here is not this situation, here we apply operation only on Ok variant.

core/src/kura.rs

Arjentix · 2022-05-20T16:19:24Z

core/src/kura.rs

+                        .map(|f_os| {
+                            let f = f_os.to_str();
+                            if f.is_some() && f.unwrap().starts_with(path_str) {
+                                let path_buf : PathBuf = f.unwrap().strip_prefix(path_str).unwrap().into();


strip_prefix() docs shows, that you don't need to add / sign before, it should work just fine without it. So you can remove path_srt manipulation above

That's not why it's important. It's important that the / get's removed so that it doesn't get treated as a root directory. If you start with dir/dir1/file2 and remove dir you get /dir1/file2 which is not what we want.

Yeah, that's what I'm talking about. Look at this playground

…per directory Signed-off-by: Sam H. Smith <sam.henning.smith@protonmail.com>

Arjentix

Good job! But can you, please, also place somewhere in the code documentation about new files structure?

Arjentix · 2022-05-20T20:04:10Z

core/src/kura.rs

            .then(move |e| {
                let io = io.clone();
-                async move { io.open(e.into_os_string()).await }
+                // async move { io.open(e.into_os_string()).await }


Commented code

Arjentix · 2022-05-20T20:10:19Z

core/src/kura.rs

@@ -463,15 +505,39 @@ async fn storage_files_base_indices<IO: DiskIO>(
    path: &Path,
    io: &IO,
 ) -> Result<BTreeSet<NonZeroU64>> {
-    let bases = io
+    let dirs: Vec<(String, IO::Dir)> = io
        .read_dir(path.to_path_buf())
        .await?
        .filter_map(|item| async {


Don't you think it looks prettier? I'm talking about using filter and map instead of filter_map.

AFAIK we should use filter_map when we will apply an operation to parameter regardless of its value. Here is not this situation, here we apply operation only on Ok variant.

Arjentix · 2022-05-20T20:15:14Z

core/src/kura.rs

+        fs::DirBuilder::new()
+            .recursive(true)
+            .create(&dir.path().join("0"))
+            .await
+            .unwrap();


create_dir_all()? Should be fine here. But if you like this style, keep it

Arjentix · 2022-05-20T20:20:03Z

core/src/kura.rs

-            .map(move |e| dir_path.join(e.to_string()))
+        let mut files = Vec::new();
+        for index in &base_indices {
+            files.push(self.filename_to_path(&index.to_string()));


What about renaming filename_to_path() function to block_index_to_path()?

Arjentix · 2022-05-20T20:24:33Z

core/src/kura.rs

    pub async fn get_block_path(&self, block_height: NonZeroU64) -> Result<PathBuf> {
-        let filename = self.get_block_filename(block_height).await?;
-        Ok(self.path.join(filename))
+        let file_string = self.get_block_filename(block_height).await?;
+        Ok(self.filename_to_path(&file_string))
    }


It's a little confusing for me that get_block_filename() does not return valid path and needs further processing... Maybe rename it to something else or refactor somehow?

github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label May 11, 2022

appetrosyan changed the title ~~draft~~ [feature] #1149: Block amount. May 11, 2022

appetrosyan self-assigned this May 11, 2022

appetrosyan deleted the branch hyperledger:iroha2-dev May 13, 2022 16:12

appetrosyan closed this May 13, 2022

appetrosyan reopened this May 16, 2022

appetrosyan added the Optimization Something isn't working as well as it should label May 20, 2022

SamHSmith force-pushed the less_than_million_block_files_per_dir branch from 92f2f30 to ffa458f Compare May 20, 2022 11:05

Arjentix suggested changes May 20, 2022

View reviewed changes

Arjentix self-assigned this May 20, 2022

SamHSmith force-pushed the less_than_million_block_files_per_dir branch 2 times, most recently from 4040754 to 721f807 Compare May 20, 2022 18:42

SamHSmith marked this pull request as ready for review May 20, 2022 18:53

SamHSmith requested review from s8sato, appetrosyan, mversic, QuentinI and outoftardis as code owners May 20, 2022 18:53

[feature] hyperledger#1149: Block file count does not exceed 1000000 …

e47e4ec

…per directory Signed-off-by: Sam H. Smith <sam.henning.smith@protonmail.com>

SamHSmith force-pushed the less_than_million_block_files_per_dir branch from 721f807 to e47e4ec Compare May 20, 2022 18:53

Arjentix approved these changes May 20, 2022

View reviewed changes

appetrosyan approved these changes May 21, 2022

View reviewed changes

appetrosyan changed the title ~~[feature] #1149: Block amount.~~ [feature] #1149: Restrict block number to 1 million per dir May 21, 2022

appetrosyan merged commit b25c82a into hyperledger:iroha2-dev May 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] #1149: Restrict block number to 1 million per dir #2194

[feature] #1149: Restrict block number to 1 million per dir #2194

SamHSmith commented May 11, 2022

codecov bot commented May 11, 2022 •

edited

Arjentix May 20, 2022

Arjentix May 20, 2022

Arjentix May 20, 2022

SamHSmith May 20, 2022

Arjentix May 20, 2022

Arjentix left a comment

Arjentix May 20, 2022

Arjentix May 20, 2022

Arjentix May 20, 2022

Arjentix May 20, 2022

Arjentix May 20, 2022

[feature] #1149: Restrict block number to 1 million per dir #2194

[feature] #1149: Restrict block number to 1 million per dir #2194

Conversation

SamHSmith commented May 11, 2022

Description of the Change

Issue

Benefits

Possible Drawbacks

codecov bot commented May 11, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arjentix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 11, 2022 •

edited