Move view index files to .recovery when db is deleted #597

jiangphcn · 2017-06-14T09:19:02Z

Overview

Before change, the view index files were renamed in place if the corresponding database was deleted and "enable_database_recovery" configuration item is set to true. This allows view index files to be re-used if database is recovered.
However, these deleted view files are spread widely and may become orphans. This makes disk management difficult.
In order to help better manage disk, the change is to implement automatic movement of index files into centralized place when main database is deleted.

Testing recommendations

make check apps=couch tests=nuke_dir_test_

======================== EUnit ========================
Nuke directory tests
  enable_database_recovery = false, context = delete
    couch_file_tests:499: make_rename_dir_test_case...ok
    couch_file_tests:500: make_rename_dir_test_case...ok
    couch_file_tests:501: make_rename_dir_test_case...ok
    [done in 0.009 s]
  enable_database_recovery = false, context = compaction
    couch_file_tests:499: make_rename_dir_test_case...ok
    couch_file_tests:500: make_rename_dir_test_case...ok
    couch_file_tests:501: make_rename_dir_test_case...ok
    [done in 0.009 s]
  enable_database_recovery = true, context = delete
    couch_file_tests:499: make_rename_dir_test_case...ok
    couch_file_tests:500: make_rename_dir_test_case...ok
    couch_file_tests:501: make_rename_dir_test_case...ok
    [done in 0.009 s]
  enable_database_recovery = true, context = compaction
    couch_file_tests:499: make_rename_dir_test_case...ok
    couch_file_tests:500: make_rename_dir_test_case...ok
    couch_file_tests:501: make_rename_dir_test_case...ok
    [done in 0.009 s]
  delete_after_rename = true
    couch_file_tests:517: make_delete_dir_test_case...ok
    couch_file_tests:518: make_delete_dir_test_case...ok
    couch_file_tests:519: make_delete_dir_test_case...ok
    couch_file_tests:520: make_delete_dir_test_case...ok
    [done in 0.012 s]
  delete_after_rename = false
    couch_file_tests:517: make_delete_dir_test_case...ok
    couch_file_tests:518: make_delete_dir_test_case...ok
    couch_file_tests:519: make_delete_dir_test_case...ok
    couch_file_tests:520: make_delete_dir_test_case...ok
    [done in 0.012 s]
  [done in 0.473 s]
=======================================================
  All 20 tests passed.

GitHub issue number

This PR will be the number

Related Pull Requests

Checklist

Code is written and works correctly;
Changes are covered by tests;
Documentation reflects the changes;

eiri

I like this better than the previous approach. A couple of things to address:

You need to ensure that your new recovery dir is actually exists and create it otherwise. Check out function init_delete_dir/1 in couch_file
So what is the final path for the recovery indexes? I should admit I'm a bit lost with all the renaming. Would be nice to get some tests for that new naming scheme added in this section
I think we at stage when we want to run some kind of perf test to ensure that this approach is actually resolving an original issue of looking for deleted dirs in overpopulated main index directory.

eiri · 2017-07-06T03:25:01Z

src/couch_index/src/couch_index_server.erl

@@ -246,10 +247,10 @@ rem_from_ets(DbName, Sig, DDocId, Pid) ->


 handle_db_event(DbName, created, St) ->
-    gen_server:cast(?MODULE, {reset_indexes, DbName}),
+    gen_server:cast(?MODULE, {reset_indexes, [{db_name, DbName}, {context, []}]}),


This is not quite right. You are setting context here to an empty list, but valid values for it only compaction and delete. You need to avoid context attribute here at all and let it be a default.

eiri · 2017-07-06T03:31:42Z

src/couch_index/src/couch_index_server.erl

@@ -219,7 +219,8 @@ new_index({Mod, IdxState, DbName, Sig}) ->
    end.


-reset_indexes(DbName, Root) ->
+reset_indexes(Options, Root) ->


I think it's better to make this into reset_indexes(DbName, Root, Options) and change the casts accordingly. A database name, unlike context, is not an optional parameter, so we shouldn't risk to accidentally forget to include it into options' list and default to undefined on a line below.

On the other hand it's fine to pass it inside of Options to nuke_dir/3, because it is an optional there, only used when we want to rename a dir.

eiri · 2017-07-06T05:12:04Z

src/couch/src/couch_file.erl

    EnableRecovery = config:get_boolean("couchdb",
        "enable_database_recovery", false),
    case EnableRecovery of
        true ->
-            rename_file(Dir);
+            Context = couch_util:get_value(context, Options, compaction),
+            case Context =:= delete of


You can avoid case double-fold here with guards in condition, something like

case EnableRecovery of true when Context == delete -> ...; true -> ...; false -> ... end

eiri · 2017-07-06T05:15:36Z

src/couch/src/couch_file.erl

@@ -264,6 +264,16 @@ rename_file(Original) ->
        Else -> Else
    end.

+rename_dir(RootDelDir, Original, DbName) ->
+    DbDir = binary_to_list(DbName) ++ "_design",
+    Deleted_Index_Dir = filename:join([RootDelDir, ".view_recovery", DbDir]),


I'd call recovery dir just .recovery if we'll want to use the same approach for db files recovery, the same as we call delete dir just .delete and not .view_delete

eiri · 2017-07-06T05:30:24Z

src/couch/test/couch_file_tests.erl

+                end,
+                RecDirPaths = RootDir ++ "/.view_recovery" ++ "/*_design",
+                case filelib:wildcard(RecDirPaths) of
+                    RecDirs -> [remove_dir(Dir) || Dir <- RecDirs];


First, you have a catch-all on a top clause, so a next line will never be reached. Compiler should've warn you about that.

Second, just do [remove_dir(Dir) || Dir <- filelib:wildcard(RecDirPaths)] here. The clause above explicitly catching an element of a single-element list, but here we want comprehension list on a whole return of wildcard, so it doesn't matter if it's an empty list or not.

jiangphcn · 2017-07-06T09:23:41Z

@eiri Thanks again for your comments. I have addressed them except for item3 "run some kind of perf test". For item2 "the final path for the recovery indexes", you can see some test result below.
make check apps=couch tests=deleted_filename_test_. Would you please take a further look?

======================== EUnit ========================
couch_file:758: should_create_proper_deleted_filename (/srv/data/dbname.couch)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/.dbname_design/mrview/3133e28517e89a3e11435dd5ac4ad85a.view)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/shards/00000000-1fffffff/dbname.1458336317.couch)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/.shards/00000000-1fffffff/dbname.1458336317_design)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/.shards/00000000-1fffffff/dbname.1458336317_design/mrview/3133e28517e89a3e11435dd5ac4ad85a.view)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/.recovery/shards/00000000-1fffffff/dbname.1499329402_design)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/.recovery/shards/00000000-1fffffff/dbname.1499329402_design/mrview/8fabddcb28f501d6764afd7def3bd352.view)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/db.name.couch)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/.db.name_design/mrview/3133e28517e89a3e11435dd5ac4ad85a.view)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/shards/00000000-1fffffff/db.name.1458336317.couch)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/.shards/00000000-1fffffff/db.name.1458336317_design)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/.shards/00000000-1fffffff/db.name.1458336317_design/mrview/3133e28517e89a3e11435dd5ac4ad85a.view)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/.recovery/shards/00000000-1fffffff/db.name.1499329402_design)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/.recovery/shards/00000000-1fffffff/db.name.1499329402_design/mrview/8fabddcb28f501d6764afd7def3bd352.view)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/user/dbname.couch)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/.user/dbname_design/mrview/3133e28517e89a3e11435dd5ac4ad85a.view)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/shards/00000000-1fffffff/user/dbname.1458336317.couch)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/.shards/00000000-1fffffff/user/dbname.1458336317_design)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/.shards/00000000-1fffffff/user/dbname.1458336317_design/mrview/3133e28517e89a3e11435dd5ac4ad85a.view)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/.recovery/shards/00000000-1fffffff/user/dbname.1499329402_design)...ok
couch_file:758: should_create_proper_deleted_filename (/srv/data/.recovery/shards/00000000-1fffffff/user/dbname.1499329402_design/mrview/8fabddcb28f501d6764afd7def3bd352.view)...ok
=======================================================
  All 21 tests passed.

davisp

Mostly minor nits other than the path handling code.

davisp · 2017-07-17T16:36:46Z

src/couch/src/couch_file.erl

+rename_dir(RootDelDir, Original, DbName) ->
+    DbDir = binary_to_list(DbName) ++ "_design",
+    [DbPureName | _R] = lists:reverse(filename:split(binary_to_list(DbName))),
+    Deleted_Index_Dir = filename:join(


Remove the underscores in variable names.

davisp · 2017-07-17T16:38:41Z

src/couch/src/couch_file.erl

@@ -264,6 +265,19 @@ rename_file(Original) ->
        Else -> Else
    end.

+rename_dir(RootDelDir, Original, DbName) ->
+    DbDir = binary_to_list(DbName) ++ "_design",
+    [DbPureName | _R] = lists:reverse(filename:split(binary_to_list(DbName))),


What is this logic doing? Its not making sense to me why we'd be looking at just the last element of the DbName.

The logic of above line is to get pure dbname with epochs suffix.

Let me give example:

RootDelDir = /srv/view_index DbDir = shards/e0000000-ffffffff/testdb1.1500350972_design Original = /srv/view_index/.shards/e0000000-ffffffff/testdb1.1500350972_design DBName = shards/e0000000-ffffffff/testdb1.1500350972

In this PR, we experienced two evolutions.

First, we select recovery directory, like filename:join([RootDelDir, ".recovery", DbDir]),. Thus, the constructed directory looks like /srv/view_index/.recovery/shards/e0000000-ffffffff/testdb1.1500350972_design. This is feasible.

However, we want to group these moved view directories and files. So you can see that current approach is filename:join([RootDelDir, ".recovery", DbPureName, DbDir]),. The DbPureName looks like testdb1.1500350972 and doesn't contains shards/e0000000-ffffffff. Thus, for view files belonging to the same database, they can be easily managed by using such directory structure. There are two points which are considered:

for database which are deleted and created using same name, their epochs are different. This will not cause confusion.

for database with the same name from different user, in most situation, their epochs are different. Even if their epochs are the same, they can be still managed because they will be put into different subdirectory, like
/srv/view_index/.recovery/testdb1.1500350972/shards/e0000000-ffffffff/user1/testdb1.1500350972_design and
/srv/view_index/.recovery/testdb1.1500350972/shards/e0000000-ffffffff/user2/testdb1.1500350972_design

Where was the discussion on grouping them like that? I don't see any immediate reason why the first approach of just mirroring the shards/$range/dbname/_design... hierarchy under the .recovery directory.

And the particular case I was referring to is if your dbname has an embedded / in it which is feasible. Because given your example it reads to me like if we had a dbname <<"foo/bar">> then we'd end up with files under .recovery/bar/shards/... which would be wrong as far as I can tell.

Also, while users is a Cloudant specific issue it doesn't change the logic here as if you had a database name hierarchy you'd end up just chopping off the last element for the group which seems like it'd be super confusing to any administrator trying to use file recovery. You'd basically have to write the same scripts to search for files as you would without the whole grouping effort which makes me think there's not much benefit to doing it in the first place.

The discussion was reflected in summary of this commit (9b1dbfd).

- Introduce dbname directory between .recovery and shards so that all view files belonging to the same db can be located at the the central place. Thus, these view files can be easily sorted by atime at /srv/data/.recovery level. When these view files from same db needs to be recovered, they can be easily moved back to view directory.

However, considering the user information in DBName and situation where "/" is included in dbname, I changed back to /srv/view_index/.recovery/shards/e0000000-ffffffff/testdb1.1500350972_design approach.

davisp · 2017-07-17T16:39:45Z

src/couch/src/couch_file.erl

@@ -264,6 +265,19 @@ rename_file(Original) ->
        Else -> Else
    end.

+rename_dir(RootDelDir, Original, DbName) ->


This function does not seem to be handling databases that may have multiple path elements (ie, a DbName with a '/' in it) like you do for cloudant-labs/hastings#3.

The situation where DbName can contains "/" was considered. For details, please see response to next comment.

davisp · 2017-07-17T16:40:47Z

src/couch/src/couch_file.erl

+        true when Context == delete ->
+            DbName = couch_util:get_value(db_name, Options),
+            rename_dir(RootDelDir, Dir, DbName);
+        true -> rename_file(Dir);


Wouldn't this be when Context == compact, thus we'd want to delete the file (via moving to the .delete directory if enabled).

case EnableRecovery of true when Context == delete -> DbName = couch_util:get_value(db_name, Options), rename_dir(RootDelDir, Dir, DbName); true -> rename_file(Dir); false -> delete_dir(RootDelDir, Dir)

From above, you can see that there are three conditions:

the first one: EnableRecovery == true and Context == delete
under this situation, we want to move index to .recovery directory

the third one: EnableRecovery == false
under this situation, we want to delete the file (either delete in place or move it to .delete directory

If we are talking about the second situation where EnableRecovery == true and Context == compact, we still want to keep original logic, i.e. rename view file in place.

Ah, gotchya. For some reason I was thinking that was somewhere else but I forgot what module i was reading.

davisp · 2017-07-17T16:41:06Z

src/couch/src/couch_file.erl

@@ -317,6 +335,9 @@ init_delete_dir(RootDir) ->
    end),
    ok.

+init_recovery_dir(RootDir) ->
+    Dir = filename:join(RootDir,".recovery"),


Missing space after comma here and the next line.

davisp · 2017-07-17T16:42:35Z

src/couch/src/couch_file.erl

+            % by splitting Format String using ~s.
+            TildeSOccNum = erlang:length(binary:split(
+                list_to_binary(Format),
+                list_to_binary("~s"),


Why not just write <<"~s">> here?

davisp · 2017-07-17T16:43:12Z

src/couch/src/couch_file.erl

    ],
    lists:flatmap(fun(DbName) ->
        lists:map(fun(Format) ->
-            filename:join("/srv/data", io_lib:format(Format, [DbName]))
+            % calculate the occurrence of "~s" in the Format string
+            % by splitting Format String using ~s.


This comment would be better as something like:

Count how many times we need to specify the database name by splitting on the ~s formatter.

davisp · 2017-07-17T16:45:25Z

src/couch/src/couch_file.erl

+                list_to_binary("~s"),
+                [global])
+            ) - 1,
+            if TildeSOccNum == 1 ->


TildeSOccNum is a fairly confusing name. Something like "ArgCount".

Using if probably isn't the cleanest approach here. I'd do something like:

Args = case ArgCount of 1 -> [DbName]; 2 -> [DbName, DbName] end, filename:join("/srv/data/", io_lib:format(Format, Args)

jiangphcn · 2017-07-18T04:40:24Z

@davisp Hi Paul, thanks for your review and comments. I addressed most of comments. For other comments, I leave some response. Would you please check? Thanks again

jiangphcn · 2017-07-19T03:47:19Z

@davisp Hi Paul, I removed dbname directory between .recovery and shards in new commit. Would you please take another look? Thanks

davisp

Super minor variable name nit. +1 once that's changed.

davisp · 2017-07-21T16:14:45Z

src/couch/src/couch_file.erl

@@ -264,6 +265,18 @@ rename_file(Original) ->
        Else -> Else
    end.

+rename_dir(RootDelDir, Original, DbName) ->
+    DbDir = binary_to_list(DbName) ++ "_design",
+    DeletedIndexDir = filename:join(


Minor nit, this should be RenamedIndexDir or future us might get confused why its called Deleted but isn't actually getting deleted.

jiangphcn · 2017-07-22T04:37:54Z

Thanks @davisp for your review. I have changed name and could you please take yet another look when getting time? Thanks again.

davisp · 2017-07-24T15:19:36Z

+1

wohali · 2017-07-24T19:16:32Z

Please do not merge this to master until after the 2.1 branch is created. This should happen in the next 7 days.

wohali · 2017-08-08T21:12:25Z

You may merge this one when ready. Thanks for waiting! :)

Bugzid 86318

cdlwjing · 2017-08-23T06:13:05Z

According to the discussion with @jiangphcn, I tried to run some test cases to compare the performance difference between two designs(the current design and the new design in this PR). From the test result, I can see there are some perf improvement with new design. There is a report with all test details under: https://github.com/cloudant/couchdb/blob/index-delete-new-design-testreport/test_report/index_delete_comparision_report.ipynb

wohali · 2017-09-20T08:12:22Z

Go ahead and merge this when ready @jiangphcn .

jiangphcn · 2017-09-21T07:23:14Z

Thanks @wohali for your reminder and patience. There is some suggestion from Ops expert about not moving view index to /srv/view_index/.recovery directory. Otherwise, it will use more inode and there is risky that there is no atime to set for some mounted driver. I have to hold on merge this PR and will come back later. Thanks again.

wohali · 2017-10-26T02:19:06Z

@jiangphcn it's been a month. If we're still not sure what to do here, could we close this PR and open a new PR in the future instead, when you're ready? Thanks.

jiangphcn · 2017-10-26T02:26:52Z

yes, let me close this PR @wohali

jiangphcn · 2017-10-26T02:29:24Z

@wohali It looks that I don't have permission to close this PR. Can you help close this PR? Thanks

jiangphcn · 2017-10-27T01:23:54Z

thanks @davisp for closing this PR.

jiangphcn changed the title ~~Move view index files to .view_deleted when db is deleted~~ [WIP] Move view index files to .view_deleted when db is deleted Jun 14, 2017

jiangphcn force-pushed the 86318-move-index-viewfiles-when-dbdeleted branch 3 times, most recently from 7750c35 to c183452 Compare June 19, 2017 03:49

jiangphcn force-pushed the 86318-move-index-viewfiles-when-dbdeleted branch from c183452 to 4baa263 Compare June 22, 2017 04:12

jiangphcn changed the title ~~[WIP] Move view index files to .view_deleted when db is deleted~~ Move view index files to .view_deleted when db is deleted Jun 22, 2017

eiri reviewed Jul 6, 2017

View reviewed changes

jiangphcn force-pushed the 86318-move-index-viewfiles-when-dbdeleted branch from f1fa568 to 57eb6d3 Compare July 6, 2017 09:18

davisp requested changes Jul 17, 2017

View reviewed changes

davisp requested changes Jul 21, 2017

View reviewed changes

jiangphcn changed the title ~~Move view index files to .view_deleted when db is deleted~~ Move view index files to .recovery when db is deleted Aug 16, 2017

jiangphcn force-pushed the 86318-move-index-viewfiles-when-dbdeleted branch 2 times, most recently from d7f54ce to dce33a7 Compare August 17, 2017 03:02

Move view index files to .view_deleted when db is deleted

dce33a7

Bugzid 86318

davisp closed this Oct 26, 2017

Move view index files to .recovery when db is deleted #597

Move view index files to .recovery when db is deleted #597

Conversation

jiangphcn commented Jun 14, 2017 • edited Loading

Overview

Testing recommendations

GitHub issue number

Related Pull Requests

Checklist

eiri 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

jiangphcn commented Jul 6, 2017

davisp 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiangphcn commented Jul 18, 2017

jiangphcn commented Jul 19, 2017

davisp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiangphcn commented Jul 22, 2017

davisp commented Jul 24, 2017

wohali commented Jul 24, 2017

wohali commented Aug 8, 2017

cdlwjing commented Aug 23, 2017

wohali commented Sep 20, 2017

jiangphcn commented Sep 21, 2017

wohali commented Oct 26, 2017

jiangphcn commented Oct 26, 2017

jiangphcn commented Oct 26, 2017

jiangphcn commented Oct 27, 2017

jiangphcn commented Jun 14, 2017 •

edited

Loading