You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2020/03/16 08:48:22 UTC
[GitHub] [couchdb] jiangphcn opened a new pull request #2666: [WIP]
soft-deletion for database
jiangphcn opened a new pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666
<!-- Thank you for your contribution!
Please file this form by replacing the Markdown comments
with your text. If a section needs no action - remove it.
Also remember, that CouchDB uses the Review-Then-Commit (RTC) model
of code collaboration. Positive feedback is represented +1 from committers
and negative is a -1. The -1 also means veto, and needs to be addressed
to proceed. Once there are no objections, the PR can be merged by a
CouchDB committer.
See: http://couchdb.apache.org/bylaws.html#decisions for more info. -->
## Overview
<!-- Please give a short brief for the pull request,
what problem it solves or how it makes things better. -->
## Testing recommendations
<!-- Describe how we can test your changes.
Does it provides any behaviour that the end users
could notice? -->
## Related Issues or Pull Requests
<!-- If your changes affects multiple components in different
repositories please put links to those issues or pull requests here. -->
## Checklist
- [ ] Code is written and works correctly
- [ ] Changes are covered by tests
- [ ] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400288974
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -346,12 +351,97 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric2_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ Timestamp = list_to_binary(fabric2_util:iso8601_timestamp()),
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, Timestamp},
+ LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeletedDbKey)) of
+ not_found ->
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
+ erlfdb:clear(Tx, DbKey);
+ _Val ->
+ erlang:error({deleted_database_exists, DbName})
+ end;
+ false ->
+ erlfdb:clear(Tx, DbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix)
+ end,
bump_metadata_version(Tx),
ok.
+deleted_dbs_info(#{} = Db0) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName}, LayerPrefix),
+ DeletedDbs = erlfdb:wait(erlfdb:get_range_startswith(Tx, DeletedDbKey)),
+ lists:foldl(fun({DbKey, DbPrefix}, Acc) ->
+ DBInfo = get_info_wait(get_info_future(Tx, DbPrefix)),
+ {?DELETED_DBS, DbName, DeletedTS} =
+ erlfdb_tuple:unpack(DbKey, LayerPrefix),
+ [{DeletedTS, DBInfo}| Acc]
+ end, [], DeletedDbs).
+
+
+undelete(#{} = Db0, TgtDbName, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+ DbKey = erlfdb_tuple:pack({?ALL_DBS, TgtDbName}, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DbKey)) of
+ Bin when is_binary(Bin) ->
+ {error, file_exists};
+ not_found ->
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found, invalid_timestamp});
+ DbPrefix ->
+ erlfdb:set(Tx, DbKey, DbPrefix),
+ erlfdb:clear(Tx, DeleteDbKey),
+ bump_metadata_version(Tx),
+ ok
+ end
+ end.
+
+
+delete_deleted(#{} = Db0, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0),
+
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
Review comment:
`DeletedDbKey` to match the variable name in all other places. And there's an extra space after the `=` operator.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r401770683
##########
File path: src/chttpd/test/eunit/chttpd_deleted_dbs_test.erl
##########
@@ -0,0 +1,206 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(chttpd_deleted_dbs_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+
+-define(USER, "chttpd_db_test_admin").
+-define(PASS, "pass").
+-define(AUTH, {basic_auth, {?USER, ?PASS}}).
+-define(CONTENT_JSON, {"Content-Type", "application/json"}).
+
+
+setup() ->
+ Hashed = couch_passwords:hash_admin_password(?PASS),
+ ok = config:set("admins", ?USER, ?b2l(Hashed), _Persist=false),
+ Addr = config:get("chttpd", "bind_address", "127.0.0.1"),
+ Port = mochiweb_socket_server:get(chttpd, port),
+ lists:concat(["http://", Addr, ":", Port, "/"]).
+
+teardown(_Url) ->
+ ok = config:delete("couchdb", "enable_database_recovery", false),
+ ok = config:delete("admins", ?USER, _Persist=false).
+
+create_db(Url) ->
+ {ok, Status, _, _} = test_request:put(Url, [?CONTENT_JSON, ?AUTH], "{}"),
+ ?assert(Status =:= 201 orelse Status =:= 202).
+
+delete_db(Url) ->
+ {ok, 200, _, _} = test_request:delete(Url, [?AUTH]).
+
+deleted_dbs_test_() ->
+ {
+ "chttpd deleted dbs tests",
+ {
+ setup,
+ fun chttpd_test_util:start_couch/0, fun chttpd_test_util:stop_couch/1,
+ {
+ foreach,
+ fun setup/0, fun teardown/1,
+ [
+ fun should_return_error_for_deleted_dbs/1,
+ fun should_list_deleted_dbs/1,
+ fun should_list_deleted_dbs_info/1,
+ fun should_undelete_db/1,
+ fun should_remove_deleted_db/1,
+ fun should_undelete_db_to_target_db/1,
+ fun should_not_undelete_db_to_existing_db/1
+ ]
+ }
+ }
+ }.
+
+
+should_return_error_for_deleted_dbs(Url) ->
+ ?_test(begin
+ create_and_delete_db(Url),
+ NewDoc = "{\"keys\": [\"db1\"]}",
+ {ok, Code, _, ResultBody} = test_request:post(Url ++ "/_deleted_dbs/",
+ [?CONTENT_JSON, ?AUTH], NewDoc),
+
+ {Body} = jiffy:decode(ResultBody),
+ [
+ ?assertEqual(<<"bad_request">>,
+ couch_util:get_value(<<"error">>, Body)),
+ ?assertEqual(400, Code)
+ ]
Review comment:
Your assertions in an array here seems to be copying some bad eunit examples from other places. Since we're in the `?_test/1` macro you don't need to return a list of assertions. You can just call them directly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400381324
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -346,12 +351,97 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric2_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ Timestamp = list_to_binary(fabric2_util:iso8601_timestamp()),
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, Timestamp},
+ LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeletedDbKey)) of
+ not_found ->
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
+ erlfdb:clear(Tx, DbKey);
+ _Val ->
+ erlang:error({deleted_database_exists, DbName})
+ end;
+ false ->
+ erlfdb:clear(Tx, DbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix)
+ end,
bump_metadata_version(Tx),
ok.
+deleted_dbs_info(#{} = Db0) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName}, LayerPrefix),
+ DeletedDbs = erlfdb:wait(erlfdb:get_range_startswith(Tx, DeletedDbKey)),
+ lists:foldl(fun({DbKey, DbPrefix}, Acc) ->
+ DBInfo = get_info_wait(get_info_future(Tx, DbPrefix)),
+ {?DELETED_DBS, DbName, DeletedTS} =
+ erlfdb_tuple:unpack(DbKey, LayerPrefix),
+ [{DeletedTS, DBInfo}| Acc]
+ end, [], DeletedDbs).
+
+
+undelete(#{} = Db0, TgtDbName, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+ DbKey = erlfdb_tuple:pack({?ALL_DBS, TgtDbName}, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DbKey)) of
+ Bin when is_binary(Bin) ->
+ {error, file_exists};
+ not_found ->
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found, invalid_timestamp});
+ DbPrefix ->
+ erlfdb:set(Tx, DbKey, DbPrefix),
+ erlfdb:clear(Tx, DeleteDbKey),
+ bump_metadata_version(Tx),
+ ok
+ end
+ end.
+
+
+delete_deleted(#{} = Db0, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0),
+
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found});
+ DbPrefix ->
+ erlfdb:clear(Tx, DeleteDbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix),
+ bump_metadata_version(Tx)
Review comment:
Ooooh, good call on db versions.
We should call `bump_db_version` after soft deletion and undeletion so that any inflight requests will fail properly.
I don't think hard deletion or snapshot deletion require that check since clearing the key would create a write conflict, no need to change it just to delete it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r395117557
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -314,12 +317,65 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ {Mega, Secs, _} = os:timestamp(),
+ NowSecs = Mega * 1000000 + Secs,
Review comment:
Makes sense. There is a balance here between using a non-standard format for time and the timestamp being an identifier of an db instance we want to restore. That is on one hand it might be nice to use https://en.wikipedia.org/wiki/ISO_8601 format (`2007-03-01T13:00:00Z` for ex.), however since we'd be passing that in the URL path we'd have to deal with `:` being a reserved character and encode it and all that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] mikerhodes commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
mikerhodes commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-604431455
> Also for the `POST /_deleted_dbs/_undelete` I'll also note that we do have a set of system database names that start with an underscore (`_dbs/_nodes/_users`). Obviously we don't have an `_undelete` database but it is technically mixing that API endpoint into the valid range of database names.
Isn't this the same for adding a `/_undelete_db` into the `/` namespace? In both cases you're basically creating a new reserved name in the list of databases, although there is certainly a lot of prior art for this in `/` so it wouldn't be as surprising and likely to be forgotten later.
While I prefer keeping new `_` endpoints in `/` to a minimum, I can live with having a few new endpoints in the `/` namespace. Before we make the final call, one additional option which keeps things contained to a single new entry in `/` while removing the dbname vs. action ambiguity @davisp mentioned, and keeping things somewhat RESTful, would be to extract the `dbs` bit into a further sub-namespace:
1. `GET /_deleted_dbs/dbs`
1. `GET /_deleted_dbs/dbs/DbName`
1. `POST /_deleted_dbs/undelete`
Alternatively you could have `/_deleted/dbs` (etc.) if you wanted to avoid the `dbs` repetition, though I feel that `_deleted` is a bit generic for my tastes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] ricellis commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
ricellis commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-604325420
> Obviously we don't have an `_undelete` database but it is technically mixing that API endpoint into the valid range of database names.
Agree that is probably better avoided.
I was thinking that maybe we could keep the functionality in the `_deleted_dbs` namespace by `POST /_deleted_dbs` with something like this:
```json
{
"undelete": {
"source": "source_database_name",
"source_timestamp": "2020-03-24T12:00:00",
"target": "target_database_name"
}
}
```
but ultimately I think that probably ends up more opaque than just using differently named endpoint `POST /_undelete_db` so I'm OK with `_undelete_db`.
@mikerhodes as the initial proposer of the `_deleted_dbs/_restore` pattern are you OK with `POST /_undelete_db`?
Unrelated to the endpoint name the example shows no TZ data in the ISO-8601 timestamp, is that an accurate representation of the output/input we're expecting?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r401764934
##########
File path: src/chttpd/test/eunit/chttpd_deleted_dbs_test.erl
##########
@@ -0,0 +1,206 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(chttpd_deleted_dbs_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+
+-define(USER, "chttpd_db_test_admin").
+-define(PASS, "pass").
+-define(AUTH, {basic_auth, {?USER, ?PASS}}).
+-define(CONTENT_JSON, {"Content-Type", "application/json"}).
+
+
+setup() ->
+ Hashed = couch_passwords:hash_admin_password(?PASS),
+ ok = config:set("admins", ?USER, ?b2l(Hashed), _Persist=false),
+ Addr = config:get("chttpd", "bind_address", "127.0.0.1"),
+ Port = mochiweb_socket_server:get(chttpd, port),
+ lists:concat(["http://", Addr, ":", Port, "/"]).
+
Review comment:
Minor nit but you're missing a second blank line before these first few functions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r395123503
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -172,7 +174,8 @@ create(#{} = Db0, Options) ->
% Eventually DbPrefix will be HCA allocated. For now
% we're just using the DbName so that debugging is easier.
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- DbPrefix = erlfdb_tuple:pack({?DBS, DbName}, LayerPrefix),
+ #{db_prefix_allocator := DBPrefixAllocator} = erlfdb_directory:root(),
Review comment:
We should probably not mix CouchDB internals with the erlfdb library. At first glance I would expect us not to touch the erlfdb library with this work. `Db` in `erlfdb`-land is an FDB database but here we are dealing with CouchDb `Db` instances so it already gets confusing.
Would this work `erlfdb_hca:create(erlfdb_tuple:pack({?HCA}, LayerPrefix))` to create an allocator instance for our layer prefix only. We are not going for full on FDB directories and nodes just need a short and unique prefix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r401804248
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -365,6 +364,65 @@ exists(#{name := DbName} = Db) when is_binary(DbName) ->
end.
+undelete(#{} = Db0, TgtDbName, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+ DbKey = erlfdb_tuple:pack({?ALL_DBS, TgtDbName}, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DbKey)) of
+ Bin when is_binary(Bin) ->
+ file_exists;
+ not_found ->
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found});
+ DbPrefix ->
+ erlfdb:set(Tx, DbKey, DbPrefix),
+ erlfdb:clear(Tx, DeleteDbKey),
+ bump_db_version(#{
+ tx => Tx,
+ db_prefix => DbPrefix
+ }),
+ ok
+ end
+ end.
+
+
+remove_deleted_db(#{} = Db0, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeletedDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeletedDbKey)) of
+ not_found ->
+ erlang:error({not_found});
Review comment:
Whatever we pick in undelete should be mirrored in this function as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r396611561
##########
File path: src/chttpd/src/chttpd_misc.erl
##########
@@ -146,6 +147,65 @@ handle_all_dbs_req(#httpd{method='GET'}=Req) ->
handle_all_dbs_req(Req) ->
send_method_not_allowed(Req, "GET,HEAD").
+handle_deleted_dbs_req(#httpd{path_parts=[<<"_deleted_dbs">>]}=Req) ->
+ deleted_dbs_req(Req);
+handle_deleted_dbs_req(#httpd{path_parts=[<<"_deleted_dbs">>|DbName]}=Req) ->
+ deleted_dbs_info_req(Req, list_to_binary(DbName)).
+
+deleted_dbs_req(#httpd{method='GET'}=Req) ->
+ #mrargs{
+ start_key = StartKey,
+ end_key = EndKey,
+ direction = Dir,
+ limit = Limit,
+ skip = Skip
+ } = couch_mrview_http:parse_params(Req, undefined),
+
+ Options = [
+ {start_key, StartKey},
+ {end_key, EndKey},
+ {dir, Dir},
+ {limit, Limit},
+ {skip, Skip}
+ ],
+
+ % Eventually the Etag for this request will be derived
+ % from the \xFFmetadataVersion key in fdb
+ Etag = <<"foo">>,
+
+ {ok, Resp} = chttpd:etag_respond(Req, Etag, fun() ->
+ {ok, Resp} = chttpd:start_delayed_json_response(Req, 200, [{"ETag",Etag}]),
+ Callback = fun all_dbs_callback/2,
+ Acc = #vacc{req=Req,resp=Resp},
+ fabric2_db:list_deleted_dbs(Callback, Acc, Options)
+ end),
+ case is_record(Resp, vacc) of
+ true -> {ok, Resp#vacc.resp};
+ _ -> {ok, Resp}
+ end;
+deleted_dbs_req(Req) ->
+ send_method_not_allowed(Req, "GET,HEAD").
+
+deleted_dbs_info_req(#httpd{user_ctx=Ctx}=Req, DbName) ->
+ couch_httpd:verify_is_server_admin(Req),
+ case fabric2_db:deleted_dbs_info(DbName, [{user_ctx, Ctx}]) of
+ {ok, Result} ->
+ {ok, Resp} = chttpd:start_json_response(Req, 200),
+ send_chunk(Resp, "["),
+ lists:foldl(fun({Timestamp, Info}, AccSeparator) ->
+ Json = ?JSON_ENCODE({[{key, DbName}, {timestamp, Timestamp},
Review comment:
Lists of key/value pairs that span multiple lines should have one pair per line to improve readability. Also, do we need to consider that this is a different structure than most of our list operations like this? We generally have `key`, `value`, `id`, `doc`, and `error`. I'd think moving `{Info}` to `value` would be straight forward. For `Timestamp` I've got nothing super awesome beyond putting it inside Info in the deleted_dbs_info implementation rather than returning `{Timestamp, Info}`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] mikerhodes edited a comment on issue #2666: soft-deletion
for database
Posted by GitBox <gi...@apache.org>.
mikerhodes edited a comment on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-605893618
I agree with @davisp's suggestion in https://github.com/apache/couchdb/pull/2666#issuecomment-605109017. I've also thumbs-up'd it to doubly make the point 😄
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on a change in pull request #2666:
[WIP] soft-deletion for database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r394975071
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -314,12 +317,65 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ {Mega, Secs, _} = os:timestamp(),
+ NowSecs = Mega * 1000000 + Secs,
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, NowSecs},
+ LayerPrefix),
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
+ erlfdb:clear(Tx, DbKey);
+ false ->
+ erlfdb:clear(Tx, DbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix)
+ end,
bump_metadata_version(Tx),
ok.
+deleted_dbs_info(#{} = Db0) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName}, LayerPrefix),
+ DeletedDbs = erlfdb:wait(erlfdb:get_range_startswith(Tx, DeletedDbKey)),
+ lists:foldl(fun({DbKey, DbPrefix}, Acc) ->
+ DBInfo = get_info_wait(get_info_future(Tx, DbPrefix)),
+ {?DELETED_DBS, DbName, DeletedTS} =
+ erlfdb_tuple:unpack(DbKey, LayerPrefix),
+ [DeletedTS| Acc]
+ end, [], DeletedDbs).
+
+
+restore(#{} = Db0, [DeletedTS|_]) ->
Review comment:
No, trailing path items will not be used. Just think not too strict here. But I can change to be strict by removing `|_`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] nickva edited a comment on issue #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
nickva edited a comment on issue #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-601266656
Thinking about this some more I wonder if it would make sense to separate enabling HCA from soft-deletion. Those are rather independent features. So for example, we'd have a PR to enable HCA. We'd test that make sure that works. Then, as a separate piece of work, we'd enable soft-deletion. Would that be reasonable?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r402295434
##########
File path: src/chttpd/test/eunit/chttpd_deleted_dbs_test.erl
##########
@@ -0,0 +1,206 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(chttpd_deleted_dbs_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+
+-define(USER, "chttpd_db_test_admin").
+-define(PASS, "pass").
+-define(AUTH, {basic_auth, {?USER, ?PASS}}).
+-define(CONTENT_JSON, {"Content-Type", "application/json"}).
+
+
+setup() ->
+ Hashed = couch_passwords:hash_admin_password(?PASS),
+ ok = config:set("admins", ?USER, ?b2l(Hashed), _Persist=false),
+ Addr = config:get("chttpd", "bind_address", "127.0.0.1"),
+ Port = mochiweb_socket_server:get(chttpd, port),
+ lists:concat(["http://", Addr, ":", Port, "/"]).
+
+teardown(_Url) ->
+ ok = config:delete("couchdb", "enable_database_recovery", false),
+ ok = config:delete("admins", ?USER, _Persist=false).
+
+create_db(Url) ->
+ {ok, Status, _, _} = test_request:put(Url, [?CONTENT_JSON, ?AUTH], "{}"),
+ ?assert(Status =:= 201 orelse Status =:= 202).
+
+delete_db(Url) ->
+ {ok, 200, _, _} = test_request:delete(Url, [?AUTH]).
+
+deleted_dbs_test_() ->
+ {
+ "chttpd deleted dbs tests",
+ {
+ setup,
+ fun chttpd_test_util:start_couch/0, fun chttpd_test_util:stop_couch/1,
+ {
+ foreach,
+ fun setup/0, fun teardown/1,
+ [
+ fun should_return_error_for_deleted_dbs/1,
+ fun should_list_deleted_dbs/1,
+ fun should_list_deleted_dbs_info/1,
+ fun should_undelete_db/1,
+ fun should_remove_deleted_db/1,
+ fun should_undelete_db_to_target_db/1,
+ fun should_not_undelete_db_to_existing_db/1
+ ]
+ }
+ }
+ }.
+
+
+should_return_error_for_deleted_dbs(Url) ->
+ ?_test(begin
+ create_and_delete_db(Url),
+ NewDoc = "{\"keys\": [\"db1\"]}",
+ {ok, Code, _, ResultBody} = test_request:post(Url ++ "/_deleted_dbs/",
+ [?CONTENT_JSON, ?AUTH], NewDoc),
+
+ {Body} = jiffy:decode(ResultBody),
+ [
+ ?assertEqual(<<"bad_request">>,
+ couch_util:get_value(<<"error">>, Body)),
+ ?assertEqual(400, Code)
+ ]
+ end).
+
+
+should_list_deleted_dbs(Url) ->
+ ?_test(begin
+ DbName1 = create_and_delete_db(Url),
+ DbName2 = create_and_delete_db(Url),
+ {ok, _, _, ResultBody} = test_request:get(Url ++ "/_deleted_dbs/",
+ [?CONTENT_JSON, ?AUTH]),
+ BodyJson = jiffy:decode(ResultBody),
+ ?assertEqual(true, lists:member(DbName1, BodyJson)),
+ ?assertEqual(true, lists:member(DbName2, BodyJson))
Review comment:
Okay, I added DBInfo for `GET _deleted_dbs` endpoint, like we did for `_deleted_dbs/DbName` endpoint. We turn on database recovery here https://github.com/apache/couchdb/pull/2666/files#diff-3fdd5966871338636ff72ba49490942fR223
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r394813352
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -314,12 +317,65 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ {Mega, Secs, _} = os:timestamp(),
+ NowSecs = Mega * 1000000 + Secs,
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, NowSecs},
+ LayerPrefix),
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
+ erlfdb:clear(Tx, DbKey);
+ false ->
+ erlfdb:clear(Tx, DbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix)
+ end,
bump_metadata_version(Tx),
ok.
+deleted_dbs_info(#{} = Db0) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName}, LayerPrefix),
+ DeletedDbs = erlfdb:wait(erlfdb:get_range_startswith(Tx, DeletedDbKey)),
+ lists:foldl(fun({DbKey, DbPrefix}, Acc) ->
+ DBInfo = get_info_wait(get_info_future(Tx, DbPrefix)),
Review comment:
I think we forgot to include `DBInfo` in the results?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] ricellis commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
ricellis commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-608502908
> `POST /_deleted_dbs` - Only accepts the undelete JSON body to *restore* the deleted db
Can I see an example of the expected body now? If `undelete` is the only "action" for `POST` is there any reason to call it out separately?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jaydoane commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
jaydoane commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-607996688
> I've also heard some concerns that having the `undelete` vs `delete` semantics of our `POST /_deleted_dbs` setup makes auditing traffic hard/impossible without introspecting the request body which seems like a legitimate critique. How about if we tweaked things to be like such:
>
> * `GET /_deleted_dbs` - List deleted dbs infos as currently.
>
> * `POST /_deleted_dbs` - Only accepts the `undelete` JSON body
>
> * `DELETE /_deleted_dbs/DbName?timestamp=2020-01-01T00:00:00` - Remove deleted db
I very much like this particular API, since it's trivial to derive three separate action names from method and path, unlike earlier versions. The corresponding names (eliding the ubiquitous `cloudantnosqldb.` prefix), I'm considering:
- `account-deleted-dbs.read`
- `account-deleted-dbs.undelete`
- `account-deleted-dbs.delete`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
davisp commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-607972201
I wasn't super fond of the chttpd tests for deleted dbs but not in a way specific enough to review very well. So I just took a quick pass through reformatting/styling etc on what looks better to me. Feel free to completely ignore this though as its purely style related. But in case you're interested, this is how I'd change things so that there's a lot less noise in that test module:
https://gist.github.com/davisp/257ea1631d443031f5c3995b70093bbe
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-603193963
thanks Paul @davisp for your review and comments. Could you please check 3d53721 I tried to use to address your comments?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on a change in pull request #2666:
[WIP] soft-deletion for database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r396049485
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -314,12 +317,65 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ {Mega, Secs, _} = os:timestamp(),
+ NowSecs = Mega * 1000000 + Secs,
Review comment:
hey @nickva, with the suggestion from @mikerhodes, the timestamp is specified in payload of POST instead of URL
```
POST /{db}/_restore
{ "sourceTS": "<deletedTS>", "destination": "animaldb"}
```
Thus, it is probably safe to use ISO_8601 format, like `2007-03-01T13:00:00Z` where `:` exists in payload.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-608411228
Thanks @davisp and @mikerhodes for your comments. Today I have one commit eefd115 which essentially implemented below API spec.
```
GET /_deleted_dbs - List deleted dbs infos
GET /_deleted_dbs?key=dbName - List specified deleted db infos
POST /_deleted_dbs - Only accepts the undelete JSON body to *restore* the deleted db
DELETE /_deleted_dbs/DbName?timestamp=2020-01-01T00:00:00 - Remove deleted db
```
From the functional set exposed to users, I think that it is similar to what @mikerhodes drafted in [1]. The difference is the API spec.
From user case and implementation's point of view, there are several points I want to mention.
1. GET /_deleted_dbs - List deleted dbs infos
We directly return dbs info instead of the deleted dbname here. The same case is for `GET /_deleted_dbs?key=dbName`. This is mainly following the approach in `_dbs_info`. I think that most likely user is not only interested in dbname, but also in some detailed information about deleted database.
```
{
"key": "db01",
"timestamp": "2020-04-03T12:09:15Z",
"value": {
"update_seq": "000000000000000000000000",
"doc_del_count": 0,
"doc_count": 0,
"sizes": {
"external": 2,
"views": 0
}
}
```
User can gradually use the progressive approach like mentioned in [1] to get the detailed information of the deleted db.
```
GET /_deleted_dbs
GET /_deleted_dbs/DbName
GET /_deleted_dbs/DbName/2020-01-01T00:00:00
```
However, looks that user most like should review information of the deletion time, doc number, disk size to decide to restore it or permanently delete it.
2. with regard to
```
GET /_deleted_dbs/DbName
GET /_deleted_dbs?key=DbName
```
In theory, we can implement both of them to get information of deleted db. @davisp also found one situation where we have to return 200 and [] if the specified db is not in deleted_db list when selecting `GET /_deleted_dbs/DbName`. This is not expected error code. So we switch to `GET /_deleted_dbs?key=DbName`.
Hope this can give some explanation how we are getting to the current status of API spec.
[1] https://gist.github.com/mikerhodes/39c66576eee468a5aeaa23c5715aec9a
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r396649958
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -79,6 +79,8 @@ handle_request(#httpd{path_parts=[DbName|RestParts],method=Method}=Req)->
end;
{_, []} ->
do_db_req(Req, fun db_req/2);
+ {_, [<<"_restore">>|_]} ->
Review comment:
Also, `_undelete` might be a good replacement given the mailing list freedback.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r396594839
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -177,10 +181,13 @@ create(#{} = Db0, Options) ->
layer_prefix := LayerPrefix
} = Db = ensure_current(Db0, false),
- % Eventually DbPrefix will be HCA allocated. For now
- % we're just using the DbName so that debugging is easier.
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- DbPrefix = erlfdb_tuple:pack({?DBS, DbName}, LayerPrefix),
+ DefDbPref = ?DEFAULT_DB_PREFIX,
+ AllDbPrefix = erlfdb_util:get(Options, db_prefix, DefDbPref),
+ DbId = erlfdb_tuple:pack({AllDbPrefix}, AllDbPrefix),
+ DbPrefixAllocator = erlfdb_hca:create(erlfdb_tuple:pack({DbId}, <<"hca">>)),
+ AllocPrefix = erlfdb_hca:allocate(DbPrefixAllocator, Tx),
+ DbPrefix = erlfdb_tuple:pack({?DBS, AllocPrefix}, LayerPrefix),
Review comment:
There's a lot of unnecessary going on here. You should just need something like:
```erlang
HCAKey = erlfdb_tuple:pack({?DB_HCA}, LayerPrefix),
HCA = erlfdb_hca:create(HCAKey),
DbId = erlfdb_hca:allocate(HCA),
DbPrefix = erlfdb_tuple:pack({?DBS, DbId}, LayerPrefix),
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on issue #2666: [WIP] soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
nickva commented on issue #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-601266656
Thinking about this some more I wonder if it would make sense to separate enabling HCA from soft-deletion. Those are rather independent features. So for example, we'd have a PR to enable HCA. We'd test make sure that works. Then, as a separate piece of work, we'd enable soft-deletion. Would that be reasonable?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
davisp commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-605109017
So, I apparently somehow managed to write an entire comment yesterday and then not post it (or I posted it to a random unrelated ticket that I can't find...)
I really don't like the `_deleted_dbs/dbs` approach. I'd vote for:
* `GET /_deleted_dbs`
* `GET /_deleted_dbs/DbName`
* `POST /_deleted_dbs` with payload:
```json
{
"undelete": {
"source": "source_database_name",
"source_timestamp": "2020-03-24T12:00:00",
"target": "target_database_name"
}
}
```
And then I also realized that we really need a "actually delete the deleted thing" API so one or both of:
* `DELETE /_deleted_dbs/DbName?timestamp=2020-03-24T12:00:00`
* `POST /_deleted_dbs` with payload:
```json
{
"delete": {
"source": "source_database_name",
"source_timestamp": "2020-03-24T12:00:00"
}
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
davisp commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-607954397
Also, a couple quick thoughts on the HTTP API. Looking at it a bit harder I realized that we've basically duplicated some functionality because these two requests should be functionally equivalent:
```
GET /_deleted_dbs/DbName`
GET /_deleted_dbs?key=DbName`
```
And I could see a convincing argument that the second form would be more "natural" within the existing APIs for views and so on.
I've also heard some concerns that having the `undelete` vs `delete` semantics of our `POST /_deleted_dbs` setup makes auditing traffic hard/impossible without introspecting the request body which seems like a legitimate critique. How about if we tweaked things to be like such:
* `GET /_deleted_dbs` - List deleted dbs infos as currently.
* `POST /_deleted_dbs` - Only accepts the `undelete` JSON body
* `DELETE /_deleted_dbs/DbName?timestamp=2020-01-01T00:00:00` - Remove deleted db
/cc @mikerhodes @ricellis @nickva
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r396593347
##########
File path: src/fabric/src/fabric2_db.erl
##########
@@ -213,6 +219,26 @@ delete(DbName, Options) ->
end.
+deleted_dbs_info(DbName, Options) ->
+ Options1 = lists:keystore(user_ctx, 1, Options, ?ADMIN_CTX),
+ Result = fabric2_fdb:transactional(DbName, Options1, fun(TxDb) ->
+ fabric2_fdb:deleted_dbs_info(TxDb)
+ end),
+ {ok, lists:reverse(Result)}.
+
+
+restore(DbName, DesDbName, TimeStamp, Options) ->
+ case validate_dbname(DesDbName) of
+ ok ->
+ Options1 = lists:keystore(user_ctx, 1, Options, ?ADMIN_CTX),
Review comment:
This isn't the right place to throw in ?ADMIN_CTX uncoditionally. That should be part of chttpd.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400289372
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -346,12 +351,97 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric2_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ Timestamp = list_to_binary(fabric2_util:iso8601_timestamp()),
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, Timestamp},
+ LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeletedDbKey)) of
+ not_found ->
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
+ erlfdb:clear(Tx, DbKey);
+ _Val ->
+ erlang:error({deleted_database_exists, DbName})
+ end;
+ false ->
+ erlfdb:clear(Tx, DbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix)
+ end,
bump_metadata_version(Tx),
ok.
+deleted_dbs_info(#{} = Db0) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName}, LayerPrefix),
+ DeletedDbs = erlfdb:wait(erlfdb:get_range_startswith(Tx, DeletedDbKey)),
+ lists:foldl(fun({DbKey, DbPrefix}, Acc) ->
+ DBInfo = get_info_wait(get_info_future(Tx, DbPrefix)),
+ {?DELETED_DBS, DbName, DeletedTS} =
+ erlfdb_tuple:unpack(DbKey, LayerPrefix),
+ [{DeletedTS, DBInfo}| Acc]
+ end, [], DeletedDbs).
+
+
+undelete(#{} = Db0, TgtDbName, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+ DbKey = erlfdb_tuple:pack({?ALL_DBS, TgtDbName}, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DbKey)) of
+ Bin when is_binary(Bin) ->
+ {error, file_exists};
+ not_found ->
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found, invalid_timestamp});
+ DbPrefix ->
+ erlfdb:set(Tx, DbKey, DbPrefix),
+ erlfdb:clear(Tx, DeleteDbKey),
+ bump_metadata_version(Tx),
+ ok
+ end
+ end.
+
+
+delete_deleted(#{} = Db0, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0),
+
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found});
+ DbPrefix ->
+ erlfdb:clear(Tx, DeleteDbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix),
+ bump_metadata_version(Tx)
Review comment:
I don't think we need to bump the metadata version here since we're not affecting `_all_dbs`? What do you think, @nickva?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-607339883
@davisp and @nickva could you please check whether there are other comments left?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400264211
##########
File path: src/fabric/src/fabric2_db.erl
##########
@@ -17,11 +17,18 @@
create/2,
open/2,
delete/2,
+ deleted_dbs_info/2,
+ undelete/4,
+ delete_deleted/3,
Review comment:
This API is getting a bit too specific I think. How about if we pass a `deleted_at` parameter to `delete/2`. So something like `fabric2_db:delete(DbName, [{deleted_at, <<"2020-03-29T12:00:00">>}])`.
For the `fabric2_fdb` API, if the logic gets too funky with combining into a single function then I'd name it `fabric2_fdb:remove_deleted_db(DbName, TimeStamp)`.
Also, in this export group lets move `deleted_dbs_info/2` down below `list_dbs_info/1,2,3`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r401769925
##########
File path: src/chttpd/test/eunit/chttpd_deleted_dbs_test.erl
##########
@@ -0,0 +1,206 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(chttpd_deleted_dbs_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+
+-define(USER, "chttpd_db_test_admin").
+-define(PASS, "pass").
+-define(AUTH, {basic_auth, {?USER, ?PASS}}).
+-define(CONTENT_JSON, {"Content-Type", "application/json"}).
+
+
+setup() ->
+ Hashed = couch_passwords:hash_admin_password(?PASS),
+ ok = config:set("admins", ?USER, ?b2l(Hashed), _Persist=false),
+ Addr = config:get("chttpd", "bind_address", "127.0.0.1"),
+ Port = mochiweb_socket_server:get(chttpd, port),
+ lists:concat(["http://", Addr, ":", Port, "/"]).
+
+teardown(_Url) ->
+ ok = config:delete("couchdb", "enable_database_recovery", false),
+ ok = config:delete("admins", ?USER, _Persist=false).
+
+create_db(Url) ->
+ {ok, Status, _, _} = test_request:put(Url, [?CONTENT_JSON, ?AUTH], "{}"),
+ ?assert(Status =:= 201 orelse Status =:= 202).
+
+delete_db(Url) ->
+ {ok, 200, _, _} = test_request:delete(Url, [?AUTH]).
+
+deleted_dbs_test_() ->
+ {
+ "chttpd deleted dbs tests",
+ {
+ setup,
+ fun chttpd_test_util:start_couch/0, fun chttpd_test_util:stop_couch/1,
+ {
+ foreach,
+ fun setup/0, fun teardown/1,
+ [
+ fun should_return_error_for_deleted_dbs/1,
+ fun should_list_deleted_dbs/1,
+ fun should_list_deleted_dbs_info/1,
+ fun should_undelete_db/1,
+ fun should_remove_deleted_db/1,
+ fun should_undelete_db_to_target_db/1,
+ fun should_not_undelete_db_to_existing_db/1
+ ]
+ }
+ }
+ }.
+
+
+should_return_error_for_deleted_dbs(Url) ->
+ ?_test(begin
+ create_and_delete_db(Url),
+ NewDoc = "{\"keys\": [\"db1\"]}",
+ {ok, Code, _, ResultBody} = test_request:post(Url ++ "/_deleted_dbs/",
+ [?CONTENT_JSON, ?AUTH], NewDoc),
Review comment:
And the error code would also be a 404 then.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r394817206
##########
File path: src/fabric/src/fabric_util.erl
##########
@@ -23,6 +23,8 @@
-export([validate_all_docs_args/2, validate_args/3]).
-export([upgrade_mrargs/1]).
-export([worker_ranges/1]).
+-export([do_recovery/0]).
Review comment:
Let's avoid using `fabric_util` and use `fabric2_util` for consistency. However, in this case if we only check the config value once from one module, let's keep it in that module as a local function for now.
Wonder if `recovery` or `soft_deletion` is a better name... The discussion so far has used the `soft_deletion` term, maybe we should use that as the config value? The section should probably be `[fabric]` just to keep consistent with other FDB related settings.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on a change in pull request #2666:
[WIP] soft-deletion for database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r394921035
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -314,12 +317,65 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ {Mega, Secs, _} = os:timestamp(),
+ NowSecs = Mega * 1000000 + Secs,
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, NowSecs},
+ LayerPrefix),
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
+ erlfdb:clear(Tx, DbKey);
+ false ->
+ erlfdb:clear(Tx, DbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix)
+ end,
bump_metadata_version(Tx),
ok.
+deleted_dbs_info(#{} = Db0) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName}, LayerPrefix),
+ DeletedDbs = erlfdb:wait(erlfdb:get_range_startswith(Tx, DeletedDbKey)),
+ lists:foldl(fun({DbKey, DbPrefix}, Acc) ->
+ DBInfo = get_info_wait(get_info_future(Tx, DbPrefix)),
Review comment:
Don't forget, but don't add it yet, and will add it soon with proposed API spec.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] mikerhodes edited a comment on issue #2666: soft-deletion
for database
Posted by GitBox <gi...@apache.org>.
mikerhodes edited a comment on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-608281551
- The main problem I can see for `GET /_deleted_dbs?key=DbName` is that we lose the mirror with including the database name like we have for `DELETE /_deleted_dbs/DbName?timestamp=2020-01-01T00:00:00`.
- I also wondered whether the `timestamp` is required for the `DELETE`? If so, passing it as part of the path would be more natural (which I'm aware was one of the original suggestions I wasn't so keen on :grin:).
- If we look to `_all_dbs` as a model (I pondered this as we're looking at databases and the API is much simpler to copy), it isn't correct that these are equivalent perhaps:
```
GET /_deleted_dbs/DbName
GET /_deleted_dbs?key=DbName
```
Because these return different values in the existing API:
```
GET /DbName
GET /_all_dbs?key=DbName
```
So there's a question here as to whether `/_deleted_dbs` is more like a `_all_dbs` or `_all_docs`? Are we talking databases or some other type of entity, that `/_deleted_dbs` is more like talking about a set of metadata about deleted databases rather than teh databases themselves.
A quick proposal with responses based on `_all_dbs` that is as REST as I could make it, with all the lessons from the discussion above tried to be taken onboard:
https://gist.github.com/mikerhodes/39c66576eee468a5aeaa23c5715aec9a
I think this could be trivially adapted to make the responses more `_all_docs`/view like if we wanted using the same paths.
I guess we have `_dbs_info` as a guide too, though that doesn't tell us what `/_deleted_dbs/DbName` should be...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400288627
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -346,12 +351,97 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric2_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ Timestamp = list_to_binary(fabric2_util:iso8601_timestamp()),
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, Timestamp},
+ LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeletedDbKey)) of
+ not_found ->
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
+ erlfdb:clear(Tx, DbKey);
+ _Val ->
+ erlang:error({deleted_database_exists, DbName})
+ end;
+ false ->
+ erlfdb:clear(Tx, DbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix)
+ end,
bump_metadata_version(Tx),
ok.
+deleted_dbs_info(#{} = Db0) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName}, LayerPrefix),
+ DeletedDbs = erlfdb:wait(erlfdb:get_range_startswith(Tx, DeletedDbKey)),
+ lists:foldl(fun({DbKey, DbPrefix}, Acc) ->
+ DBInfo = get_info_wait(get_info_future(Tx, DbPrefix)),
+ {?DELETED_DBS, DbName, DeletedTS} =
+ erlfdb_tuple:unpack(DbKey, LayerPrefix),
+ [{DeletedTS, DBInfo}| Acc]
+ end, [], DeletedDbs).
+
+
+undelete(#{} = Db0, TgtDbName, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+ DbKey = erlfdb_tuple:pack({?ALL_DBS, TgtDbName}, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DbKey)) of
+ Bin when is_binary(Bin) ->
+ {error, file_exists};
+ not_found ->
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found, invalid_timestamp});
+ DbPrefix ->
+ erlfdb:set(Tx, DbKey, DbPrefix),
+ erlfdb:clear(Tx, DeleteDbKey),
+ bump_metadata_version(Tx),
+ ok
+ end
+ end.
+
+
+delete_deleted(#{} = Db0, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0),
Review comment:
I'm pretty sure this needs to be `ensure_current(Db0, false)`. I'm surprised it works without disabling the db version check.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r399260472
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -346,12 +350,68 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric2_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ Timestamp = list_to_binary(fabric2_util:iso8601_timestamp()),
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, Timestamp},
+ LayerPrefix),
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
Review comment:
thanks @davisp for bringing up this corner case. I added some lines to make check. Also I made change on API naming. Could you please review again?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] mikerhodes commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
mikerhodes commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-608281551
- The main problem I can see for `GET /_deleted_dbs?key=DbName` is that we lose the mirror with including the database name like we have for `DELETE /_deleted_dbs/DbName?timestamp=2020-01-01T00:00:00`.
- I also wondered whether the `timestamp` is required for the `DELETE`? If so, passing it as part of the path would be more natural (which I'm aware was one of the original suggestions I wasn't so keen on :grin:).
- If we look to `_all_dbs` as a model (I pondered this as we're looking at databases and the API is much simpler to copy), this isn't correct:
```
GET /_deleted_dbs/DbName
GET /_deleted_dbs?key=DbName
```
Because these return different values:
```
GET /DbName
GET /_all_dbs?key=DbName
```
So there's a question here as to whether `/_deleted_dbs` is more like a `_all_dbs` or `_all_docs`? Are we talking databases or some other type of entity, that `/_deleted_dbs` is more like talking about a set of metadata about deleted databases rather than teh databases themselves.
A quick proposal with responses based on `_all_dbs` that is as REST as I could make it, with all the lessons from the discussion above tried to be taken onboard:
https://gist.github.com/mikerhodes/39c66576eee468a5aeaa23c5715aec9a
I think this could be trivially adapted to make the responses more `_all_docs`/view like if we wanted using the same paths.
I guess we have `_dbs_info` as a guide too, though that doesn't tell us what `/_deleted_dbs/DbName` should be...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r401769780
##########
File path: src/chttpd/test/eunit/chttpd_deleted_dbs_test.erl
##########
@@ -0,0 +1,206 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(chttpd_deleted_dbs_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+
+-define(USER, "chttpd_db_test_admin").
+-define(PASS, "pass").
+-define(AUTH, {basic_auth, {?USER, ?PASS}}).
+-define(CONTENT_JSON, {"Content-Type", "application/json"}).
+
+
+setup() ->
+ Hashed = couch_passwords:hash_admin_password(?PASS),
+ ok = config:set("admins", ?USER, ?b2l(Hashed), _Persist=false),
+ Addr = config:get("chttpd", "bind_address", "127.0.0.1"),
+ Port = mochiweb_socket_server:get(chttpd, port),
+ lists:concat(["http://", Addr, ":", Port, "/"]).
+
+teardown(_Url) ->
+ ok = config:delete("couchdb", "enable_database_recovery", false),
+ ok = config:delete("admins", ?USER, _Persist=false).
+
+create_db(Url) ->
+ {ok, Status, _, _} = test_request:put(Url, [?CONTENT_JSON, ?AUTH], "{}"),
+ ?assert(Status =:= 201 orelse Status =:= 202).
+
+delete_db(Url) ->
+ {ok, 200, _, _} = test_request:delete(Url, [?AUTH]).
+
+deleted_dbs_test_() ->
+ {
+ "chttpd deleted dbs tests",
+ {
+ setup,
+ fun chttpd_test_util:start_couch/0, fun chttpd_test_util:stop_couch/1,
+ {
+ foreach,
+ fun setup/0, fun teardown/1,
+ [
+ fun should_return_error_for_deleted_dbs/1,
+ fun should_list_deleted_dbs/1,
+ fun should_list_deleted_dbs_info/1,
+ fun should_undelete_db/1,
+ fun should_remove_deleted_db/1,
+ fun should_undelete_db_to_target_db/1,
+ fun should_not_undelete_db_to_existing_db/1
+ ]
+ }
+ }
+ }.
+
+
+should_return_error_for_deleted_dbs(Url) ->
+ ?_test(begin
+ create_and_delete_db(Url),
+ NewDoc = "{\"keys\": [\"db1\"]}",
+ {ok, Code, _, ResultBody} = test_request:post(Url ++ "/_deleted_dbs/",
+ [?CONTENT_JSON, ?AUTH], NewDoc),
Review comment:
The test name and the test body don't seem to agree here. I think you're testing that without database recovery enabled that we don't soft delete it. However, you're using a `POST` with a `"keys"` member here which just gets rejected in `deleted_dbs_post_req/` since it doesn't have the `"undelete"` or `"delete"` key present.
I think you're just wanting a `GET /_deleted_dbs/DbName` instead here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400289710
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -346,12 +351,97 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric2_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ Timestamp = list_to_binary(fabric2_util:iso8601_timestamp()),
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, Timestamp},
+ LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeletedDbKey)) of
+ not_found ->
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
+ erlfdb:clear(Tx, DbKey);
+ _Val ->
+ erlang:error({deleted_database_exists, DbName})
+ end;
+ false ->
+ erlfdb:clear(Tx, DbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix)
+ end,
bump_metadata_version(Tx),
ok.
+deleted_dbs_info(#{} = Db0) ->
Review comment:
Make sure we move this implementation to match the exports order.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r394810354
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -172,7 +174,8 @@ create(#{} = Db0, Options) ->
% Eventually DbPrefix will be HCA allocated. For now
% we're just using the DbName so that debugging is easier.
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- DbPrefix = erlfdb_tuple:pack({?DBS, DbName}, LayerPrefix),
+ #{db_prefix_allocator := DBPrefixAllocator} = erlfdb_directory:root(),
Review comment:
Maybe we'd want an allocator instance under the LayerPrefix and not use the main directory one just to keep things isolated. Without reading the hca code too closely, I assume that's where it would store its counters. We should do a quick benchmark at least to see if we see considerable contention when making lots of new databases in parallel. I suspect we won't but it's worth checking it out perhaps...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400293926
##########
File path: src/chttpd/src/chttpd_misc.erl
##########
@@ -146,6 +147,143 @@ handle_all_dbs_req(#httpd{method='GET'}=Req) ->
handle_all_dbs_req(Req) ->
send_method_not_allowed(Req, "GET,HEAD").
+handle_deleted_dbs_req(#httpd{path_parts=[<<"_deleted_dbs">>]}=Req) ->
+ deleted_dbs_req(Req);
+handle_deleted_dbs_req(#httpd{path_parts=[<<"_deleted_dbs">>, DbName]}=Req) ->
+ deleted_dbs_info_req(Req, DbName);
+handle_deleted_dbs_req(Req) ->
+ chttpd:send_error(Req, not_found).
+
+deleted_dbs_req(#httpd{method='GET'}=Req) ->
+ #mrargs{
+ start_key = StartKey,
+ end_key = EndKey,
+ direction = Dir,
+ limit = Limit,
+ skip = Skip
+ } = couch_mrview_http:parse_params(Req, undefined),
+
+ Options = [
+ {start_key, StartKey},
+ {end_key, EndKey},
+ {dir, Dir},
+ {limit, Limit},
+ {skip, Skip}
+ ],
+
+ % Eventually the Etag for this request will be derived
+ % from the \xFFmetadataVersion key in fdb
+ Etag = <<"foo">>,
+
+ {ok, Resp} = chttpd:etag_respond(Req, Etag, fun() ->
+ {ok, Resp} = chttpd:start_delayed_json_response(Req, 200, [{"ETag",Etag}]),
+ Callback = fun all_dbs_callback/2,
+ Acc = #vacc{req=Req,resp=Resp},
+ fabric2_db:list_deleted_dbs(Callback, Acc, Options)
+ end),
+ case is_record(Resp, vacc) of
+ true -> {ok, Resp#vacc.resp};
+ _ -> {ok, Resp}
+ end;
+deleted_dbs_req(#httpd{method='POST'}=Req) ->
+ deleted_dbs_post_req(Req);
+deleted_dbs_req(Req) ->
+ send_method_not_allowed(Req, "GET,POST,HEAD").
+
+deleted_dbs_info_req(#httpd{user_ctx=Ctx}=Req, DbName) ->
+ couch_httpd:verify_is_server_admin(Req),
+ case fabric2_db:deleted_dbs_info(DbName, [{user_ctx, Ctx}]) of
+ {ok, Result} ->
+ {ok, Resp} = chttpd:start_json_response(Req, 200),
+ send_chunk(Resp, "["),
+ lists:foldl(fun({Timestamp, Info}, AccSeparator) ->
+ Json = ?JSON_ENCODE({[
+ {key, DbName},
+ {timestamp, Timestamp},
+ {value, {Info}}
+ ]}),
+ send_chunk(Resp, AccSeparator ++ Json),
+ "," % AccSeparator now has a comma
+ end, "", Result),
+ send_chunk(Resp, "]"),
+ chttpd:end_json_response(Resp);
+ Error ->
+ throw(Error)
+ end;
+deleted_dbs_info_req(Req, _DbName) ->
+ send_method_not_allowed(Req, "GET,HEAD").
+
+deleted_dbs_post_req(#httpd{user_ctx=Ctx}=Req) ->
+ couch_httpd:verify_is_server_admin(Req),
+ chttpd:validate_ctype(Req, "application/json"),
+ {JsonProps} = chttpd:json_body_obj(Req),
+ UndeleteJson0 = couch_util:get_value(<<"undelete">>, JsonProps, undefined),
+ DeleteJson0 = couch_util:get_value(<<"delete">>, JsonProps, undefined),
+ case {UndeleteJson0, DeleteJson0} of
+ {undefined, undefined} ->
+ throw({bad_request, <<"POST body must include `undeleted` "
+ "or `delete` parameter.">>});
+ {UndeleteJson, undefined} ->
+ handle_undelete_db_req(Req, UndeleteJson);
+ {undefined, DeleteJson} ->
+ handle_delete_deleted_req(Req, DeleteJson);
+ {_Else, _Else} ->
+ throw({bad_request,
+ <<"`undeleted` and `delete` are mutually exclusive">>})
+ end.
+
+handle_undelete_db_req(#httpd{user_ctx=Ctx}=Req, {JsonProps}) ->
+ DbName = case couch_util:get_value(<<"source">>, JsonProps) of
+ undefined ->
+ throw({bad_request,
+ <<"POST body must include `source` parameter.">>});
+ DbName0 ->
+ DbName0
+ end,
+ TimeStamp = case couch_util:get_value(<<"source_timestamp">>, JsonProps) of
+ undefined ->
+ throw({bad_request,
+ <<"POST body must include `source_timestamp` parameter.">>});
+ TimeStamp0 ->
+ TimeStamp0
+ end,
+ TgtDbName = case couch_util:get_value(<<"target">>, JsonProps) of
+ undefined -> DbName;
+ TgtDbName0 -> TgtDbName0
+ end,
+ case fabric2_db:undelete(DbName, TgtDbName, TimeStamp, [{user_ctx, Ctx}]) of
+ ok ->
+ send_json(Req, 200, {[{ok, true}]});
+ {error, file_exists} ->
+ chttpd:send_error(Req, file_exists);
Review comment:
file_exists for the target database existing is good. But we'll also need a not_found when the source dbname or timestmap are incorrect.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
davisp commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-608544648
So, apparently I'm the only one that's been assuming that this is a mirror of the `GET _dbs_info` API end point and working from that perspective.
@mikerhodes I don't think the `_all_dbs` approach is very useful. When would you ever want just a list of database names that have soft deletions or just a list of timestamps? Its RESTy in all the bad ways that lead us to adding another API endpoint that dumps all of that information in a single response like we already had to do for `GET|POST /_dbs_info`.
@ricellis If we stay with the `POST _deleted_dbs` then I don't think its a good idea to remove the `"undelete"` key as the API loses its self-documentation aspect. Plus we'd be hamstrung adding other operations in the future (not that I can think of any). If we want to revert to `POST _undelete` then I'd be fine dropping the JSON key.
Consistency in the `"timestamp"` key is a good idea. I'd vote for `"timestamp"` since there's no target timestamp to disambiguate and `DELETE /_deleted_dbs/DbName?source_timestamp=foo` reads awkwardly.
@jiangphcn I'm can't decide whether you're suggesting we support the `GET /_deleted_dbs/DbName/Timestamp` API or not. I don't think we should add alternative APIs for things that can be accomplished with a single view based API so I would vote against adding them. But you might also saying we don't need them, in which case we're in agreement and you can ignore me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jaydoane removed a comment on issue #2666: soft-deletion
for database
Posted by GitBox <gi...@apache.org>.
jaydoane removed a comment on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-607996688
> I've also heard some concerns that having the `undelete` vs `delete` semantics of our `POST /_deleted_dbs` setup makes auditing traffic hard/impossible without introspecting the request body which seems like a legitimate critique. How about if we tweaked things to be like such:
>
> * `GET /_deleted_dbs` - List deleted dbs infos as currently.
>
> * `POST /_deleted_dbs` - Only accepts the `undelete` JSON body
>
> * `DELETE /_deleted_dbs/DbName?timestamp=2020-01-01T00:00:00` - Remove deleted db
I very much like this particular API, since it's trivial to derive three separate action names from method and path, unlike earlier versions. The corresponding names (eliding the ubiquitous `cloudantnosqldb.` prefix), I'm considering:
- `account-deleted-dbs.read`
- `account-deleted-dbs.undelete`
- `account-deleted-dbs.delete`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on a change in pull request #2666:
[WIP] soft-deletion for database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r394973917
##########
File path: src/fabric/src/fabric_util.erl
##########
@@ -23,6 +23,8 @@
-export([validate_all_docs_args/2, validate_args/3]).
-export([upgrade_mrargs/1]).
-export([worker_ranges/1]).
+-export([do_recovery/0]).
Review comment:
`couchdb.enable_database_recovery` is inherited from CouchDB 3.0x or early. Is there similar case in CouchDB 4.0 to introduce different name for configuration? or we have to keep the same as before?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r394797524
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -314,12 +317,65 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ {Mega, Secs, _} = os:timestamp(),
+ NowSecs = Mega * 1000000 + Secs,
Review comment:
This could be `erlang:system_time(second).` I think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r396602850
##########
File path: src/fabric/src/fabric2_util.erl
##########
@@ -345,3 +347,16 @@ pmap_exec(Fun, Arg) ->
end.
-endif.
+
+
+iso8601_timestamp() ->
+ Now = os:timestamp(),
+ {{Year, Month, Date}, {Hour, Minute,
+ Second}} = calendar:now_to_datetime(Now),
Review comment:
Minor nit:
```erlang
{{Year, Month, Date}, {Hour, Minute, Second}} =
calendar:now_to_datetime(Now),
```
I forget the words but having line breaks around operators is usually more readable when possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r401803793
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -365,6 +364,65 @@ exists(#{name := DbName} = Db) when is_binary(DbName) ->
end.
+undelete(#{} = Db0, TgtDbName, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+ DbKey = erlfdb_tuple:pack({?ALL_DBS, TgtDbName}, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DbKey)) of
+ Bin when is_binary(Bin) ->
+ file_exists;
+ not_found ->
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found});
Review comment:
We're mixing error exceptions vs error returns here vs line 376. We should pick one or the other and be consistent.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] ricellis commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
ricellis commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-608532116
OK, so now the `POST` can only do an undelete couldn't the body just be:
```
{
"source_timestamp": "2020-04-01T07:13:42Z",
"source": "db01",
}
```
That also removes the awkward naming around `undelete` that I know was bothering some people.
Also given
`DELETE /_deleted_dbs/{dbName}`
shouldn't we also
`POST /_deleted_dbs/{dbName}`
since they are operating on the same resource?
Then the body would be just (or also with an optional `target`):
```
{
"source_timestamp": "2020-04-01T07:13:42Z"
}
```
Although I think the parameter name in the body should match that used on the `DELETE` request as well; either both `timestamp` or both `source_timestamp`.
It seems like it would be nice to use paths consistently throughout as [@mikerhodes proposed](https://gist.github.com/mikerhodes/39c66576eee468a5aeaa23c5715aec9a), but I can see the rationale for preferring to use `key=dbname` to be consistent with the view-like APIs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r401767219
##########
File path: src/chttpd/test/eunit/chttpd_deleted_dbs_test.erl
##########
@@ -0,0 +1,206 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(chttpd_deleted_dbs_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+
+-define(USER, "chttpd_db_test_admin").
+-define(PASS, "pass").
+-define(AUTH, {basic_auth, {?USER, ?PASS}}).
+-define(CONTENT_JSON, {"Content-Type", "application/json"}).
+
+
+setup() ->
+ Hashed = couch_passwords:hash_admin_password(?PASS),
+ ok = config:set("admins", ?USER, ?b2l(Hashed), _Persist=false),
+ Addr = config:get("chttpd", "bind_address", "127.0.0.1"),
+ Port = mochiweb_socket_server:get(chttpd, port),
+ lists:concat(["http://", Addr, ":", Port, "/"]).
+
+teardown(_Url) ->
+ ok = config:delete("couchdb", "enable_database_recovery", false),
+ ok = config:delete("admins", ?USER, _Persist=false).
+
+create_db(Url) ->
+ {ok, Status, _, _} = test_request:put(Url, [?CONTENT_JSON, ?AUTH], "{}"),
+ ?assert(Status =:= 201 orelse Status =:= 202).
+
+delete_db(Url) ->
+ {ok, 200, _, _} = test_request:delete(Url, [?AUTH]).
+
+deleted_dbs_test_() ->
+ {
+ "chttpd deleted dbs tests",
+ {
+ setup,
+ fun chttpd_test_util:start_couch/0, fun chttpd_test_util:stop_couch/1,
Review comment:
Also for the foreach section below.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r401799267
##########
File path: src/chttpd/test/eunit/chttpd_deleted_dbs_test.erl
##########
@@ -0,0 +1,206 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(chttpd_deleted_dbs_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+
+-define(USER, "chttpd_db_test_admin").
+-define(PASS, "pass").
+-define(AUTH, {basic_auth, {?USER, ?PASS}}).
+-define(CONTENT_JSON, {"Content-Type", "application/json"}).
+
+
+setup() ->
+ Hashed = couch_passwords:hash_admin_password(?PASS),
+ ok = config:set("admins", ?USER, ?b2l(Hashed), _Persist=false),
+ Addr = config:get("chttpd", "bind_address", "127.0.0.1"),
+ Port = mochiweb_socket_server:get(chttpd, port),
+ lists:concat(["http://", Addr, ":", Port, "/"]).
+
+teardown(_Url) ->
+ ok = config:delete("couchdb", "enable_database_recovery", false),
+ ok = config:delete("admins", ?USER, _Persist=false).
+
+create_db(Url) ->
+ {ok, Status, _, _} = test_request:put(Url, [?CONTENT_JSON, ?AUTH], "{}"),
+ ?assert(Status =:= 201 orelse Status =:= 202).
+
+delete_db(Url) ->
+ {ok, 200, _, _} = test_request:delete(Url, [?AUTH]).
+
+deleted_dbs_test_() ->
+ {
+ "chttpd deleted dbs tests",
+ {
+ setup,
+ fun chttpd_test_util:start_couch/0, fun chttpd_test_util:stop_couch/1,
+ {
+ foreach,
+ fun setup/0, fun teardown/1,
+ [
+ fun should_return_error_for_deleted_dbs/1,
+ fun should_list_deleted_dbs/1,
+ fun should_list_deleted_dbs_info/1,
+ fun should_undelete_db/1,
+ fun should_remove_deleted_db/1,
+ fun should_undelete_db_to_target_db/1,
+ fun should_not_undelete_db_to_existing_db/1
+ ]
+ }
+ }
+ }.
+
+
+should_return_error_for_deleted_dbs(Url) ->
+ ?_test(begin
+ create_and_delete_db(Url),
+ NewDoc = "{\"keys\": [\"db1\"]}",
+ {ok, Code, _, ResultBody} = test_request:post(Url ++ "/_deleted_dbs/",
+ [?CONTENT_JSON, ?AUTH], NewDoc),
+
+ {Body} = jiffy:decode(ResultBody),
+ [
+ ?assertEqual(<<"bad_request">>,
+ couch_util:get_value(<<"error">>, Body)),
+ ?assertEqual(400, Code)
+ ]
+ end).
+
+
+should_list_deleted_dbs(Url) ->
+ ?_test(begin
+ DbName1 = create_and_delete_db(Url),
+ DbName2 = create_and_delete_db(Url),
+ {ok, _, _, ResultBody} = test_request:get(Url ++ "/_deleted_dbs/",
+ [?CONTENT_JSON, ?AUTH]),
+ BodyJson = jiffy:decode(ResultBody),
+ ?assertEqual(true, lists:member(DbName1, BodyJson)),
+ ?assertEqual(true, lists:member(DbName2, BodyJson))
+ end).
+
+
+should_list_deleted_dbs_info(Url) ->
+ ?_test(begin
+ DbName = create_and_delete_db(Url),
+ {ok, _, _, ResultBody} = test_request:get(Url ++ "/_deleted_dbs/" ++
+ DbName, [?CONTENT_JSON, ?AUTH]),
+ BodyJson = jiffy:decode(ResultBody),
+
+ {Db1Data} = lists:nth(1, BodyJson),
+ ?assertEqual(DbName,
+ couch_util:get_value(<<"key">>, Db1Data))
+ end).
+
+
+should_undelete_db(Url) ->
+ ?_test(begin
+ DbName = create_and_delete_db(Url),
+ {ok, _, _, ResultBody} = test_request:get(Url ++ "/_deleted_dbs/" ++
+ DbName, [?CONTENT_JSON, ?AUTH]),
+ BodyJson = jiffy:decode(ResultBody),
+
+ {Db1Data} = lists:nth(1, BodyJson),
+ TimeStamp = couch_util:get_value(<<"timestamp">>, Db1Data),
+
+ NewDoc = "{\"undelete\": {\"source\": \"" ++ ?b2l(DbName) ++
Review comment:
Oooh, the defaulting of the target to be the same as the source is a good idea.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400925228
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -346,12 +351,97 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric2_util:do_recovery(),
Review comment:
I split `delete` into `soft_delete_db/1` and `hard_delete_db/1`. For `remove_deleted_db`, it is a separate call from `fabric2_db:delete/2`.
```
delete(#{} = Db) ->
DoRecovery = fabric2_util:do_recovery(),
case DoRecovery of
true -> soft_delete_db(Db);
false -> hard_delete_db(Db)
end.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-607841279
Just for simplification, the _deleted_dbs wasn't returning DbInfo blobs directly in the past. In order to get DbInfo blobs, need to call `GET _deleted_dbs/{db}/`. But with new commit 031cb85, DbInfo Blobs is added as well for `GET _deleted_dbs`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400301844
##########
File path: src/chttpd/src/chttpd_misc.erl
##########
@@ -146,6 +147,143 @@ handle_all_dbs_req(#httpd{method='GET'}=Req) ->
handle_all_dbs_req(Req) ->
send_method_not_allowed(Req, "GET,HEAD").
+handle_deleted_dbs_req(#httpd{path_parts=[<<"_deleted_dbs">>]}=Req) ->
Review comment:
The combination of function names here is a bit difficult. I'd change this around to move function implementations to this top level function. Something like:
```erlang
handle_deleted_dbs_req(#httpd{method = 'GET', path_parts = [_]}) ->
... body of deleted_dbs_req with method='GET' ...;
handle_deleted_dbs_req(#httpd{method = 'GET', path_parts = [_, DbName]) ->
... body of deleted_dbs_info_req ...;
handle_deleted_dbs_req(#httpd{method = 'POST', path_parts = [_]}) ->
... body of deleted_dbs_post_req ...;
handle_deleted_dbs_req(#httpd{path_parts = PP}) when length(PP) =< 2 ->
send_method_not_allowed("GET,HEAD,POST");
handle_deleted_dbs_req(Req) ->
chttpd:send_error({error, not_found}).
```
And to be clear, I'd leave handle_undelete/delete_deleted as separate functions (though rename delete_deleted to remove_deleted or similar to match fabric2_db API names.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400292224
##########
File path: src/fabric/test/fabric2_db_crud_tests.erl
##########
@@ -139,6 +146,44 @@ recreate_db(_) ->
?assertError(database_does_not_exist, fabric2_db:open(DbName, BadOpts)).
+undelete_db(_) ->
+ DbName = ?tempdb(),
+ ?assertError(database_does_not_exist, fabric2_db:delete(DbName, [])),
+
+ ?assertMatch({ok, _}, fabric2_db:create(DbName, [])),
+ ?assertEqual(true, ets:member(fabric2_server, DbName)),
+
+ ok = config:set("couchdb", "enable_database_recovery", "true", false),
+ ?assertEqual(ok, fabric2_db:delete(DbName, [])),
+ ok = config:delete("couchdb", "enable_database_recovery", false),
+ ?assertEqual(false, ets:member(fabric2_server, DbName)),
+
+ {ok, [{Timestamp, _Info}]} = fabric2_db:deleted_dbs_info(DbName, []),
Review comment:
For both undelete and delete_deleted we should add tests that assert the not_found error if we give it a bad dbname as well as a valid dbname but invalid timestamp.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn edited a comment on issue #2666: soft-deletion
for database
Posted by GitBox <gi...@apache.org>.
jiangphcn edited a comment on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-608511710
> POST /_deleted_dbs - Only accepts the undelete JSON body to restore the deleted db
> Can I see an example of the expected body now? If undelete is the only "action" for POST is there any reason to call it out separately?
Yes, below is one example.
```
{
"undelete": {
"source_timestamp": "2020-04-01T07:13:42Z",
"source": "db01"
}
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-604340091
> Unrelated to the endpoint name the example shows no TZ data in the ISO-8601 timestamp, is that an accurate representation of the output/input we're expecting?
There are 2 different formats for ISO-8601 timestamp, `1994-11-05T08:15:30-05:00` and `1994-11-05T13:15:30Z`. They all correspond to November 5, 1994, 8:15:30 am, US Eastern Standard Time. The detail can be found at https://www.w3.org/TR/NOTE-datetime. We are using `1994-11-05T13:15:30Z` where TZ information exists as well.
We can add more description in document to state the format we used here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn edited a comment on issue #2666: soft-deletion
for database
Posted by GitBox <gi...@apache.org>.
jiangphcn edited a comment on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-609814355
thanks @davisp and @mikerhodes for comments over weekend. I come with up one new commit
e8de2fc where mainly covers the suggestion from Paul about code refactor. One thing is related to combine implementation of `/_deleted_dbs` and `/_deleted_dbs?key`. This mainly follows the way of `_dbs_info` while there is slight difference for `/_deleted_dbs?key` because the key in key/value pair for `_deleted_dbs` is not only `dbname`, but also includes `timestamp`. So i introduce the way of https://github.com/apache/couchdb/pull/2666/commits/527d2817e296a30439c6a5337dc0794adf2eeec0#diff-20d0112d14281f71e2bd09f75da80f27R286-R295 to find dbinfo for specified key.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r401797528
##########
File path: src/chttpd/test/eunit/chttpd_deleted_dbs_test.erl
##########
@@ -0,0 +1,206 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(chttpd_deleted_dbs_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+
+-define(USER, "chttpd_db_test_admin").
+-define(PASS, "pass").
+-define(AUTH, {basic_auth, {?USER, ?PASS}}).
+-define(CONTENT_JSON, {"Content-Type", "application/json"}).
+
+
+setup() ->
+ Hashed = couch_passwords:hash_admin_password(?PASS),
+ ok = config:set("admins", ?USER, ?b2l(Hashed), _Persist=false),
+ Addr = config:get("chttpd", "bind_address", "127.0.0.1"),
+ Port = mochiweb_socket_server:get(chttpd, port),
+ lists:concat(["http://", Addr, ":", Port, "/"]).
+
+teardown(_Url) ->
+ ok = config:delete("couchdb", "enable_database_recovery", false),
+ ok = config:delete("admins", ?USER, _Persist=false).
+
+create_db(Url) ->
+ {ok, Status, _, _} = test_request:put(Url, [?CONTENT_JSON, ?AUTH], "{}"),
+ ?assert(Status =:= 201 orelse Status =:= 202).
+
+delete_db(Url) ->
+ {ok, 200, _, _} = test_request:delete(Url, [?AUTH]).
+
+deleted_dbs_test_() ->
+ {
+ "chttpd deleted dbs tests",
+ {
+ setup,
+ fun chttpd_test_util:start_couch/0, fun chttpd_test_util:stop_couch/1,
+ {
+ foreach,
+ fun setup/0, fun teardown/1,
+ [
+ fun should_return_error_for_deleted_dbs/1,
+ fun should_list_deleted_dbs/1,
+ fun should_list_deleted_dbs_info/1,
+ fun should_undelete_db/1,
+ fun should_remove_deleted_db/1,
+ fun should_undelete_db_to_target_db/1,
+ fun should_not_undelete_db_to_existing_db/1
+ ]
+ }
+ }
+ }.
+
+
+should_return_error_for_deleted_dbs(Url) ->
+ ?_test(begin
+ create_and_delete_db(Url),
+ NewDoc = "{\"keys\": [\"db1\"]}",
+ {ok, Code, _, ResultBody} = test_request:post(Url ++ "/_deleted_dbs/",
+ [?CONTENT_JSON, ?AUTH], NewDoc),
+
+ {Body} = jiffy:decode(ResultBody),
+ [
+ ?assertEqual(<<"bad_request">>,
+ couch_util:get_value(<<"error">>, Body)),
+ ?assertEqual(400, Code)
+ ]
+ end).
+
+
+should_list_deleted_dbs(Url) ->
+ ?_test(begin
+ DbName1 = create_and_delete_db(Url),
+ DbName2 = create_and_delete_db(Url),
+ {ok, _, _, ResultBody} = test_request:get(Url ++ "/_deleted_dbs/",
+ [?CONTENT_JSON, ?AUTH]),
+ BodyJson = jiffy:decode(ResultBody),
+ ?assertEqual(true, lists:member(DbName1, BodyJson)),
+ ?assertEqual(true, lists:member(DbName2, BodyJson))
Review comment:
Also, I'm not seeing how we turn on database recovery for these tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r402439168
##########
File path: src/fabric/src/fabric2_db.erl
##########
@@ -1025,6 +1101,51 @@ drain_all_info_futures(FutureQ, UserFun, Acc) ->
Acc
end.
+make_deleted_dbs_info(DbName, TimeStamp, Props) ->
+ BaseProps = [
+ {key, DbName},
+ {timestamp, TimeStamp},
+ {value, {[
+ {cluster, {[{n, 0}, {q, 0}, {r, 0}, {w, 0}]}},
+ {compact_running, false},
+ {data_size, 0},
+ {db_name, DbName},
+ {timestamp, TimeStamp},
+ {disk_format_version, 0},
+ {disk_size, 0},
+ {instance_start_time, <<"0">>},
+ {purge_seq, 0}
+ ]}
+ }
Review comment:
Super minor nit, but this should be a single line `]}}` at two indents.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-608646035
hey @davisp
I think that you are *not* the only one that's been assuming that this is a mirror of the GET _dbs_info API end point and working from that perspective. From https://github.com/apache/couchdb/pull/2666/#issuecomment-608411228, you can see that I am for that and implemented with this approach.
In addition, I don't support the suggestion about `the GET /_deleted_dbs/DbName/Timestamp API`. As i mentioned before, it looks that user most likely should review information of the deletion time, doc number, disk size to decide to restore it or permanently delete it. It is convenient to return DbInfo one time instead of using the progressive approach like mentioned in [1] to get the detailed information of the deleted db.
```
GET /_deleted_dbs
GET /_deleted_dbs/DbName
GET /_deleted_dbs/DbName/2020-01-01T00:00:00
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on a change in pull request #2666:
[WIP] soft-deletion for database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r395619990
##########
File path: src/fabric/src/fabric_util.erl
##########
@@ -23,6 +23,8 @@
-export([validate_all_docs_args/2, validate_args/3]).
-export([upgrade_mrargs/1]).
-export([worker_ranges/1]).
+-export([do_recovery/0]).
Review comment:
Sure, and moved this to `fabric2_util`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400295708
##########
File path: src/chttpd/src/chttpd_misc.erl
##########
@@ -146,6 +147,143 @@ handle_all_dbs_req(#httpd{method='GET'}=Req) ->
handle_all_dbs_req(Req) ->
send_method_not_allowed(Req, "GET,HEAD").
+handle_deleted_dbs_req(#httpd{path_parts=[<<"_deleted_dbs">>]}=Req) ->
+ deleted_dbs_req(Req);
+handle_deleted_dbs_req(#httpd{path_parts=[<<"_deleted_dbs">>, DbName]}=Req) ->
+ deleted_dbs_info_req(Req, DbName);
+handle_deleted_dbs_req(Req) ->
+ chttpd:send_error(Req, not_found).
+
+deleted_dbs_req(#httpd{method='GET'}=Req) ->
+ #mrargs{
+ start_key = StartKey,
+ end_key = EndKey,
+ direction = Dir,
+ limit = Limit,
+ skip = Skip
+ } = couch_mrview_http:parse_params(Req, undefined),
+
+ Options = [
+ {start_key, StartKey},
+ {end_key, EndKey},
+ {dir, Dir},
+ {limit, Limit},
+ {skip, Skip}
+ ],
+
+ % Eventually the Etag for this request will be derived
+ % from the \xFFmetadataVersion key in fdb
+ Etag = <<"foo">>,
+
+ {ok, Resp} = chttpd:etag_respond(Req, Etag, fun() ->
+ {ok, Resp} = chttpd:start_delayed_json_response(Req, 200, [{"ETag",Etag}]),
+ Callback = fun all_dbs_callback/2,
+ Acc = #vacc{req=Req,resp=Resp},
+ fabric2_db:list_deleted_dbs(Callback, Acc, Options)
+ end),
+ case is_record(Resp, vacc) of
+ true -> {ok, Resp#vacc.resp};
+ _ -> {ok, Resp}
+ end;
+deleted_dbs_req(#httpd{method='POST'}=Req) ->
+ deleted_dbs_post_req(Req);
+deleted_dbs_req(Req) ->
+ send_method_not_allowed(Req, "GET,POST,HEAD").
+
+deleted_dbs_info_req(#httpd{user_ctx=Ctx}=Req, DbName) ->
+ couch_httpd:verify_is_server_admin(Req),
+ case fabric2_db:deleted_dbs_info(DbName, [{user_ctx, Ctx}]) of
+ {ok, Result} ->
+ {ok, Resp} = chttpd:start_json_response(Req, 200),
+ send_chunk(Resp, "["),
+ lists:foldl(fun({Timestamp, Info}, AccSeparator) ->
+ Json = ?JSON_ENCODE({[
+ {key, DbName},
+ {timestamp, Timestamp},
+ {value, {Info}}
+ ]}),
+ send_chunk(Resp, AccSeparator ++ Json),
+ "," % AccSeparator now has a comma
+ end, "", Result),
+ send_chunk(Resp, "]"),
+ chttpd:end_json_response(Resp);
+ Error ->
+ throw(Error)
+ end;
+deleted_dbs_info_req(Req, _DbName) ->
+ send_method_not_allowed(Req, "GET,HEAD").
+
+deleted_dbs_post_req(#httpd{user_ctx=Ctx}=Req) ->
+ couch_httpd:verify_is_server_admin(Req),
+ chttpd:validate_ctype(Req, "application/json"),
+ {JsonProps} = chttpd:json_body_obj(Req),
+ UndeleteJson0 = couch_util:get_value(<<"undelete">>, JsonProps, undefined),
+ DeleteJson0 = couch_util:get_value(<<"delete">>, JsonProps, undefined),
+ case {UndeleteJson0, DeleteJson0} of
+ {undefined, undefined} ->
+ throw({bad_request, <<"POST body must include `undeleted` "
+ "or `delete` parameter.">>});
+ {UndeleteJson, undefined} ->
+ handle_undelete_db_req(Req, UndeleteJson);
+ {undefined, DeleteJson} ->
+ handle_delete_deleted_req(Req, DeleteJson);
+ {_Else, _Else} ->
+ throw({bad_request,
+ <<"`undeleted` and `delete` are mutually exclusive">>})
+ end.
+
+handle_undelete_db_req(#httpd{user_ctx=Ctx}=Req, {JsonProps}) ->
+ DbName = case couch_util:get_value(<<"source">>, JsonProps) of
+ undefined ->
+ throw({bad_request,
+ <<"POST body must include `source` parameter.">>});
+ DbName0 ->
+ DbName0
+ end,
+ TimeStamp = case couch_util:get_value(<<"source_timestamp">>, JsonProps) of
+ undefined ->
+ throw({bad_request,
+ <<"POST body must include `source_timestamp` parameter.">>});
+ TimeStamp0 ->
+ TimeStamp0
+ end,
+ TgtDbName = case couch_util:get_value(<<"target">>, JsonProps) of
+ undefined -> DbName;
+ TgtDbName0 -> TgtDbName0
+ end,
+ case fabric2_db:undelete(DbName, TgtDbName, TimeStamp, [{user_ctx, Ctx}]) of
+ ok ->
+ send_json(Req, 200, {[{ok, true}]});
+ {error, file_exists} ->
+ chttpd:send_error(Req, file_exists);
Review comment:
Also now that I think harder, these should be exceptions instead of returned values. chttpd will catch them and do the proper send_error logic (assuming that we match one of the existing error patterns or add any new ones). So something like:
```erlang
ok = fabric2_db:undelete(DbName, TgtDbName, TimeStamp, [{user_ctx, Ctx}]),
send_json(Req, 200, {[{ok, true}]}).
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400979556
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -346,12 +351,97 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric2_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ Timestamp = list_to_binary(fabric2_util:iso8601_timestamp()),
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, Timestamp},
+ LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeletedDbKey)) of
+ not_found ->
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
+ erlfdb:clear(Tx, DbKey);
+ _Val ->
+ erlang:error({deleted_database_exists, DbName})
+ end;
+ false ->
+ erlfdb:clear(Tx, DbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix)
+ end,
bump_metadata_version(Tx),
ok.
+deleted_dbs_info(#{} = Db0) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName}, LayerPrefix),
+ DeletedDbs = erlfdb:wait(erlfdb:get_range_startswith(Tx, DeletedDbKey)),
+ lists:foldl(fun({DbKey, DbPrefix}, Acc) ->
+ DBInfo = get_info_wait(get_info_future(Tx, DbPrefix)),
+ {?DELETED_DBS, DbName, DeletedTS} =
+ erlfdb_tuple:unpack(DbKey, LayerPrefix),
+ [{DeletedTS, DBInfo}| Acc]
+ end, [], DeletedDbs).
+
+
+undelete(#{} = Db0, TgtDbName, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+ DbKey = erlfdb_tuple:pack({?ALL_DBS, TgtDbName}, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DbKey)) of
+ Bin when is_binary(Bin) ->
+ {error, file_exists};
+ not_found ->
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found, invalid_timestamp});
+ DbPrefix ->
+ erlfdb:set(Tx, DbKey, DbPrefix),
+ erlfdb:clear(Tx, DeleteDbKey),
+ bump_metadata_version(Tx),
+ ok
+ end
+ end.
+
+
+delete_deleted(#{} = Db0, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0),
+
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found});
+ DbPrefix ->
+ erlfdb:clear(Tx, DeleteDbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix),
+ bump_metadata_version(Tx)
Review comment:
Thinking more about tests I think we'd want at least for these scenarios:
Cases where old db handle can't read the data anymore:
- db soft deleted
- db hard deleted
- db soft deleted and re-created
- db soft deleted and undeleted
- db hard deleted and re-created
Indexing stops when:
- db soft deleted
- db hard deleted
- db soft deleted and re-created
- db soft deleted and undeleted
- db hard deleted and re-created
Obviously indexing will stop if the first set of tests pass, however we want to make sure the way indexing detects that db is now deleted hasn't changed and it reacts the same as before. Look for db re-created or rename tests in recent commits and I think we could modify those to take soft deletion into consideration.
If we are using timestamps with 1 second resolution also make sure we handle cases were we soft-delete/undelete multiple times in the same second, and also tests where we do it across a few different seconds.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r396609556
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -79,6 +79,8 @@ handle_request(#httpd{path_parts=[DbName|RestParts],method=Method}=Req)->
end;
{_, []} ->
do_db_req(Req, fun db_req/2);
+ {_, [<<"_restore">>|_]} ->
Review comment:
This needs to go into the handlers list [1] and not be hard coded at this level.
[1] https://github.com/apache/couchdb/blob/prototype/fdb-layer/src/chttpd/src/chttpd_httpd_handlers.erl#L20-L33
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-604451352
Thanks for more comments.
It looks to me that `_deleted_dbs` is more clear than `_deleted`. So the current design becomes
```
GET /_deleted_dbs/dbs
GET /_deleted_dbs/dbs/DbName
POST /_deleted_dbs/undelete
```
Any more comments?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r396598481
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -346,12 +353,64 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric2_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ Timestamp = list_to_binary(fabric2_util:iso8601_timestamp()),
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, Timestamp},
+ LayerPrefix),
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
+ erlfdb:clear(Tx, DbKey);
+ false ->
+ erlfdb:clear(Tx, DbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix)
+ end,
bump_metadata_version(Tx),
ok.
+deleted_dbs_info(#{} = Db0) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName}, LayerPrefix),
+ DeletedDbs = erlfdb:wait(erlfdb:get_range_startswith(Tx, DeletedDbKey)),
+ lists:foldl(fun({DbKey, DbPrefix}, Acc) ->
+ DBInfo = get_info_wait(get_info_future(Tx, DbPrefix)),
+ {?DELETED_DBS, DbName, DeletedTS} =
+ erlfdb_tuple:unpack(DbKey, LayerPrefix),
+ [{DeletedTS, DBInfo}| Acc]
+ end, [], DeletedDbs).
+
+
+restore(#{} = Db0, DesDbName, Timestamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+ DbKey = erlfdb_tuple:pack({?ALL_DBS, DesDbName}, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DbKey)) of
+ Bin when is_binary(Bin) ->
+ {error, file_exists};
+ not_found ->
+ DeleteDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName,
Review comment:
Slight style nit. And this is personal preference, but I find that FDB keys that span multiple lines its easier to read if we break them out to one line per. So I'd normal do something like:
```erlang
DeletedDbTupleKey = {
?DELETED_DBS,
DbName,
TimeStamp
},
DeletedDbKey = erlfdb_tuple:pack(DeletedDbTuple, LayerPrefix),
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r396591094
##########
File path: src/fabric/include/fabric2.hrl
##########
@@ -17,10 +17,13 @@
% Prefix Definitions
+-define(DEFAULT_DB_PREFIX, <<16#FD>>).
+
% Layer Level: (LayerPrefix, X, ...)
-define(CLUSTER_CONFIG, 0).
-define(ALL_DBS, 1).
+-define(DELETED_DBS, 2).
Review comment:
For the HCA, you can either create a new subspace here with something like `DB_NAME_HCA` at 3 or alternatively create a key in the `CLUSTER_CONFIG` subspace. All you need for the HCA API is a specific key that's used with the HCA API.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r399410655
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -356,8 +356,13 @@ delete(#{} = Db) ->
Timestamp = list_to_binary(fabric2_util:iso8601_timestamp()),
DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, Timestamp},
LayerPrefix),
- erlfdb:set(Tx, DeletedDbKey, DbPrefix),
- erlfdb:clear(Tx, DbKey);
+ case erlfdb:wait(erlfdb:get(Tx, DeletedDbKey)) of
+ not_found ->
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
+ erlfdb:clear(Tx, DbKey);
+ _Val ->
+ error(database_was_deleted)
Review comment:
Lets qualify this and also include a better error message:
```erlang
erlang:error({deleted_database_exists, DbName})
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-609814355
thanks @davisp and @mikerhodes for comments over weekend. I come with up one new commit
527d281 where mainly covers the suggestion from Paul about code refactor. One thing is related to combine implementation of `/_deleted_dbs` and `/_deleted_dbs?key`. This mainly follows the way of `_dbs_info` while there is slight difference for `/_deleted_dbs?key` because the key in key/value pair for `_deleted_dbs` is not only `dbname`, but also includes `timestamp`. So i introduce the way of https://github.com/apache/couchdb/pull/2666/commits/527d2817e296a30439c6a5337dc0794adf2eeec0#diff-20d0112d14281f71e2bd09f75da80f27R286-R295 to find dbinfo for specified key.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400291193
##########
File path: src/fabric/test/fabric2_db_crud_tests.erl
##########
@@ -139,6 +146,44 @@ recreate_db(_) ->
?assertError(database_does_not_exist, fabric2_db:open(DbName, BadOpts)).
+undelete_db(_) ->
+ DbName = ?tempdb(),
+ ?assertError(database_does_not_exist, fabric2_db:delete(DbName, [])),
+
+ ?assertMatch({ok, _}, fabric2_db:create(DbName, [])),
+ ?assertEqual(true, ets:member(fabric2_server, DbName)),
+
+ ok = config:set("couchdb", "enable_database_recovery", "true", false),
+ ?assertEqual(ok, fabric2_db:delete(DbName, [])),
+ ok = config:delete("couchdb", "enable_database_recovery", false),
Review comment:
We can remove these calls undoing the config change now that we have the call in the cleanup (unless you have a test that's making assertions with and without it on of course).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn merged pull request #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
jiangphcn merged pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-608511710
> POST /_deleted_dbs - Only accepts the undelete JSON body to restore the deleted db
> Can I see an example of the expected body now? If undelete is the only "action" for POST is there any reason to call it out separately?
Yes, below is one example.
{
"undelete": {
"source_timestamp": "2020-04-01T07:13:42Z",
"source": "db01"
}
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r401821038
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -346,12 +351,97 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric2_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ Timestamp = list_to_binary(fabric2_util:iso8601_timestamp()),
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, Timestamp},
+ LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeletedDbKey)) of
+ not_found ->
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
+ erlfdb:clear(Tx, DbKey);
+ _Val ->
+ erlang:error({deleted_database_exists, DbName})
+ end;
+ false ->
+ erlfdb:clear(Tx, DbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix)
+ end,
bump_metadata_version(Tx),
ok.
+deleted_dbs_info(#{} = Db0) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName}, LayerPrefix),
+ DeletedDbs = erlfdb:wait(erlfdb:get_range_startswith(Tx, DeletedDbKey)),
+ lists:foldl(fun({DbKey, DbPrefix}, Acc) ->
+ DBInfo = get_info_wait(get_info_future(Tx, DbPrefix)),
+ {?DELETED_DBS, DbName, DeletedTS} =
+ erlfdb_tuple:unpack(DbKey, LayerPrefix),
+ [{DeletedTS, DBInfo}| Acc]
+ end, [], DeletedDbs).
+
+
+undelete(#{} = Db0, TgtDbName, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+ DbKey = erlfdb_tuple:pack({?ALL_DBS, TgtDbName}, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DbKey)) of
+ Bin when is_binary(Bin) ->
+ {error, file_exists};
+ not_found ->
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found, invalid_timestamp});
+ DbPrefix ->
+ erlfdb:set(Tx, DbKey, DbPrefix),
+ erlfdb:clear(Tx, DeleteDbKey),
+ bump_metadata_version(Tx),
+ ok
+ end
+ end.
+
+
+delete_deleted(#{} = Db0, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0),
+
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found});
+ DbPrefix ->
+ erlfdb:clear(Tx, DeleteDbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix),
+ bump_metadata_version(Tx)
Review comment:
Oh, for after creation. For that "use after deletion" we should make sure that between deletion and undeletion that the handle isn't usable as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400923495
##########
File path: src/chttpd/src/chttpd_misc.erl
##########
@@ -146,6 +147,143 @@ handle_all_dbs_req(#httpd{method='GET'}=Req) ->
handle_all_dbs_req(Req) ->
send_method_not_allowed(Req, "GET,HEAD").
+handle_deleted_dbs_req(#httpd{path_parts=[<<"_deleted_dbs">>]}=Req) ->
+ deleted_dbs_req(Req);
+handle_deleted_dbs_req(#httpd{path_parts=[<<"_deleted_dbs">>, DbName]}=Req) ->
+ deleted_dbs_info_req(Req, DbName);
+handle_deleted_dbs_req(Req) ->
+ chttpd:send_error(Req, not_found).
+
+deleted_dbs_req(#httpd{method='GET'}=Req) ->
+ #mrargs{
+ start_key = StartKey,
+ end_key = EndKey,
+ direction = Dir,
+ limit = Limit,
+ skip = Skip
+ } = couch_mrview_http:parse_params(Req, undefined),
+
+ Options = [
+ {start_key, StartKey},
+ {end_key, EndKey},
+ {dir, Dir},
+ {limit, Limit},
+ {skip, Skip}
+ ],
+
+ % Eventually the Etag for this request will be derived
+ % from the \xFFmetadataVersion key in fdb
+ Etag = <<"foo">>,
+
+ {ok, Resp} = chttpd:etag_respond(Req, Etag, fun() ->
+ {ok, Resp} = chttpd:start_delayed_json_response(Req, 200, [{"ETag",Etag}]),
+ Callback = fun all_dbs_callback/2,
+ Acc = #vacc{req=Req,resp=Resp},
+ fabric2_db:list_deleted_dbs(Callback, Acc, Options)
+ end),
+ case is_record(Resp, vacc) of
+ true -> {ok, Resp#vacc.resp};
+ _ -> {ok, Resp}
+ end;
+deleted_dbs_req(#httpd{method='POST'}=Req) ->
+ deleted_dbs_post_req(Req);
+deleted_dbs_req(Req) ->
+ send_method_not_allowed(Req, "GET,POST,HEAD").
+
+deleted_dbs_info_req(#httpd{user_ctx=Ctx}=Req, DbName) ->
+ couch_httpd:verify_is_server_admin(Req),
+ case fabric2_db:deleted_dbs_info(DbName, [{user_ctx, Ctx}]) of
+ {ok, Result} ->
+ {ok, Resp} = chttpd:start_json_response(Req, 200),
+ send_chunk(Resp, "["),
+ lists:foldl(fun({Timestamp, Info}, AccSeparator) ->
+ Json = ?JSON_ENCODE({[
+ {key, DbName},
+ {timestamp, Timestamp},
+ {value, {Info}}
+ ]}),
+ send_chunk(Resp, AccSeparator ++ Json),
+ "," % AccSeparator now has a comma
+ end, "", Result),
+ send_chunk(Resp, "]"),
+ chttpd:end_json_response(Resp);
+ Error ->
+ throw(Error)
+ end;
+deleted_dbs_info_req(Req, _DbName) ->
+ send_method_not_allowed(Req, "GET,HEAD").
+
+deleted_dbs_post_req(#httpd{user_ctx=Ctx}=Req) ->
+ couch_httpd:verify_is_server_admin(Req),
+ chttpd:validate_ctype(Req, "application/json"),
+ {JsonProps} = chttpd:json_body_obj(Req),
+ UndeleteJson0 = couch_util:get_value(<<"undelete">>, JsonProps, undefined),
+ DeleteJson0 = couch_util:get_value(<<"delete">>, JsonProps, undefined),
+ case {UndeleteJson0, DeleteJson0} of
+ {undefined, undefined} ->
+ throw({bad_request, <<"POST body must include `undeleted` "
+ "or `delete` parameter.">>});
+ {UndeleteJson, undefined} ->
+ handle_undelete_db_req(Req, UndeleteJson);
+ {undefined, DeleteJson} ->
+ handle_delete_deleted_req(Req, DeleteJson);
+ {_Else, _Else} ->
+ throw({bad_request,
+ <<"`undeleted` and `delete` are mutually exclusive">>})
+ end.
+
+handle_undelete_db_req(#httpd{user_ctx=Ctx}=Req, {JsonProps}) ->
+ DbName = case couch_util:get_value(<<"source">>, JsonProps) of
+ undefined ->
+ throw({bad_request,
+ <<"POST body must include `source` parameter.">>});
+ DbName0 ->
+ DbName0
+ end,
+ TimeStamp = case couch_util:get_value(<<"source_timestamp">>, JsonProps) of
+ undefined ->
+ throw({bad_request,
+ <<"POST body must include `source_timestamp` parameter.">>});
+ TimeStamp0 ->
+ TimeStamp0
+ end,
+ TgtDbName = case couch_util:get_value(<<"target">>, JsonProps) of
+ undefined -> DbName;
+ TgtDbName0 -> TgtDbName0
+ end,
+ case fabric2_db:undelete(DbName, TgtDbName, TimeStamp, [{user_ctx, Ctx}]) of
+ ok ->
+ send_json(Req, 200, {[{ok, true}]});
+ {error, file_exists} ->
+ chttpd:send_error(Req, file_exists);
Review comment:
Similar to `create_db_req` https://github.com/apache/couchdb/blob/prototype/fdb-layer/src/chttpd/src/chttpd_db.erl#L386-L387, we use returned values.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r395126867
##########
File path: src/fabric/src/fabric_util.erl
##########
@@ -23,6 +23,8 @@
-export([validate_all_docs_args/2, validate_args/3]).
-export([upgrade_mrargs/1]).
-export([worker_ranges/1]).
+-export([do_recovery/0]).
Review comment:
Ah if it is an already setting that's fine. It'd still move the check for it to the module were we are reading the setting (fabric2_fdb).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r401767108
##########
File path: src/chttpd/test/eunit/chttpd_deleted_dbs_test.erl
##########
@@ -0,0 +1,206 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(chttpd_deleted_dbs_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+
+-define(USER, "chttpd_db_test_admin").
+-define(PASS, "pass").
+-define(AUTH, {basic_auth, {?USER, ?PASS}}).
+-define(CONTENT_JSON, {"Content-Type", "application/json"}).
+
+
+setup() ->
+ Hashed = couch_passwords:hash_admin_password(?PASS),
+ ok = config:set("admins", ?USER, ?b2l(Hashed), _Persist=false),
+ Addr = config:get("chttpd", "bind_address", "127.0.0.1"),
+ Port = mochiweb_socket_server:get(chttpd, port),
+ lists:concat(["http://", Addr, ":", Port, "/"]).
+
+teardown(_Url) ->
+ ok = config:delete("couchdb", "enable_database_recovery", false),
+ ok = config:delete("admins", ?USER, _Persist=false).
+
+create_db(Url) ->
+ {ok, Status, _, _} = test_request:put(Url, [?CONTENT_JSON, ?AUTH], "{}"),
+ ?assert(Status =:= 201 orelse Status =:= 202).
+
+delete_db(Url) ->
+ {ok, 200, _, _} = test_request:delete(Url, [?AUTH]).
+
+deleted_dbs_test_() ->
+ {
+ "chttpd deleted dbs tests",
+ {
+ setup,
+ fun chttpd_test_util:start_couch/0, fun chttpd_test_util:stop_couch/1,
Review comment:
List the setup/teardown function on separate lines.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r396612116
##########
File path: src/fabric/src/fabric2_db.erl
##########
@@ -213,6 +219,26 @@ delete(DbName, Options) ->
end.
+deleted_dbs_info(DbName, Options) ->
+ Options1 = lists:keystore(user_ctx, 1, Options, ?ADMIN_CTX),
Review comment:
This should be handled by chttpd.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r396604849
##########
File path: src/fabric/test/fabric2_db_crud_tests.erl
##########
@@ -139,6 +144,25 @@ recreate_db(_) ->
?assertError(database_does_not_exist, fabric2_db:open(DbName, BadOpts)).
+restore_db(_) ->
+ DbName = ?tempdb(),
+ ?assertError(database_does_not_exist, fabric2_db:delete(DbName, [])),
+
+ ?assertMatch({ok, _}, fabric2_db:create(DbName, [])),
+ ?assertEqual(true, ets:member(fabric2_server, DbName)),
+
+ ok = config:set("couchdb", "enable_database_recovery", "true", false),
Review comment:
You should add a function in the cleanup to set the value back to `false` so that we're in a known state after every test runs. Changing the config during a test which could fail leaving the config in a weird state is an easy way to cause confusion and frustration when debugging test failures.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-610668810
## Soft delete
Instead of automatically and immediately removing the data and index in the database after a delete operation, soft delete restores the deleted data back to the original state after an accidental delete or undesired delete operation, up to a defined period, such as 48 hours.
### `GET /_deleted_dbs`
Send a `GET` request to find a list of all the deleted databases in the {{site.data.keyword.cloudant_short_notm}} instance.
*Request Headers*:
```sh
Accept –
application/json
text/plain
```
{: codeblock}
| Query parameters | Description |
|------------------|-------------|
| `descending` (boolean) | Return the databases in descending order by key. Default is false. |
| `endkey` (json) | Stop returning databases when the specified key is reached. |
| `end_key` (json) | Alias for `endkey` parameter. |
| `key` (json) | Return the databases for the specified key. It can be database name or database name + timestamp.|
| `limit` (number) | Limit the number of the returned databases to the specified number. |
| `skip` (number) | Skip this number of databases before starting to return the results. Default is 0. |
| `startkey` (json) | Return databases starting with the specified key. |
| `start_key` (json) | Alias for `startkey`. |
*Response Headers*:
```sh
Content-Type –
application/json
text/plain; charset=utf-8
```
{: codeblock}
| Code | Description |
|------|-------------|
| `200 OK` | Request completed successfully. |
| `400 Bad Request` | Bad Request. Invalid payload in request. |
| `401 Unauthorized` | CouchDB Server Administrator privileges required. |
| `404 NotFound` | Invalid deleted database or timestamp. |
See the following example request:
```sh
GET /_deleted_dbs HTTP/1.1
Accept: application/json
Host: localhost:5984
```
{: codeblock}
See the following example response:
```sh
HTTP/1.1 200 OK
Cache-Control: must-revalidate
Content-Length: 52
Content-Type: application/json
Date: Tue, 23 Mar 2020 06:57:48 GMT
Server: CouchDB (Erlang/OTP)
[
{
"cluster": {
"n": 0,
"q": 0,
"r": 0,
"w": 0
},
"compact_running": false,
"data_size": 193164408719,
"db_name": "invoices",
"disk_format_version": 0,
"disk_size": 46114703224,
"instance_start_time": "0",
"purge_seq": 0,
"update_seq": "981...uUQ",
"doc_del_count": 5564,
"doc_count": 9818541,
"sizes": {
"external": 193164408719,
"views": 34961621142
},
"deleted": true,
"timestamp": "2020-04-07T23:19:12Z"
},
{
"cluster": {
"n": 0,
"q": 0,
"r": 0,
"w": 0
},
"compact_running": false,
"data_size": 0,
"db_name": "contacts",
"disk_format_version": 0,
"disk_size": 0,
"instance_start_time": "0",
"purge_seq": 0,
"update_seq": "000000000000000000000000",
"doc_del_count": 0,
"doc_count": 0,
"sizes": {
"external": 2,
"views": 0
},
"deleted": true,
"timestamp": "2020-04-07T23:19:02Z"
},
]
```
{: codeblock}
### `POST /_deleted_dbs`
{: #post-_deleted_dbs_undelete}
Send a `POST` request to restore a deleted database.
Parameters:
None
Request Headers:
```sh
Accept –
application/json
text/plain
{
"undelete": {
"timestamp" : "2020-03-23T02:26:35Z",
"source" : "db01",
"target" : "db02"
}
}
- "timestamp" - timestamp when database was deleted
- "target" could be optional and default to the source database name.
```
{: codeblock}
Response Headers:
```sh
Content-Type –
application/json
text/plain; charset=utf-8
```
{: codeblock}
| Response JSON object | Description |
|----------------------|-------------|
| `ok` (boolean) | Operation status. Available in case of success. |
| `error` (string) | Error type. Available if response code is 4xx. |
| `reason` (string) | Error description. Available if response code is 4xx. |
| Code | Message |
|--------------|---------|
| `200 Undeleted` | Database undeleted successfully. |
| `400 Bad Request` | Bad Request. Invalid payload in request. |
| `401 Unauthorized` | CouchDB Server Administrator privileges required. |
| `404 NotFound` | Invalid deleted timestamp. |
| `412 Precondition Failed` | Database already exists. |
### `DELETE /_deleted_dbs/{db}`
{: #get-_deleted_dbs-db}
Send a `DELETE` request to permanently delete the database instance which was soft-deleted with the specified timestamp.
| Parameters | Description |
|------------|-------------|
| `timestamp` | Timestamp when database was deleted |
Request Headers:
```sh
Content-Type –
application/json
```
{: codeblock}
Response Headers:
```sh
Content-Type –
application/json
text/plain; charset=utf-8
```
{: codeblock}
| Response JSON object | Description |
|----------------------|-------------|
| `ok` (boolean) | Operation status. Available in case of success. |
| `error` (string) | Error type. Available if response code is 4xx. |
| `reason` (string) | Error description. Available if response code is 4xx. |
| Code | Message |
|--------------|---------|
| `200 Deleted` | Database permanently deleted successfully. |
| `400 Bad Request` | Bad Request. Invalid payload in request. |
| `401 Unauthorized` | CouchDB Server Administrator privileges required. |
| `404 NotFound` | Invalid deleted database or timestamp. |
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400266581
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -23,12 +23,17 @@
ensure_current/1,
delete/1,
exists/1,
+ deleted_dbs_info/1,
Review comment:
Need to move this export as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-605975760
thanks @mikerhodes
hey Paul @davisp, based on new API design, I changed codes with
65a88d0 . Could you please review again?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on a change in pull request #2666:
[WIP] soft-deletion for database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r394920448
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -172,7 +174,8 @@ create(#{} = Db0, Options) ->
% Eventually DbPrefix will be HCA allocated. For now
% we're just using the DbName so that debugging is easier.
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- DbPrefix = erlfdb_tuple:pack({?DBS, DbName}, LayerPrefix),
+ #{db_prefix_allocator := DBPrefixAllocator} = erlfdb_directory:root(),
+ DbPrefix = erlfdb_hca:allocate(DBPrefixAllocator, Tx),
Review comment:
Makes sense. Although `DbPrefix` will be a bit longer, it can help better management of data.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on a change in pull request #2666:
[WIP] soft-deletion for database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r395619589
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -314,12 +317,65 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ {Mega, Secs, _} = os:timestamp(),
+ NowSecs = Mega * 1000000 + Secs,
Review comment:
It looks to be fine to use ISO_8601 format, i.e. using `:` in URL.
```
curl -u foo:bar http://127.0.0.1:15984/db01 -X GET
{"update_seq":"000000000000000000000000","cluster":{"n":0,"q":0,"r":0,"w":0},"compact_running":false,"data_size":0,"db_name":"db01","disk_format_version":0,"disk_size":0,"instance_start_time":"0","purge_seq":0,"doc_del_count":0,"doc_count":0,"sizes":{"external":2,"views":0}}
curl -u foo:bar http://127.0.0.1:15984/db01 -X DELETE
{"ok":true}
curl -u foo:bar http://127.0.0.1:15984/db01/_deleted_dbs_info
"[\"2020-03-20T11:42:54Z\"]"
curl -u foo:bar http://127.0.0.1:15984/db01?recover=2020-03-20T11:42:54Z -X PUT
{"ok":true}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn edited a comment on issue #2666: soft-deletion
for database
Posted by GitBox <gi...@apache.org>.
jiangphcn edited a comment on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-608411228
Thanks @davisp and @mikerhodes for your comments. Today I have one commit 1f439c4 which essentially implemented below API spec.
```
GET /_deleted_dbs - List deleted dbs infos
GET /_deleted_dbs?key=dbName - List specified deleted db infos
POST /_deleted_dbs - Only accepts the undelete JSON body to *restore* the deleted db
DELETE /_deleted_dbs/DbName?timestamp=2020-01-01T00:00:00 - Remove deleted db
```
From the functional set exposed to users, I think that it is similar to what @mikerhodes drafted in [1]. The difference is the API spec.
From user case and implementation's point of view, there are several points I want to mention.
1. GET /_deleted_dbs - List deleted dbs infos
We directly return dbs info instead of the deleted dbname here. The same case is for `GET /_deleted_dbs?key=dbName`. This is mainly following the approach in `_dbs_info`. I think that most likely user is not only interested in dbname, but also in some detailed information about deleted database.
```
{
"key": "db01",
"timestamp": "2020-04-03T12:09:15Z",
"value": {
"update_seq": "000000000000000000000000",
"doc_del_count": 0,
"doc_count": 0,
"sizes": {
"external": 2,
"views": 0
}
}
```
User can gradually use the progressive approach like mentioned in [1] to get the detailed information of the deleted db.
```
GET /_deleted_dbs
GET /_deleted_dbs/DbName
GET /_deleted_dbs/DbName/2020-01-01T00:00:00
```
However, looks that user most like should review information of the deletion time, doc number, disk size to decide to restore it or permanently delete it.
2. with regard to
```
GET /_deleted_dbs/DbName
GET /_deleted_dbs?key=DbName
```
In theory, we can implement both of them to get information of deleted db. @davisp also found one situation where we have to return 200 and [] if the specified db is not in deleted_db list when selecting `GET /_deleted_dbs/DbName`. This is not expected error code. So we switch to `GET /_deleted_dbs?key=DbName`.
Hope this can give some explanation how we are getting to the current status of API spec.
[1] https://gist.github.com/mikerhodes/39c66576eee468a5aeaa23c5715aec9a
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400377646
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -346,12 +351,97 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric2_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ Timestamp = list_to_binary(fabric2_util:iso8601_timestamp()),
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, Timestamp},
+ LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeletedDbKey)) of
+ not_found ->
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
+ erlfdb:clear(Tx, DbKey);
+ _Val ->
+ erlang:error({deleted_database_exists, DbName})
+ end;
+ false ->
+ erlfdb:clear(Tx, DbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix)
+ end,
bump_metadata_version(Tx),
ok.
+deleted_dbs_info(#{} = Db0) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName}, LayerPrefix),
+ DeletedDbs = erlfdb:wait(erlfdb:get_range_startswith(Tx, DeletedDbKey)),
+ lists:foldl(fun({DbKey, DbPrefix}, Acc) ->
+ DBInfo = get_info_wait(get_info_future(Tx, DbPrefix)),
+ {?DELETED_DBS, DbName, DeletedTS} =
+ erlfdb_tuple:unpack(DbKey, LayerPrefix),
+ [{DeletedTS, DBInfo}| Acc]
+ end, [], DeletedDbs).
+
+
+undelete(#{} = Db0, TgtDbName, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+ DbKey = erlfdb_tuple:pack({?ALL_DBS, TgtDbName}, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DbKey)) of
+ Bin when is_binary(Bin) ->
+ {error, file_exists};
+ not_found ->
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found, invalid_timestamp});
+ DbPrefix ->
+ erlfdb:set(Tx, DbKey, DbPrefix),
+ erlfdb:clear(Tx, DeleteDbKey),
+ bump_metadata_version(Tx),
+ ok
+ end
+ end.
+
+
+delete_deleted(#{} = Db0, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0),
+
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found});
+ DbPrefix ->
+ erlfdb:clear(Tx, DeleteDbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix),
+ bump_metadata_version(Tx)
Review comment:
I think we're ok not bumping it here if we guarantee (let's have a test for it) that any soft deleted db handle will reopen. For that we need to bump the db version when we soft-delete and ensure that to hard deletion (snapshot removal) always goes through the soft-deletion path first.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r401799054
##########
File path: src/chttpd/test/eunit/chttpd_deleted_dbs_test.erl
##########
@@ -0,0 +1,206 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(chttpd_deleted_dbs_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+
+-define(USER, "chttpd_db_test_admin").
+-define(PASS, "pass").
+-define(AUTH, {basic_auth, {?USER, ?PASS}}).
+-define(CONTENT_JSON, {"Content-Type", "application/json"}).
+
+
+setup() ->
+ Hashed = couch_passwords:hash_admin_password(?PASS),
+ ok = config:set("admins", ?USER, ?b2l(Hashed), _Persist=false),
+ Addr = config:get("chttpd", "bind_address", "127.0.0.1"),
+ Port = mochiweb_socket_server:get(chttpd, port),
+ lists:concat(["http://", Addr, ":", Port, "/"]).
+
+teardown(_Url) ->
+ ok = config:delete("couchdb", "enable_database_recovery", false),
+ ok = config:delete("admins", ?USER, _Persist=false).
+
+create_db(Url) ->
+ {ok, Status, _, _} = test_request:put(Url, [?CONTENT_JSON, ?AUTH], "{}"),
+ ?assert(Status =:= 201 orelse Status =:= 202).
+
+delete_db(Url) ->
+ {ok, 200, _, _} = test_request:delete(Url, [?AUTH]).
+
+deleted_dbs_test_() ->
+ {
+ "chttpd deleted dbs tests",
+ {
+ setup,
+ fun chttpd_test_util:start_couch/0, fun chttpd_test_util:stop_couch/1,
+ {
+ foreach,
+ fun setup/0, fun teardown/1,
+ [
+ fun should_return_error_for_deleted_dbs/1,
+ fun should_list_deleted_dbs/1,
+ fun should_list_deleted_dbs_info/1,
+ fun should_undelete_db/1,
+ fun should_remove_deleted_db/1,
+ fun should_undelete_db_to_target_db/1,
+ fun should_not_undelete_db_to_existing_db/1
+ ]
+ }
+ }
+ }.
+
+
+should_return_error_for_deleted_dbs(Url) ->
+ ?_test(begin
+ create_and_delete_db(Url),
+ NewDoc = "{\"keys\": [\"db1\"]}",
+ {ok, Code, _, ResultBody} = test_request:post(Url ++ "/_deleted_dbs/",
+ [?CONTENT_JSON, ?AUTH], NewDoc),
+
+ {Body} = jiffy:decode(ResultBody),
+ [
+ ?assertEqual(<<"bad_request">>,
+ couch_util:get_value(<<"error">>, Body)),
+ ?assertEqual(400, Code)
+ ]
+ end).
+
+
+should_list_deleted_dbs(Url) ->
+ ?_test(begin
+ DbName1 = create_and_delete_db(Url),
+ DbName2 = create_and_delete_db(Url),
+ {ok, _, _, ResultBody} = test_request:get(Url ++ "/_deleted_dbs/",
+ [?CONTENT_JSON, ?AUTH]),
+ BodyJson = jiffy:decode(ResultBody),
+ ?assertEqual(true, lists:member(DbName1, BodyJson)),
+ ?assertEqual(true, lists:member(DbName2, BodyJson))
+ end).
+
+
+should_list_deleted_dbs_info(Url) ->
+ ?_test(begin
+ DbName = create_and_delete_db(Url),
+ {ok, _, _, ResultBody} = test_request:get(Url ++ "/_deleted_dbs/" ++
+ DbName, [?CONTENT_JSON, ?AUTH]),
+ BodyJson = jiffy:decode(ResultBody),
+
+ {Db1Data} = lists:nth(1, BodyJson),
+ ?assertEqual(DbName,
+ couch_util:get_value(<<"key">>, Db1Data))
+ end).
+
+
+should_undelete_db(Url) ->
+ ?_test(begin
+ DbName = create_and_delete_db(Url),
+ {ok, _, _, ResultBody} = test_request:get(Url ++ "/_deleted_dbs/" ++
+ DbName, [?CONTENT_JSON, ?AUTH]),
+ BodyJson = jiffy:decode(ResultBody),
+
+ {Db1Data} = lists:nth(1, BodyJson),
+ TimeStamp = couch_util:get_value(<<"timestamp">>, Db1Data),
+
+ NewDoc = "{\"undelete\": {\"source\": \"" ++ ?b2l(DbName) ++
Review comment:
NewDoc doesn't seem like a good variable name here. Also this would be cleaner if you just construct the Erlang representation and encode it I think:
```erlang
ErlJSON = {[{undelete, {[
{source, DbName},
{source_timestamp, TimeStamp}
]}}]},
Body = jiffy:encode(ErlJSON),
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r402441847
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -1172,6 +1261,45 @@ check_db_version(#{} = Db, CheckDbVersion) ->
end.
+soft_delete_db(Db) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix,
+ db_prefix := DbPrefix
+ } = ensure_current(Db),
+
+ DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
+ Timestamp = list_to_binary(fabric2_util:iso8601_timestamp()),
+ DeletedDbKeyTuple = {?DELETED_DBS, DbName, Timestamp},
+ DeletedDbKey = erlfdb_tuple:pack(DeletedDbKeyTuple, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeletedDbKey)) of
+ not_found ->
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
+ erlfdb:clear(Tx, DbKey),
+ bump_db_version(Db);
+ _Val ->
+ erlang:error({deleted_database_exists, DbName})
Review comment:
This error message isn't quite right since we can have multiple deleted databases, just not at the same timestamp. How about `{deletion_frequency_exceeded, DbName}`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn edited a comment on issue #2666: soft-deletion
for database
Posted by GitBox <gi...@apache.org>.
jiangphcn edited a comment on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-608646035
hey @davisp
I think that you are **not** the only one that's been assuming that this is a mirror of the GET _dbs_info API end point and working from that perspective. From https://github.com/apache/couchdb/pull/2666/#issuecomment-608411228, you can see that I am for that and implemented with this approach.
In addition, I don't support the suggestion about `the GET /_deleted_dbs/DbName/Timestamp API`. As i mentioned before, it looks that user most likely should review information of the deletion time, doc number, disk size to decide to restore it or permanently delete it. It is convenient to return DbInfo one time instead of using the progressive approach like mentioned in [1] to get the detailed information of the deleted db.
```
GET /_deleted_dbs
GET /_deleted_dbs/DbName
GET /_deleted_dbs/DbName/2020-01-01T00:00:00
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
davisp commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-603909503
Also for the `POST /_deleted_dbs/_undelete` I'll also note that we do have a set of system database names that start with an underscore (`_dbs/_nodes/_users`). Obviously we don't have an `_undelete` database but it is technically mixing that API endpoint into the valid range of database names.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r394813145
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -172,7 +174,8 @@ create(#{} = Db0, Options) ->
% Eventually DbPrefix will be HCA allocated. For now
% we're just using the DbName so that debugging is easier.
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- DbPrefix = erlfdb_tuple:pack({?DBS, DbName}, LayerPrefix),
+ #{db_prefix_allocator := DBPrefixAllocator} = erlfdb_directory:root(),
+ DbPrefix = erlfdb_hca:allocate(DBPrefixAllocator, Tx),
Review comment:
We want to generate small prefixes but I we'd also want to keep the data under the `LayerPrefix` subspace. This might become important for backups and such or any places where we do range reads over the whole "CouchDB" instances sharing the same FDB cluster for example.
Then it might be something like
```
AllocPrefix = erlfdb_hca:allocate(Allocator, Tx),
DbPrefix = erlfdb_tuple:pack({?DBS, AllocPrefix}, LayerPrefix)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r402439777
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -365,6 +364,65 @@ exists(#{name := DbName} = Db) when is_binary(DbName) ->
end.
+undelete(#{} = Db0, TgtDbName, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+ DbKey = erlfdb_tuple:pack({?ALL_DBS, TgtDbName}, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DbKey)) of
+ Bin when is_binary(Bin) ->
+ file_exists;
+ not_found ->
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found});
Review comment:
You resolved this but you don't appear to have addressed the issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on a change in pull request #2666:
[WIP] soft-deletion for database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r394802252
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -314,12 +317,65 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ {Mega, Secs, _} = os:timestamp(),
+ NowSecs = Mega * 1000000 + Secs,
Review comment:
yes, thanks @nickva for comments.
I changed this locally with
```
{{Y, Mon, D}, {H, Min, S}} = calendar:universal_time(),
NowSecs = list_to_binary(lists:flatten(
io_lib:format("~w~2.10.0B~2.10.0B."
++ "~2.10.0B~2.10.0B~2.10.0B",
[Y, Mon, D, H, Min, S])
)),
DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, NowSecs},
LayerPrefix),
```
and will push new commit today with other changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r401730663
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -346,12 +351,97 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric2_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ Timestamp = list_to_binary(fabric2_util:iso8601_timestamp()),
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, Timestamp},
+ LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeletedDbKey)) of
+ not_found ->
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
+ erlfdb:clear(Tx, DbKey);
+ _Val ->
+ erlang:error({deleted_database_exists, DbName})
+ end;
+ false ->
+ erlfdb:clear(Tx, DbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix)
+ end,
bump_metadata_version(Tx),
ok.
+deleted_dbs_info(#{} = Db0) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName}, LayerPrefix),
+ DeletedDbs = erlfdb:wait(erlfdb:get_range_startswith(Tx, DeletedDbKey)),
+ lists:foldl(fun({DbKey, DbPrefix}, Acc) ->
+ DBInfo = get_info_wait(get_info_future(Tx, DbPrefix)),
+ {?DELETED_DBS, DbName, DeletedTS} =
+ erlfdb_tuple:unpack(DbKey, LayerPrefix),
+ [{DeletedTS, DBInfo}| Acc]
+ end, [], DeletedDbs).
+
+
+undelete(#{} = Db0, TgtDbName, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+ DbKey = erlfdb_tuple:pack({?ALL_DBS, TgtDbName}, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DbKey)) of
+ Bin when is_binary(Bin) ->
+ {error, file_exists};
+ not_found ->
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found, invalid_timestamp});
+ DbPrefix ->
+ erlfdb:set(Tx, DbKey, DbPrefix),
+ erlfdb:clear(Tx, DeleteDbKey),
+ bump_metadata_version(Tx),
+ ok
+ end
+ end.
+
+
+delete_deleted(#{} = Db0, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0),
+
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found});
+ DbPrefix ->
+ erlfdb:clear(Tx, DeleteDbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix),
+ bump_metadata_version(Tx)
Review comment:
Great suggestion @nickva on test case. I added it in https://github.com/apache/couchdb/pull/2666/commits/b1d2b08afb80ca9f8d8f4b9ca37ad65cd54b3352#diff-5c0948df6382504d34bd12ee0d4485d0R197-R250.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
davisp commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-607973871
Also, something I noticed while playing with the tests is that the `GET /_deleted_dbs/DbName` is not very ergonomic. Due to how things are implemented, if we don't have any deleted snapshots we still return a `200` response with an empty array `[]` as the body. I would have expected a `404` in that situation given that the `DbName` is part of the URL. I realize that's purely due to how things are implemented, but it suggests to me that the `GET /_deleted_dbs?key=DbName` approach is better since we'd be more likely to understand that a `200` and empty array returned is expected since its more like a view query where results might or might not exist.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r394813145
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -172,7 +174,8 @@ create(#{} = Db0, Options) ->
% Eventually DbPrefix will be HCA allocated. For now
% we're just using the DbName so that debugging is easier.
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- DbPrefix = erlfdb_tuple:pack({?DBS, DbName}, LayerPrefix),
+ #{db_prefix_allocator := DBPrefixAllocator} = erlfdb_directory:root(),
+ DbPrefix = erlfdb_hca:allocate(DBPrefixAllocator, Tx),
Review comment:
We want to generate small prefixes but we also want to keep the data under the `LayerPrefix` subspace. This might become important for backups and such or any places where we do range reads over the whole "CouchDB" instances sharing the same FDB cluster for example.
Then it might be something like
```
AllocPrefix = erlfdb_hca:allocate(Allocator, Tx),
DbPrefix = erlfdb_tuple:pack({?DBS, AllocPrefix}, LayerPrefix)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400287905
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -346,12 +351,97 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric2_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ Timestamp = list_to_binary(fabric2_util:iso8601_timestamp()),
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, Timestamp},
+ LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeletedDbKey)) of
+ not_found ->
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
+ erlfdb:clear(Tx, DbKey);
+ _Val ->
+ erlang:error({deleted_database_exists, DbName})
+ end;
+ false ->
+ erlfdb:clear(Tx, DbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix)
+ end,
bump_metadata_version(Tx),
ok.
+deleted_dbs_info(#{} = Db0) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName}, LayerPrefix),
Review comment:
Extra space after the`=` operator.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r401799728
##########
File path: src/chttpd/test/eunit/chttpd_deleted_dbs_test.erl
##########
@@ -0,0 +1,206 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(chttpd_deleted_dbs_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+
+-define(USER, "chttpd_db_test_admin").
+-define(PASS, "pass").
+-define(AUTH, {basic_auth, {?USER, ?PASS}}).
+-define(CONTENT_JSON, {"Content-Type", "application/json"}).
+
+
+setup() ->
+ Hashed = couch_passwords:hash_admin_password(?PASS),
+ ok = config:set("admins", ?USER, ?b2l(Hashed), _Persist=false),
+ Addr = config:get("chttpd", "bind_address", "127.0.0.1"),
+ Port = mochiweb_socket_server:get(chttpd, port),
+ lists:concat(["http://", Addr, ":", Port, "/"]).
+
+teardown(_Url) ->
+ ok = config:delete("couchdb", "enable_database_recovery", false),
+ ok = config:delete("admins", ?USER, _Persist=false).
+
+create_db(Url) ->
+ {ok, Status, _, _} = test_request:put(Url, [?CONTENT_JSON, ?AUTH], "{}"),
+ ?assert(Status =:= 201 orelse Status =:= 202).
+
+delete_db(Url) ->
+ {ok, 200, _, _} = test_request:delete(Url, [?AUTH]).
+
+deleted_dbs_test_() ->
+ {
+ "chttpd deleted dbs tests",
+ {
+ setup,
+ fun chttpd_test_util:start_couch/0, fun chttpd_test_util:stop_couch/1,
+ {
+ foreach,
+ fun setup/0, fun teardown/1,
+ [
+ fun should_return_error_for_deleted_dbs/1,
+ fun should_list_deleted_dbs/1,
+ fun should_list_deleted_dbs_info/1,
+ fun should_undelete_db/1,
+ fun should_remove_deleted_db/1,
+ fun should_undelete_db_to_target_db/1,
+ fun should_not_undelete_db_to_existing_db/1
+ ]
+ }
+ }
+ }.
+
+
+should_return_error_for_deleted_dbs(Url) ->
+ ?_test(begin
+ create_and_delete_db(Url),
+ NewDoc = "{\"keys\": [\"db1\"]}",
+ {ok, Code, _, ResultBody} = test_request:post(Url ++ "/_deleted_dbs/",
+ [?CONTENT_JSON, ?AUTH], NewDoc),
+
+ {Body} = jiffy:decode(ResultBody),
+ [
+ ?assertEqual(<<"bad_request">>,
+ couch_util:get_value(<<"error">>, Body)),
+ ?assertEqual(400, Code)
+ ]
+ end).
+
+
+should_list_deleted_dbs(Url) ->
+ ?_test(begin
+ DbName1 = create_and_delete_db(Url),
+ DbName2 = create_and_delete_db(Url),
+ {ok, _, _, ResultBody} = test_request:get(Url ++ "/_deleted_dbs/",
+ [?CONTENT_JSON, ?AUTH]),
+ BodyJson = jiffy:decode(ResultBody),
+ ?assertEqual(true, lists:member(DbName1, BodyJson)),
+ ?assertEqual(true, lists:member(DbName2, BodyJson))
+ end).
+
+
+should_list_deleted_dbs_info(Url) ->
+ ?_test(begin
+ DbName = create_and_delete_db(Url),
+ {ok, _, _, ResultBody} = test_request:get(Url ++ "/_deleted_dbs/" ++
+ DbName, [?CONTENT_JSON, ?AUTH]),
+ BodyJson = jiffy:decode(ResultBody),
+
+ {Db1Data} = lists:nth(1, BodyJson),
+ ?assertEqual(DbName,
+ couch_util:get_value(<<"key">>, Db1Data))
+ end).
+
+
+should_undelete_db(Url) ->
+ ?_test(begin
+ DbName = create_and_delete_db(Url),
+ {ok, _, _, ResultBody} = test_request:get(Url ++ "/_deleted_dbs/" ++
+ DbName, [?CONTENT_JSON, ?AUTH]),
+ BodyJson = jiffy:decode(ResultBody),
+
+ {Db1Data} = lists:nth(1, BodyJson),
+ TimeStamp = couch_util:get_value(<<"timestamp">>, Db1Data),
+
+ NewDoc = "{\"undelete\": {\"source\": \"" ++ ?b2l(DbName) ++
+ "\", \"source_timestamp\":\"" ++ ?b2l(TimeStamp) ++ "\"}}",
+
+ {ok, Status, _, _} = test_request:post(Url ++ "/_deleted_dbs",
+ [?CONTENT_JSON, ?AUTH], NewDoc),
+ ?assertEqual(200, Status),
+
+ {ok, Status2, _, _} = test_request:get(Url ++ DbName,
+ [?CONTENT_JSON, ?AUTH], NewDoc),
+ ?assertEqual(200, Status2)
+ end).
+
+
+should_remove_deleted_db(Url) ->
+ ?_test(begin
+ DbName = create_and_delete_db(Url),
+ {ok, _, _, ResultBody} = test_request:get(Url ++ "/_deleted_dbs/" ++
+ DbName, [?CONTENT_JSON, ?AUTH]),
+ BodyJson = jiffy:decode(ResultBody),
+
+ {Db1Data} = lists:nth(1, BodyJson),
+ TimeStamp = couch_util:get_value(<<"timestamp">>, Db1Data),
+
+ NewDoc = "{\"delete\": {\"source\": \"" ++ ?b2l(DbName) ++
+ "\", \"source_timestamp\":\"" ++ ?b2l(TimeStamp) ++ "\"}}",
+
+ {ok, Status, _, _} = test_request:post(Url ++ "/_deleted_dbs",
+ [?CONTENT_JSON, ?AUTH], NewDoc),
+ ?assertEqual(200, Status)
Review comment:
You should add an assertion here that the entry from`_deleted_dbs/DbName` is now gone.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r394810354
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -172,7 +174,8 @@ create(#{} = Db0, Options) ->
% Eventually DbPrefix will be HCA allocated. For now
% we're just using the DbName so that debugging is easier.
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- DbPrefix = erlfdb_tuple:pack({?DBS, DbName}, LayerPrefix),
+ #{db_prefix_allocator := DBPrefixAllocator} = erlfdb_directory:root(),
Review comment:
Maybe we'd want an allocator instance under the LayerPrefix and not use the main directory one just to keep things isolated. Without reading the hca code too closely, I assume that's where it would store its counters. Maybe something like `Allocator = erlfdb_hca:create(erlfdb_tuple:pack({?HCA}, LayerPrefix))`
We should do a quick benchmark at least to see if we see considerable contention when making lots of new databases in parallel. I suspect we won't but it's worth checking it out perhaps...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r402442742
##########
File path: src/fabric/src/fabric2_util.erl
##########
@@ -40,7 +40,9 @@
encode_all_doc_key/1,
pmap/2,
- pmap/3
+ pmap/3,
+ iso8601_timestamp/0,
+ do_recovery/0
Review comment:
Super minor nit but lest move these above `pmap/N` into their own section. Don't forget to move the implementations as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r396594839
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -177,10 +181,13 @@ create(#{} = Db0, Options) ->
layer_prefix := LayerPrefix
} = Db = ensure_current(Db0, false),
- % Eventually DbPrefix will be HCA allocated. For now
- % we're just using the DbName so that debugging is easier.
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- DbPrefix = erlfdb_tuple:pack({?DBS, DbName}, LayerPrefix),
+ DefDbPref = ?DEFAULT_DB_PREFIX,
+ AllDbPrefix = erlfdb_util:get(Options, db_prefix, DefDbPref),
+ DbId = erlfdb_tuple:pack({AllDbPrefix}, AllDbPrefix),
+ DbPrefixAllocator = erlfdb_hca:create(erlfdb_tuple:pack({DbId}, <<"hca">>)),
+ AllocPrefix = erlfdb_hca:allocate(DbPrefixAllocator, Tx),
+ DbPrefix = erlfdb_tuple:pack({?DBS, AllocPrefix}, LayerPrefix),
Review comment:
There's a lot of unnecessary going on here. You should just need something like:
```erlang
HCAKey = erlfdb_tuple:pack({?DB_HCA}, LayerPrefix),
HCA = erlfdb_hca:create(HCAKey),
DbPrefix = erlfdb_hca:allocate(HCA).
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r394814590
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -314,12 +317,65 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ {Mega, Secs, _} = os:timestamp(),
+ NowSecs = Mega * 1000000 + Secs,
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, NowSecs},
+ LayerPrefix),
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
+ erlfdb:clear(Tx, DbKey);
+ false ->
+ erlfdb:clear(Tx, DbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix)
+ end,
bump_metadata_version(Tx),
ok.
+deleted_dbs_info(#{} = Db0) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName}, LayerPrefix),
+ DeletedDbs = erlfdb:wait(erlfdb:get_range_startswith(Tx, DeletedDbKey)),
+ lists:foldl(fun({DbKey, DbPrefix}, Acc) ->
+ DBInfo = get_info_wait(get_info_future(Tx, DbPrefix)),
+ {?DELETED_DBS, DbName, DeletedTS} =
+ erlfdb_tuple:unpack(DbKey, LayerPrefix),
+ [DeletedTS| Acc]
+ end, [], DeletedDbs).
+
+
+restore(#{} = Db0, [DeletedTS|_]) ->
Review comment:
Do we expect any trailing path items, wondering about the `|_` part ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400377646
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -346,12 +351,97 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric2_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ Timestamp = list_to_binary(fabric2_util:iso8601_timestamp()),
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, Timestamp},
+ LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeletedDbKey)) of
+ not_found ->
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
+ erlfdb:clear(Tx, DbKey);
+ _Val ->
+ erlang:error({deleted_database_exists, DbName})
+ end;
+ false ->
+ erlfdb:clear(Tx, DbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix)
+ end,
bump_metadata_version(Tx),
ok.
+deleted_dbs_info(#{} = Db0) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName}, LayerPrefix),
+ DeletedDbs = erlfdb:wait(erlfdb:get_range_startswith(Tx, DeletedDbKey)),
+ lists:foldl(fun({DbKey, DbPrefix}, Acc) ->
+ DBInfo = get_info_wait(get_info_future(Tx, DbPrefix)),
+ {?DELETED_DBS, DbName, DeletedTS} =
+ erlfdb_tuple:unpack(DbKey, LayerPrefix),
+ [{DeletedTS, DBInfo}| Acc]
+ end, [], DeletedDbs).
+
+
+undelete(#{} = Db0, TgtDbName, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+ DbKey = erlfdb_tuple:pack({?ALL_DBS, TgtDbName}, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DbKey)) of
+ Bin when is_binary(Bin) ->
+ {error, file_exists};
+ not_found ->
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found, invalid_timestamp});
+ DbPrefix ->
+ erlfdb:set(Tx, DbKey, DbPrefix),
+ erlfdb:clear(Tx, DeleteDbKey),
+ bump_metadata_version(Tx),
+ ok
+ end
+ end.
+
+
+delete_deleted(#{} = Db0, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0),
+
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found});
+ DbPrefix ->
+ erlfdb:clear(Tx, DeleteDbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix),
+ bump_metadata_version(Tx)
Review comment:
I think we're ok not bumping it here if we guarantee (let's have a test for it) that any soft deleted db handle will reopen. For that we need to bump the db version when we soft-delete and ensure that and then perhaps during hard deletion (snapshot removal) always goes through the soft-deletion path first.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r395126867
##########
File path: src/fabric/src/fabric_util.erl
##########
@@ -23,6 +23,8 @@
-export([validate_all_docs_args/2, validate_args/3]).
-export([upgrade_mrargs/1]).
-export([worker_ranges/1]).
+-export([do_recovery/0]).
Review comment:
Ah if it is a setting already that's fine. It'd still move the check for it to the module were we are reading the setting (fabric2_fdb).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r401800271
##########
File path: src/chttpd/test/eunit/chttpd_deleted_dbs_test.erl
##########
@@ -0,0 +1,206 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(chttpd_deleted_dbs_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+
+-define(USER, "chttpd_db_test_admin").
+-define(PASS, "pass").
+-define(AUTH, {basic_auth, {?USER, ?PASS}}).
+-define(CONTENT_JSON, {"Content-Type", "application/json"}).
+
+
+setup() ->
+ Hashed = couch_passwords:hash_admin_password(?PASS),
+ ok = config:set("admins", ?USER, ?b2l(Hashed), _Persist=false),
+ Addr = config:get("chttpd", "bind_address", "127.0.0.1"),
+ Port = mochiweb_socket_server:get(chttpd, port),
+ lists:concat(["http://", Addr, ":", Port, "/"]).
+
+teardown(_Url) ->
+ ok = config:delete("couchdb", "enable_database_recovery", false),
+ ok = config:delete("admins", ?USER, _Persist=false).
+
+create_db(Url) ->
+ {ok, Status, _, _} = test_request:put(Url, [?CONTENT_JSON, ?AUTH], "{}"),
+ ?assert(Status =:= 201 orelse Status =:= 202).
+
+delete_db(Url) ->
+ {ok, 200, _, _} = test_request:delete(Url, [?AUTH]).
+
+deleted_dbs_test_() ->
+ {
+ "chttpd deleted dbs tests",
+ {
+ setup,
+ fun chttpd_test_util:start_couch/0, fun chttpd_test_util:stop_couch/1,
+ {
+ foreach,
+ fun setup/0, fun teardown/1,
+ [
+ fun should_return_error_for_deleted_dbs/1,
+ fun should_list_deleted_dbs/1,
+ fun should_list_deleted_dbs_info/1,
+ fun should_undelete_db/1,
+ fun should_remove_deleted_db/1,
+ fun should_undelete_db_to_target_db/1,
+ fun should_not_undelete_db_to_existing_db/1
+ ]
+ }
+ }
+ }.
+
+
+should_return_error_for_deleted_dbs(Url) ->
+ ?_test(begin
+ create_and_delete_db(Url),
+ NewDoc = "{\"keys\": [\"db1\"]}",
+ {ok, Code, _, ResultBody} = test_request:post(Url ++ "/_deleted_dbs/",
+ [?CONTENT_JSON, ?AUTH], NewDoc),
+
+ {Body} = jiffy:decode(ResultBody),
+ [
+ ?assertEqual(<<"bad_request">>,
+ couch_util:get_value(<<"error">>, Body)),
+ ?assertEqual(400, Code)
+ ]
+ end).
+
+
+should_list_deleted_dbs(Url) ->
+ ?_test(begin
+ DbName1 = create_and_delete_db(Url),
+ DbName2 = create_and_delete_db(Url),
+ {ok, _, _, ResultBody} = test_request:get(Url ++ "/_deleted_dbs/",
+ [?CONTENT_JSON, ?AUTH]),
+ BodyJson = jiffy:decode(ResultBody),
+ ?assertEqual(true, lists:member(DbName1, BodyJson)),
+ ?assertEqual(true, lists:member(DbName2, BodyJson))
+ end).
+
+
+should_list_deleted_dbs_info(Url) ->
+ ?_test(begin
+ DbName = create_and_delete_db(Url),
+ {ok, _, _, ResultBody} = test_request:get(Url ++ "/_deleted_dbs/" ++
+ DbName, [?CONTENT_JSON, ?AUTH]),
+ BodyJson = jiffy:decode(ResultBody),
+
+ {Db1Data} = lists:nth(1, BodyJson),
+ ?assertEqual(DbName,
+ couch_util:get_value(<<"key">>, Db1Data))
+ end).
+
+
+should_undelete_db(Url) ->
+ ?_test(begin
+ DbName = create_and_delete_db(Url),
+ {ok, _, _, ResultBody} = test_request:get(Url ++ "/_deleted_dbs/" ++
+ DbName, [?CONTENT_JSON, ?AUTH]),
+ BodyJson = jiffy:decode(ResultBody),
+
+ {Db1Data} = lists:nth(1, BodyJson),
+ TimeStamp = couch_util:get_value(<<"timestamp">>, Db1Data),
+
+ NewDoc = "{\"undelete\": {\"source\": \"" ++ ?b2l(DbName) ++
+ "\", \"source_timestamp\":\"" ++ ?b2l(TimeStamp) ++ "\"}}",
+
+ {ok, Status, _, _} = test_request:post(Url ++ "/_deleted_dbs",
+ [?CONTENT_JSON, ?AUTH], NewDoc),
+ ?assertEqual(200, Status),
+
+ {ok, Status2, _, _} = test_request:get(Url ++ DbName,
+ [?CONTENT_JSON, ?AUTH], NewDoc),
+ ?assertEqual(200, Status2)
+ end).
+
+
+should_remove_deleted_db(Url) ->
+ ?_test(begin
+ DbName = create_and_delete_db(Url),
+ {ok, _, _, ResultBody} = test_request:get(Url ++ "/_deleted_dbs/" ++
+ DbName, [?CONTENT_JSON, ?AUTH]),
+ BodyJson = jiffy:decode(ResultBody),
+
+ {Db1Data} = lists:nth(1, BodyJson),
+ TimeStamp = couch_util:get_value(<<"timestamp">>, Db1Data),
+
+ NewDoc = "{\"delete\": {\"source\": \"" ++ ?b2l(DbName) ++
+ "\", \"source_timestamp\":\"" ++ ?b2l(TimeStamp) ++ "\"}}",
+
+ {ok, Status, _, _} = test_request:post(Url ++ "/_deleted_dbs",
+ [?CONTENT_JSON, ?AUTH], NewDoc),
+ ?assertEqual(200, Status)
+ end).
+
+
+should_undelete_db_to_target_db(Url) ->
+ ?_test(begin
+ DbName = create_and_delete_db(Url),
+ {ok, _, _, ResultBody} = test_request:get(Url ++ "/_deleted_dbs/" ++
+ DbName, [?CONTENT_JSON, ?AUTH]),
+ BodyJson = jiffy:decode(ResultBody),
+
+ {Db1Data} = lists:nth(1, BodyJson),
+ TimeStamp = couch_util:get_value(<<"timestamp">>, Db1Data),
+
+ NewDbName = ?tempdb(),
+ NewDoc = "{\"undelete\": {\"source\": \"" ++ ?b2l(DbName) ++
+ "\", \"source_timestamp\":\"" ++ ?b2l(TimeStamp) ++
+ "\", \"target\": \"" ++ ?b2l(NewDbName) ++ "\" }}",
+
+ {ok, RC2, _, _} = test_request:post(Url ++ "/_deleted_dbs",
+ [?CONTENT_JSON, ?AUTH], NewDoc),
+ ?assertEqual(200, RC2),
+
+ {ok, RC3, _, _} = test_request:get(Url ++ NewDbName,
+ [?CONTENT_JSON, ?AUTH]),
+ ?assertEqual(200, RC3)
+ end).
+
+
+should_not_undelete_db_to_existing_db(Url) ->
+ ?_test(begin
+ DbName = create_and_delete_db(Url),
+ {ok, _, _, ResultBody} = test_request:get(Url ++ "/_deleted_dbs/" ++
+ DbName, [?CONTENT_JSON, ?AUTH]),
+ BodyJson = jiffy:decode(ResultBody),
+
+ {Db1Data} = lists:nth(1, BodyJson),
+ TimeStamp = couch_util:get_value(<<"timestamp">>, Db1Data),
+
+ NewDbName = ?tempdb(),
+ create_db(Url ++ NewDbName),
+ NewDoc = "{\"undelete\": {\"source\": \"" ++ ?b2l(DbName) ++
+ "\", \"source_timestamp\":\"" ++ ?b2l(TimeStamp) ++
+ "\", \"target\": \"" ++ ?b2l(NewDbName) ++ "\" }}",
+
+ {ok, RC, _, Body} = test_request:post(Url ++ "/_deleted_dbs",
+ [?CONTENT_JSON, ?AUTH], NewDoc),
+ {JsonBody} = jiffy:decode(Body),
+ [
Review comment:
Another place where we shouldn't be using arrays for assertions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r402437795
##########
File path: src/fabric/src/fabric2_db.erl
##########
@@ -204,12 +211,37 @@ delete(DbName, Options) ->
% Delete doesn't check user_ctx, that's done at the HTTP API level
% here we just care to get the `database_does_not_exist` error thrown
Options1 = lists:keystore(user_ctx, 1, Options, ?ADMIN_CTX),
- {ok, Db} = open(DbName, Options1),
- Resp = fabric2_fdb:transactional(Db, fun(TxDb) ->
- fabric2_fdb:delete(TxDb)
+ case lists:keyfind(deleted_at, 1, Options1) of
+ {deleted_at, TimeStamp} ->
+ fabric2_fdb:transactional(DbName, Options1, fun(TxDb) ->
+ fabric2_fdb:remove_deleted_db(TxDb, TimeStamp)
+ end);
+ false ->
+ {ok, Db} = open(DbName, Options1),
+ Resp = fabric2_fdb:transactional(Db, fun(TxDb) ->
+ fabric2_fdb:delete(TxDb)
+ end),
+ if Resp /= ok -> Resp; true ->
+ fabric2_server:remove(DbName)
+ end
+ end.
+
+
+deleted_dbs_info(DbName, Options) ->
Review comment:
This should be moved to after list_dbs_info so that it matches the order in the exports list.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r402982473
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -365,6 +364,65 @@ exists(#{name := DbName} = Db) when is_binary(DbName) ->
end.
+undelete(#{} = Db0, TgtDbName, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+ DbKey = erlfdb_tuple:pack({?ALL_DBS, TgtDbName}, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DbKey)) of
+ Bin when is_binary(Bin) ->
+ file_exists;
+ not_found ->
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found});
+ DbPrefix ->
+ erlfdb:set(Tx, DbKey, DbPrefix),
+ erlfdb:clear(Tx, DeleteDbKey),
+ bump_db_version(#{
+ tx => Tx,
+ db_prefix => DbPrefix
+ }),
+ ok
+ end
+ end.
+
+
+remove_deleted_db(#{} = Db0, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeletedDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeletedDbKey)) of
+ not_found ->
+ erlang:error({not_found});
Review comment:
I made change locally and didn't `git push` for this. The change was reflected in eefd115 as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r402293714
##########
File path: src/chttpd/test/eunit/chttpd_deleted_dbs_test.erl
##########
@@ -0,0 +1,206 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(chttpd_deleted_dbs_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+
+-define(USER, "chttpd_db_test_admin").
+-define(PASS, "pass").
+-define(AUTH, {basic_auth, {?USER, ?PASS}}).
+-define(CONTENT_JSON, {"Content-Type", "application/json"}).
+
+
+setup() ->
+ Hashed = couch_passwords:hash_admin_password(?PASS),
+ ok = config:set("admins", ?USER, ?b2l(Hashed), _Persist=false),
+ Addr = config:get("chttpd", "bind_address", "127.0.0.1"),
+ Port = mochiweb_socket_server:get(chttpd, port),
+ lists:concat(["http://", Addr, ":", Port, "/"]).
+
+teardown(_Url) ->
+ ok = config:delete("couchdb", "enable_database_recovery", false),
+ ok = config:delete("admins", ?USER, _Persist=false).
+
+create_db(Url) ->
+ {ok, Status, _, _} = test_request:put(Url, [?CONTENT_JSON, ?AUTH], "{}"),
+ ?assert(Status =:= 201 orelse Status =:= 202).
+
+delete_db(Url) ->
+ {ok, 200, _, _} = test_request:delete(Url, [?AUTH]).
+
+deleted_dbs_test_() ->
+ {
+ "chttpd deleted dbs tests",
+ {
+ setup,
+ fun chttpd_test_util:start_couch/0, fun chttpd_test_util:stop_couch/1,
+ {
+ foreach,
+ fun setup/0, fun teardown/1,
+ [
+ fun should_return_error_for_deleted_dbs/1,
+ fun should_list_deleted_dbs/1,
+ fun should_list_deleted_dbs_info/1,
+ fun should_undelete_db/1,
+ fun should_remove_deleted_db/1,
+ fun should_undelete_db_to_target_db/1,
+ fun should_not_undelete_db_to_existing_db/1
+ ]
+ }
+ }
+ }.
+
+
+should_return_error_for_deleted_dbs(Url) ->
+ ?_test(begin
+ create_and_delete_db(Url),
+ NewDoc = "{\"keys\": [\"db1\"]}",
+ {ok, Code, _, ResultBody} = test_request:post(Url ++ "/_deleted_dbs/",
+ [?CONTENT_JSON, ?AUTH], NewDoc),
Review comment:
Originally, `POST` is not supported for `/_deleted_dbs` endpoint, so I added one test to check unsupported method. Now I use `DELETE` instead.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] mikerhodes commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
mikerhodes commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-609459407
@davisp @jiangphcn Apologies if I missed that we were mirroring `_dbs_info` in our API. That makes things a bit clearer around use of `key`. Flipping between internal structures (`DbInfo`) and external APIs (`/_dbs_info`) probably got in the way of my understanding.
I didn't particularly think `_all_dbs` was that useful, but it seemed like we were not following any particular pattern so I just picked the one that required me to type fewest characters to demonstrate the idea (an idea which doesn't make much sense now I understand the motivation behind the older API, which I'm not sure anyone really spelled out in the mailing list or here before now, but I probably missed something somewhere 😞 ).
Anyway, I think we've reached a decent stopping point here (given that I think mostly I've just got everyone to justify an already good solution tbh rather than moving us forward), so let's move forward and not make @jiangphcn implement yet another thing 😉 Nice work all in all.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r401724116
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -346,12 +351,97 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric2_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ Timestamp = list_to_binary(fabric2_util:iso8601_timestamp()),
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, Timestamp},
+ LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeletedDbKey)) of
+ not_found ->
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
+ erlfdb:clear(Tx, DbKey);
+ _Val ->
+ erlang:error({deleted_database_exists, DbName})
+ end;
+ false ->
+ erlfdb:clear(Tx, DbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix)
+ end,
bump_metadata_version(Tx),
ok.
+deleted_dbs_info(#{} = Db0) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName}, LayerPrefix),
+ DeletedDbs = erlfdb:wait(erlfdb:get_range_startswith(Tx, DeletedDbKey)),
+ lists:foldl(fun({DbKey, DbPrefix}, Acc) ->
+ DBInfo = get_info_wait(get_info_future(Tx, DbPrefix)),
+ {?DELETED_DBS, DbName, DeletedTS} =
+ erlfdb_tuple:unpack(DbKey, LayerPrefix),
+ [{DeletedTS, DBInfo}| Acc]
+ end, [], DeletedDbs).
+
+
+undelete(#{} = Db0, TgtDbName, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+ DbKey = erlfdb_tuple:pack({?ALL_DBS, TgtDbName}, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DbKey)) of
+ Bin when is_binary(Bin) ->
+ {error, file_exists};
+ not_found ->
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found, invalid_timestamp});
+ DbPrefix ->
+ erlfdb:set(Tx, DbKey, DbPrefix),
+ erlfdb:clear(Tx, DeleteDbKey),
+ bump_metadata_version(Tx),
+ ok
+ end
+ end.
+
+
+delete_deleted(#{} = Db0, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0),
+
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found});
+ DbPrefix ->
+ erlfdb:clear(Tx, DeleteDbKey),
+ erlfdb:clear_range_startswith(Tx, DbPrefix),
+ bump_metadata_version(Tx)
Review comment:
Actually I was wrong about the `db soft deleted and undeleted`. Thanks, @jiangphcn for pointing it out. It seems in that case the old handle will be usable because:
1) we bumped the db version only
2) old handle would notice metadata and db version being out of date
3) it would automatically re-open
4) it would continue to be used
We could assert that in the test perhaps then. Good catch @jiangphcn !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r402440239
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -365,6 +364,65 @@ exists(#{name := DbName} = Db) when is_binary(DbName) ->
end.
+undelete(#{} = Db0, TgtDbName, TimeStamp) ->
+ #{
+ name := DbName,
+ tx := Tx,
+ layer_prefix := LayerPrefix
+ } = ensure_current(Db0, false),
+ DbKey = erlfdb_tuple:pack({?ALL_DBS, TgtDbName}, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DbKey)) of
+ Bin when is_binary(Bin) ->
+ file_exists;
+ not_found ->
+ DeletedDbTupleKey = {
+ ?DELETED_DBS,
+ DbName,
+ TimeStamp
+ },
+ DeleteDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
+ case erlfdb:wait(erlfdb:get(Tx, DeleteDbKey)) of
+ not_found ->
+ erlang:error({not_found});
Review comment:
Also a note that if you pick the exceptions that we shouldn't be raising `{not_found}` as a 1-tuple. It should be either `not_found` or `{not_found, missing}` to match the existing exceptions in other places.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400264507
##########
File path: src/fabric/src/fabric2_db.erl
##########
@@ -178,8 +185,7 @@ create(DbName, Options) ->
open(DbName, Options) ->
- UUID = fabric2_util:get_value(uuid, Options),
- case fabric2_server:fetch(DbName, UUID) of
Review comment:
You'll need to rebase this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r401797050
##########
File path: src/chttpd/test/eunit/chttpd_deleted_dbs_test.erl
##########
@@ -0,0 +1,206 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(chttpd_deleted_dbs_test).
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+
+-define(USER, "chttpd_db_test_admin").
+-define(PASS, "pass").
+-define(AUTH, {basic_auth, {?USER, ?PASS}}).
+-define(CONTENT_JSON, {"Content-Type", "application/json"}).
+
+
+setup() ->
+ Hashed = couch_passwords:hash_admin_password(?PASS),
+ ok = config:set("admins", ?USER, ?b2l(Hashed), _Persist=false),
+ Addr = config:get("chttpd", "bind_address", "127.0.0.1"),
+ Port = mochiweb_socket_server:get(chttpd, port),
+ lists:concat(["http://", Addr, ":", Port, "/"]).
+
+teardown(_Url) ->
+ ok = config:delete("couchdb", "enable_database_recovery", false),
+ ok = config:delete("admins", ?USER, _Persist=false).
+
+create_db(Url) ->
+ {ok, Status, _, _} = test_request:put(Url, [?CONTENT_JSON, ?AUTH], "{}"),
+ ?assert(Status =:= 201 orelse Status =:= 202).
+
+delete_db(Url) ->
+ {ok, 200, _, _} = test_request:delete(Url, [?AUTH]).
+
+deleted_dbs_test_() ->
+ {
+ "chttpd deleted dbs tests",
+ {
+ setup,
+ fun chttpd_test_util:start_couch/0, fun chttpd_test_util:stop_couch/1,
+ {
+ foreach,
+ fun setup/0, fun teardown/1,
+ [
+ fun should_return_error_for_deleted_dbs/1,
+ fun should_list_deleted_dbs/1,
+ fun should_list_deleted_dbs_info/1,
+ fun should_undelete_db/1,
+ fun should_remove_deleted_db/1,
+ fun should_undelete_db_to_target_db/1,
+ fun should_not_undelete_db_to_existing_db/1
+ ]
+ }
+ }
+ }.
+
+
+should_return_error_for_deleted_dbs(Url) ->
+ ?_test(begin
+ create_and_delete_db(Url),
+ NewDoc = "{\"keys\": [\"db1\"]}",
+ {ok, Code, _, ResultBody} = test_request:post(Url ++ "/_deleted_dbs/",
+ [?CONTENT_JSON, ?AUTH], NewDoc),
+
+ {Body} = jiffy:decode(ResultBody),
+ [
+ ?assertEqual(<<"bad_request">>,
+ couch_util:get_value(<<"error">>, Body)),
+ ?assertEqual(400, Code)
+ ]
+ end).
+
+
+should_list_deleted_dbs(Url) ->
+ ?_test(begin
+ DbName1 = create_and_delete_db(Url),
+ DbName2 = create_and_delete_db(Url),
+ {ok, _, _, ResultBody} = test_request:get(Url ++ "/_deleted_dbs/",
+ [?CONTENT_JSON, ?AUTH]),
+ BodyJson = jiffy:decode(ResultBody),
+ ?assertEqual(true, lists:member(DbName1, BodyJson)),
+ ?assertEqual(true, lists:member(DbName2, BodyJson))
Review comment:
I apparently haven't been reading close enough to realize that we're not returning deleted database info blobs like we did for the `_deleted_dbs/DbName` endpoint. I think we'd want to be doing that since a deleted database may have multiple entires in that list?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400287360
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -346,12 +351,97 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric2_util:do_recovery(),
Review comment:
I think splitting this up into three helpers would be the cleanest API. Something like this:
```erlang
delete(Db, Options) ->
DoRecovery = fabric2_util:do_recovery(),
DeletedAt = fabric2_util:get_value(deleted_at, Options),
case {DoRecovery, DeletedAt} of
{true, undefined} -> soft_delete_db(Db);
{false, undefined} -> hard_delete_db(Db);
{_, TimeStamp} -> remove_deleted_db(Db, Timestamp)
end.
soft_delete_db(Db) ->
#{
name := DbName,
tx := Tx,
layer_prefix := LayerPrefix
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix)
Timestamp = list_to_binary(fabric2_util:iso8601_timestamp()),
DeletedDbKeyTuple = {?DELETED_DBS, DbName, Timestamp},
DeletedDbKey = erlfdb_tuple:pack(DeletedDbKeyTuple, LayerPrefix),
case erlfdb:wait(erlfdb:get(Tx, DeletedDbKey)) of
not_found ->
erlfdb:set(Tx, DeletedDbKey, DbPrefix),
erlfdb:clear(Tx, DbKey);
_Val ->
erlang:error({deleted_database_exists, DbName})
end,
bump_metadata_version(Tx),
ok.
hard_delete_db(Db) ->
#{
name := DbName,
tx := Tx,
layer_prefix := LayerPrefix,
db_prefix := DbPrefix
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix)
erlfdb:clear(Tx, DbKey),
erlfdb:clear_range_startswith(Tx, DbPrefix),
bump_metadata_version(Tx),
ok.
remove_deleted_db(Db, Timestamp) ->
#{
name := DbName,
tx := Tx,
layer_prefix := LayerPrefix
} = ensure_current(Db0, false),
DeletedDbTupleKey = {?DELETED_DBS, DbName, TimeStamp},
DeletedDbKey = erlfdb_tuple:pack(DeletedDbTupleKey, LayerPrefix),
case erlfdb:wait(erlfdb:get(Tx, DeletedDbKey)) of
not_found ->
erlang:error({not_found, missing_timestamp});
DbPrefix ->
erlfdb:clear(Tx, DeleteDbKey),
erlfdb:clear_range_startswith(Tx, DbPrefix)
end,
ok.
```
Though the three helpers get moved down into the helper function section.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r396592286
##########
File path: src/fabric/src/fabric2_db.erl
##########
@@ -213,6 +219,26 @@ delete(DbName, Options) ->
end.
+deleted_dbs_info(DbName, Options) ->
+ Options1 = lists:keystore(user_ctx, 1, Options, ?ADMIN_CTX),
+ Result = fabric2_fdb:transactional(DbName, Options1, fun(TxDb) ->
+ fabric2_fdb:deleted_dbs_info(TxDb)
+ end),
+ {ok, lists:reverse(Result)}.
+
+
+restore(DbName, DesDbName, TimeStamp, Options) ->
Review comment:
Super picky but `DestDbName` or `TgtDbName`. Took me a while to realized that `DesDbName` isn't a typo.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on a change in pull request #2666:
[WIP] soft-deletion for database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r394976025
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -172,7 +174,8 @@ create(#{} = Db0, Options) ->
% Eventually DbPrefix will be HCA allocated. For now
% we're just using the DbName so that debugging is easier.
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- DbPrefix = erlfdb_tuple:pack({?DBS, DbName}, LayerPrefix),
+ #{db_prefix_allocator := DBPrefixAllocator} = erlfdb_directory:root(),
Review comment:
`DbPrefixAllocator` is defined in https://github.com/cloudant-labs/couchdb-erlfdb/pull/16/files#diff-cf5acaebd8465ae3276370bbf8d7aaf1R380-R383 with different `DbPrefix`. It will be different from `NodeIdAllocator`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400293152
##########
File path: src/chttpd/src/chttpd_misc.erl
##########
@@ -146,6 +147,143 @@ handle_all_dbs_req(#httpd{method='GET'}=Req) ->
handle_all_dbs_req(Req) ->
send_method_not_allowed(Req, "GET,HEAD").
+handle_deleted_dbs_req(#httpd{path_parts=[<<"_deleted_dbs">>]}=Req) ->
+ deleted_dbs_req(Req);
+handle_deleted_dbs_req(#httpd{path_parts=[<<"_deleted_dbs">>, DbName]}=Req) ->
+ deleted_dbs_info_req(Req, DbName);
+handle_deleted_dbs_req(Req) ->
+ chttpd:send_error(Req, not_found).
+
+deleted_dbs_req(#httpd{method='GET'}=Req) ->
+ #mrargs{
+ start_key = StartKey,
+ end_key = EndKey,
+ direction = Dir,
+ limit = Limit,
+ skip = Skip
+ } = couch_mrview_http:parse_params(Req, undefined),
+
+ Options = [
+ {start_key, StartKey},
+ {end_key, EndKey},
+ {dir, Dir},
+ {limit, Limit},
+ {skip, Skip}
+ ],
+
+ % Eventually the Etag for this request will be derived
+ % from the \xFFmetadataVersion key in fdb
+ Etag = <<"foo">>,
+
+ {ok, Resp} = chttpd:etag_respond(Req, Etag, fun() ->
+ {ok, Resp} = chttpd:start_delayed_json_response(Req, 200, [{"ETag",Etag}]),
+ Callback = fun all_dbs_callback/2,
+ Acc = #vacc{req=Req,resp=Resp},
+ fabric2_db:list_deleted_dbs(Callback, Acc, Options)
+ end),
+ case is_record(Resp, vacc) of
+ true -> {ok, Resp#vacc.resp};
+ _ -> {ok, Resp}
+ end;
+deleted_dbs_req(#httpd{method='POST'}=Req) ->
+ deleted_dbs_post_req(Req);
+deleted_dbs_req(Req) ->
+ send_method_not_allowed(Req, "GET,POST,HEAD").
+
+deleted_dbs_info_req(#httpd{user_ctx=Ctx}=Req, DbName) ->
+ couch_httpd:verify_is_server_admin(Req),
+ case fabric2_db:deleted_dbs_info(DbName, [{user_ctx, Ctx}]) of
+ {ok, Result} ->
+ {ok, Resp} = chttpd:start_json_response(Req, 200),
+ send_chunk(Resp, "["),
+ lists:foldl(fun({Timestamp, Info}, AccSeparator) ->
+ Json = ?JSON_ENCODE({[
+ {key, DbName},
+ {timestamp, Timestamp},
+ {value, {Info}}
+ ]}),
+ send_chunk(Resp, AccSeparator ++ Json),
+ "," % AccSeparator now has a comma
+ end, "", Result),
+ send_chunk(Resp, "]"),
+ chttpd:end_json_response(Resp);
+ Error ->
+ throw(Error)
+ end;
+deleted_dbs_info_req(Req, _DbName) ->
+ send_method_not_allowed(Req, "GET,HEAD").
+
+deleted_dbs_post_req(#httpd{user_ctx=Ctx}=Req) ->
+ couch_httpd:verify_is_server_admin(Req),
+ chttpd:validate_ctype(Req, "application/json"),
+ {JsonProps} = chttpd:json_body_obj(Req),
+ UndeleteJson0 = couch_util:get_value(<<"undelete">>, JsonProps, undefined),
+ DeleteJson0 = couch_util:get_value(<<"delete">>, JsonProps, undefined),
+ case {UndeleteJson0, DeleteJson0} of
+ {undefined, undefined} ->
+ throw({bad_request, <<"POST body must include `undeleted` "
+ "or `delete` parameter.">>});
+ {UndeleteJson, undefined} ->
+ handle_undelete_db_req(Req, UndeleteJson);
+ {undefined, DeleteJson} ->
+ handle_delete_deleted_req(Req, DeleteJson);
+ {_Else, _Else} ->
+ throw({bad_request,
+ <<"`undeleted` and `delete` are mutually exclusive">>})
+ end.
+
+handle_undelete_db_req(#httpd{user_ctx=Ctx}=Req, {JsonProps}) ->
+ DbName = case couch_util:get_value(<<"source">>, JsonProps) of
+ undefined ->
+ throw({bad_request,
+ <<"POST body must include `source` parameter.">>});
+ DbName0 ->
+ DbName0
+ end,
+ TimeStamp = case couch_util:get_value(<<"source_timestamp">>, JsonProps) of
+ undefined ->
+ throw({bad_request,
+ <<"POST body must include `source_timestamp` parameter.">>});
+ TimeStamp0 ->
+ TimeStamp0
+ end,
+ TgtDbName = case couch_util:get_value(<<"target">>, JsonProps) of
+ undefined -> DbName;
+ TgtDbName0 -> TgtDbName0
+ end,
+ case fabric2_db:undelete(DbName, TgtDbName, TimeStamp, [{user_ctx, Ctx}]) of
+ ok ->
+ send_json(Req, 200, {[{ok, true}]});
+ {error, file_exists} ->
+ chttpd:send_error(Req, file_exists);
+ Error ->
+ throw(Error)
+ end.
+
+handle_delete_deleted_req(#httpd{user_ctx=Ctx}=Req, {JsonProps}) ->
+ DbName = case couch_util:get_value(<<"source">>, JsonProps) of
+ undefined ->
+ throw({bad_request,
+ <<"POST body must include `source` parameter.">>});
+ DbName0 ->
+ DbName0
+ end,
+ TimeStamp = case couch_util:get_value(<<"source_timestamp">>, JsonProps) of
+ undefined ->
+ throw({bad_request,
+ <<"POST body must include `source_timestamp` parameter.">>});
+ TimeStamp0 ->
+ TimeStamp0
+ end,
+ case fabric2_db:delete_deleted(DbName, TimeStamp, [{user_ctx, Ctx}]) of
+ ok ->
+ send_json(Req, 200, {[{ok, true}]});
+ {error, file_exists} ->
+ chttpd:send_error(Req, file_exists);
Review comment:
I don't think file_exists is the right error here. I should be not_found, shouldn't it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666: [WIP]
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r396590266
##########
File path: src/fabric/include/fabric2.hrl
##########
@@ -17,10 +17,13 @@
% Prefix Definitions
+-define(DEFAULT_DB_PREFIX, <<16#FD>>).
Review comment:
This is a carry over from the directory layer that you don't need to use.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r400302599
##########
File path: src/chttpd/src/chttpd_misc.erl
##########
@@ -146,6 +147,143 @@ handle_all_dbs_req(#httpd{method='GET'}=Req) ->
handle_all_dbs_req(Req) ->
send_method_not_allowed(Req, "GET,HEAD").
+handle_deleted_dbs_req(#httpd{path_parts=[<<"_deleted_dbs">>]}=Req) ->
Review comment:
Also note you don't have to re-assert `_deleted_dbs` in the URL path as that's defined in the URL handlers bits.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on issue #2666: [WIP] soft-deletion
for database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on issue #2666: [WIP] soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-601693049
> Thinking about this some more I wonder if it would make sense to separate enabling HCA from soft-deletion. Those are rather independent features. So for example, we'd have a PR to enable HCA. We'd test that make sure that works. Then, as a separate piece of work, we'd enable soft-deletion. Would that be reasonable?
Yes, it is totally reasonable. We can have 2 PRs, i.e. one is for enabling HCA, and another is for soft-deletion. The only point I have is where to call ` DbPrefixAllocator = erlfdb_hca:create(?ERLFDB_EXTEND(DbId, <<"hca">>)),`. In https://github.com/cloudant-labs/couchdb-erlfdb/pull/16/files#diff-cf5acaebd8465ae3276370bbf8d7aaf1R383, it was supposed that this can be called once or limited times, but after moving to `fabric2_fdb.erl `, I stil try to find right place to call it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jiangphcn commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
jiangphcn commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-605742396
thanks again @davisp for your suggestion. It totally makes sense with
```
GET /_deleted_dbs
GET /_deleted_dbs/DbName
POST /_deleted_dbs with payload:
{
"undelete": {
"source": "source_database_name",
"source_timestamp": "2020-03-24T12:00:00",
"target": "target_database_name"
}
}
```
I will wait until today to see whether there is more comment from @mikerhodes and @ricellis. Otherwise, I will change with above naming convention.
Also, it is nice to have concept of `"actually delete the deleted thing"`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] mikerhodes commented on issue #2666: soft-deletion for
database
Posted by GitBox <gi...@apache.org>.
mikerhodes commented on issue #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#issuecomment-605893618
I am good with @davisp's suggestion in https://github.com/apache/couchdb/pull/2666#issuecomment-605109017. I've also thumbs-up'd it to doubly make the point 😄
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2666:
soft-deletion for database
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2666: soft-deletion for database
URL: https://github.com/apache/couchdb/pull/2666#discussion_r398820068
##########
File path: src/fabric/src/fabric2_fdb.erl
##########
@@ -346,12 +350,68 @@ delete(#{} = Db) ->
} = ensure_current(Db),
DbKey = erlfdb_tuple:pack({?ALL_DBS, DbName}, LayerPrefix),
- erlfdb:clear(Tx, DbKey),
- erlfdb:clear_range_startswith(Tx, DbPrefix),
+ DoRecovery = fabric2_util:do_recovery(),
+ case DoRecovery of
+ true ->
+ Timestamp = list_to_binary(fabric2_util:iso8601_timestamp()),
+ DeletedDbKey = erlfdb_tuple:pack({?DELETED_DBS, DbName, Timestamp},
+ LayerPrefix),
+ erlfdb:set(Tx, DeletedDbKey, DbPrefix),
Review comment:
It just occurred to me, we need to read the `DeletedDbKey` and make sure that it comes back `not_found` or else we end up losing references to deleted databases if users create/delete databases too quickly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services