Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Background database deletion #2857

Merged
merged 1 commit into from
May 13, 2020
Merged

Background database deletion #2857

merged 1 commit into from
May 13, 2020

Conversation

jiangphcn
Copy link
Contributor

@jiangphcn jiangphcn commented Apr 30, 2020 •
edited
Loading

Overview

Allow to permanently delete those soft-deleted databases under background. In general, the soft-deleted database can be kept from several hours to several days. Later, they need to be deleted to reclaim disk space. So one criteria can be specified to decide how to select which soft-deleted databases to be deleted.

Testing recommendations

make check apps=fabric tests=crud_test_,scheduled_db_remove_error_test_

INFO:  Cover compiling /Users/jiangph/fdb/fdb-dbcore-0413/fdb-dbcore/src/couchdb/src/fabric
======================== EUnit ========================
Test scheduled database remove operations
  fabric2_db_crud_tests:77: scheduled_db_remove_error_test_ (scheduled_remove_deleted_dbs_with_error)...[0.026 s] ok
[os_mon] cpu supervisor port (cpu_sup): Erlang has closed
  [done in 0.082 s]
Test database CRUD operations
  fabric2_db_crud_tests:37: crud_test_ (create_db)...[0.031 s] ok
  fabric2_db_crud_tests:38: crud_test_ (open_db)...[0.015 s] ok
  fabric2_db_crud_tests:39: crud_test_ (delete_db)...[0.023 s] ok
  fabric2_db_crud_tests:40: crud_test_ (recreate_db)...[0.058 s] ok
  fabric2_db_crud_tests:41: crud_test_ (undelete_db)...[0.039 s] ok
  fabric2_db_crud_tests:42: crud_test_ (remove_deleted_db)...[0.044 s] ok
  fabric2_db_crud_tests:43: crud_test_ (scheduled_remove_deleted_db)...[1.312 s] ok
  fabric2_db_crud_tests:44: crud_test_ (scheduled_remove_deleted_dbs)...[2.016 s] ok
  fabric2_db_crud_tests:45: crud_test_ (old_db_handle)...[0.163 s] ok
  fabric2_db_crud_tests:46: crud_test_ (list_dbs)...[0.024 s] ok
  fabric2_db_crud_tests:47: crud_test_ (list_dbs_user_fun)...[0.014 s] ok
  fabric2_db_crud_tests:48: crud_test_ (list_dbs_user_fun_partial)...ok
  fabric2_db_crud_tests:49: crud_test_ (list_dbs_info)...[0.036 s] ok
  fabric2_db_crud_tests:50: crud_test_ (list_dbs_info_partial)...ok
  fabric2_db_crud_tests:51: crud_test_ (list_dbs_tx_too_old)...[0.076 s] ok
  fabric2_db_crud_tests:52: crud_test_ (list_dbs_info_tx_too_old)...[0.772 s] ok
  fabric2_db_crud_tests:53: crud_test_ (list_deleted_dbs_info)...[0.038 s] ok
  fabric2_db_crud_tests:54: crud_test_ (list_deleted_dbs_info_user_fun)...[0.026 s] ok
  fabric2_db_crud_tests:55: crud_test_ (list_deleted_dbs_info_user_fun_partial)...ok
  fabric2_db_crud_tests:56: crud_test_ (list_deleted_dbs_info_with_timestamps)...[2.284 s] ok
  fabric2_db_crud_tests:57: crud_test_ (get_info_wait_retry_on_tx_too_old)...[0.081 s] ok
  fabric2_db_crud_tests:58: crud_test_ (get_info_wait_retry_on_tx_abort)...[0.080 s] ok
[os_mon] cpu supervisor port (cpu_sup): Erlang has closed
  [done in 8.509 s]
=======================================================
  All 23 tests passed.
Cover analysis: /Users/jiangph/fdb/fdb-dbcore-0413/fdb-dbcore/src/couchdb/src/fabric/.eunit/index.html

Related Issues or Pull Requests

Checklist

@jiangphcn jiangphcn marked this pull request as ready for review May 11, 2020 06:43
@jiangphcn jiangphcn force-pushed the background-db-deletion branch 4 times, most recently from 43b0caa to 2144b30 Compare May 11, 2020 07:12
nickva
nickva reviewed May 11, 2020
View reviewed changes
src/fabric/src/fabric2_db_expiration.erl Outdated
end,
{ok, NewAcc}
end,
{ok, _Infos} = fabric2_db:list_deleted_dbs_info(Callback, [], []),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could technically run longer than 5 seconds so we should use the {restart_tx, true} option here.

@nickva
Copy link
Contributor

nickva commented May 11, 2020

@jiangphcn look great, Peng Hui. Nice work! See a few minor updates and a simplification in regards to batching and it should be good to go.

nickva
nickva reviewed May 13, 2020
View reviewed changes
src/fabric/src/fabric2_db_expiration.erl Outdated
ScheduleSec = schedule_sec(),
Opts = #{max_sched_time => Now + min(ScheduleSec div 3, 15)},
case couch_jobs:accept(?JOB_TYPE, Opts) of
% maybe handle timeout here, need to check the api
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove the comment since we don't need the timeout there

nickva
nickva reviewed May 13, 2020
View reviewed changes
src/fabric/src/fabric2_db_expiration.erl Outdated


process_expirations(#{} = Job, #{} = Data) ->
{ok, Infos} = fabric2_db:list_deleted_dbs_info([{restart_tx, true}]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could still use a callback here just so we don't have to load all the infos into memory.

It might be better to use the callback here and LastUpdateAt would be the accumulator value

nickva
nickva reviewed May 13, 2020
View reviewed changes
src/fabric/src/fabric2_db_expiration.erl Outdated
case Since >= timestamp_to_sec(TimeStamp) of
true ->
couch_log:notice("Permanently deleting ~p database with"
" timestamp ~p", [DbName, TimeStamp]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use a 4 space indent here since it's a continuation

nickva
nickva approved these changes May 13, 2020
View reviewed changes
Copy link
Contributor

@nickva nickva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 nice work, Peng Hui

@jiangphcn
background deletion for soft-deleted database
91becc1 
allow background job to delete soft-deleted database according to
specified criteria to release space. Once database is hard-deleted,
the data can't be fetched back.

Co-authored-by: Nick Vatamaniuc<[email protected]>
@jiangphcn
Copy link
Contributor Author

thanks @nickva so much for your kindly support and review!

@jiangphcn jiangphcn merged commit 1127908 into prototype/fdb-layer May 13, 2020
@jiangphcn jiangphcn deleted the background-db-deletion branch May 13, 2020 15:22
@jiangphcn jiangphcn mentioned this pull request May 21, 2020
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickva nickva nickva approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jiangphcn @nickva