jaydoane commented on code in PR #5168:
URL: https://github.com/apache/couchdb/pull/5168#discussion_r1703072856
##########
src/couch/src/couch_lru.erl:
##########
@@ -62,5 +71,166 @@ close_int({Lru, DbName, Iter}, {Tree, Dict} = Cache) ->
false ->
NewTree = gb_trees:delete(Lru, Tree),
NewIter = gb_trees:iterator(NewTree),
- close_int(gb_trees:next(NewIter), {NewTree, dict:erase(DbName,
Dict)})
+ close_int(gb_trees:next(NewIter), {NewTree, maps:remove(DbName,
Map)})
end.
+
+-ifdef(TEST).
+
+-include_lib("couch/include/couch_eunit.hrl").
+
+-define(DB1, <<"db1">>).
+-define(DB2, <<"db2">>).
+
+couch_lru_test_() ->
+ {
+ foreach,
+ fun setup/0,
+ fun teardown/1,
+ [
+ ?TDEF_FE(t_new),
+ ?TDEF_FE(t_insert),
+ ?TDEF_FE(t_insert_duplicate),
+ ?TDEF_FE(t_update),
+ ?TDEF_FE(t_close_empty),
+ ?TDEF_FE(t_close_unlocked_idle),
+ ?TDEF_FE(t_close_bump_busy_all),
+ ?TDEF_FE(t_close_bump_busy_one),
+ ?TDEF_FE(t_close_entry_one_is_missing)
+ ]
+ }.
+
+t_new(_) ->
+ Cache = new(),
+ ?assertMatch({_, _}, Cache),
+ {Tree, Map} = Cache,
+ ?assert(gb_trees:is_empty(Tree)),
+ ?assert(is_map(Map) andalso map_size(Map) == 0).
+
+t_insert(_) ->
+ {Tree, Map} = insert(?DB1, new()),
+ ?assertEqual(1, gb_trees:size(Tree)),
+ ?assertEqual(1, map_size(Map)),
+ ?assertMatch(#{?DB1 := _}, Map),
+ #{?DB1 := Int} = Map,
+ ?assert(is_integer(Int)),
+ ?assert(Int > 0),
+ ?assertEqual([{Int, ?DB1}], gb_trees:to_list(Tree)).
+
+t_insert_duplicate(_) ->
+ % We technically allow this, but is this right? Should we always use update
+ % instead which would reap the old LRU entry
Review Comment:
Good question! It seems like
[this](https://github.com/apache/couchdb/blob/09cb5213892f566827cd36e425f4cead65065166/src/couch/src/couch_server.erl#L563)
is the only place `couch_lru:insert/2` is used, but it doesn't appear to
prevent duplicates there either.
##########
src/couch/src/couch_lru.erl:
##########
@@ -15,34 +15,43 @@
-include("couch_server_int.hrl").
+-type cache() :: {any(), #{binary() => pos_integer()}}.
Review Comment:
Could this `any()` be tightened up to be a `gb_trees:tree()`?
##########
src/couch/src/couch_lru.erl:
##########
@@ -62,5 +71,166 @@ close_int({Lru, DbName, Iter}, {Tree, Dict} = Cache) ->
false ->
NewTree = gb_trees:delete(Lru, Tree),
NewIter = gb_trees:iterator(NewTree),
- close_int(gb_trees:next(NewIter), {NewTree, dict:erase(DbName,
Dict)})
+ close_int(gb_trees:next(NewIter), {NewTree, maps:remove(DbName,
Map)})
end.
+
+-ifdef(TEST).
+
+-include_lib("couch/include/couch_eunit.hrl").
+
+-define(DB1, <<"db1">>).
+-define(DB2, <<"db2">>).
+
+couch_lru_test_() ->
+ {
+ foreach,
+ fun setup/0,
+ fun teardown/1,
+ [
+ ?TDEF_FE(t_new),
+ ?TDEF_FE(t_insert),
+ ?TDEF_FE(t_insert_duplicate),
+ ?TDEF_FE(t_update),
+ ?TDEF_FE(t_close_empty),
+ ?TDEF_FE(t_close_unlocked_idle),
+ ?TDEF_FE(t_close_bump_busy_all),
+ ?TDEF_FE(t_close_bump_busy_one),
+ ?TDEF_FE(t_close_entry_one_is_missing)
+ ]
+ }.
+
+t_new(_) ->
+ Cache = new(),
+ ?assertMatch({_, _}, Cache),
+ {Tree, Map} = Cache,
+ ?assert(gb_trees:is_empty(Tree)),
+ ?assert(is_map(Map) andalso map_size(Map) == 0).
+
+t_insert(_) ->
+ {Tree, Map} = insert(?DB1, new()),
+ ?assertEqual(1, gb_trees:size(Tree)),
+ ?assertEqual(1, map_size(Map)),
+ ?assertMatch(#{?DB1 := _}, Map),
+ #{?DB1 := Int} = Map,
+ ?assert(is_integer(Int)),
+ ?assert(Int > 0),
+ ?assertEqual([{Int, ?DB1}], gb_trees:to_list(Tree)).
+
+t_insert_duplicate(_) ->
+ % We technically allow this, but is this right? Should we always use update
+ % instead which would reap the old LRU entry
+ %
+ {Tree, Map} = insert(?DB1, insert(?DB1, new())),
+ ?assertEqual(2, gb_trees:size(Tree)),
+ ?assertEqual(1, map_size(Map)),
+ ?assertMatch(#{?DB1 := _}, Map),
+ ?assertMatch([{_, ?DB1}, {_, ?DB1}], gb_trees:to_list(Tree)).
+
+t_update(_) ->
+ % Insert followed by update.
+ {Tree, Map} = update(?DB1, insert(?DB1, new())),
+ ?assertEqual(1, gb_trees:size(Tree)),
+ ?assertEqual(1, map_size(Map)),
+ ?assertMatch(#{?DB1 := _}, Map),
+ #{?DB1 := Int} = Map,
+ ?assertEqual([{Int, ?DB1}], gb_trees:to_list(Tree)).
+
+t_close_empty(_) ->
+ ?assertEqual(false, close(new())).
+
+t_close_unlocked_idle({Dbs, DbsPids, [Pid1, _]}) ->
+ % There is one db handle and it's idle. It should get closed.
+ Cache = insert(?DB1, new()),
+ Res = close(Cache),
+ ?assertMatch({true, {_, #{}}}, Res),
+ {true, {Tree, Map}} = Res,
+ ?assert(gb_trees:is_empty(Tree)),
+ ?assert(is_map(Map) andalso map_size(Map) == 0),
+ ?assertNot(is_process_alive(Pid1)),
+ ?assertEqual([], ets:lookup(Dbs, ?DB1)),
+ ?assertEqual([], ets:lookup(DbsPids, Pid1)).
+
+t_close_bump_busy_all({_Dbs, _DbsPids, [Pid1, _]}) ->
+ % There is one db handle and it's busy. We should stay up.
+ Cache = insert(?DB1, new()),
+ meck:expect(couch_db, is_idle, 1, false),
+ % Yeah, it is odd that we're throwing away the updated cache by returning
+ % just false if we don't end up finding an idle Db.
Review Comment:
Great point! The strange spec for `close` had me wondering about the curious
asymmetry.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
