Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Enchance/mw/bg manager #538

Closed
wants to merge 5 commits into from

3 participants

@lordnull
Collaborator

Add tests for coverage of exported api, and removed unused/old code in the process.

lordnull added some commits
@lordnull lordnull Removed unneeded macro.
The table is not given to the bg_manager. The supervising process
creates the tables named and public. The bg_manager always refers to
the tables by name, so there's waiting for a transfer.
3c51eab
@lordnull lordnull Added defaults to state record. e50efe1
@lordnull lordnull Prefer unlinking in test rather than test only start.
The more similar the code used in test is to production, the better.
If the test needs to survive killing the manager, unlink can be used.
3a45117
@lordnull lordnull Removed reference to start/0 function 04a86bb
@lordnull lordnull Tests and changes to boost coverage and correct specs.
Most tests are there to ensure the external api remains consistent and
cover the interesting cases. This has boosted coverage from 60's to 90's.
This also caught incorrect specs for some return values.
4e5a3b4
@cmeiklejohn
Collaborator

Is this targeted for 2.0 or 2.1?

@jrwest jrwest commented on the diff
src/riak_core_bg_manager.erl
((11 lines not shown))
%% @doc Disable handing out resource of the given kind.
-spec disable(bg_resource()) -> disabled | unregistered | bypassed.
disable(Resource) ->
- gen_server:call(?SERVER, {disable, Resource}).
+ case gen_server:call(?SERVER, {disable, Resource}) of
+ {unregistered, Resource} ->
+ unregistered;
@jrwest
jrwest added a note

what's the benefit of changing the API here. I understand the information is redundant but is there a reason? Also, this code path seems repeated in many places.

@lordnull Collaborator

The spec did not match the actual return value. Looking at where this was used, it seemed the better option was to change the code so it matched the spec rather than the other way around. This was discovered while writing the unit test for the function.

@jrwest
jrwest added a note

hrm, neither dialyzer [1] nor the eqc test caught this. one should...once again, i'd like to move away from the unit tests (and see them removed once any non-redundant parts are part of the eqc test). the unit tests caused several problems along the way during review.

[1] https://github.com/basho/riak_core/pull/528/files#diff-9

@lordnull Collaborator

Double checking, this breaks the eqc, so fixing the spec would been the right way to go.

@jrwest
jrwest added a note

ok wfm. i'm guessing btw that dialyzer can't figure it out bc of the gen_server message boundary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jrwest jrwest commented on the diff
src/riak_core_bg_manager.erl
@@ -141,11 +137,6 @@ start_link() ->
_ = maybe_create_ets(),
gen_server:start_link({local, ?SERVER}, ?MODULE, [], []).
-%% Test entry point to start stand-alone server
-start() ->
@jrwest
jrwest added a note

this is useful for local testing

@lordnull Collaborator

As is using the function that will be used in production and unlinking to it. The fewer differences between production and testing, the better. If this is useful in testing, and intended only for testing, it should be wrapped in an -ifdef.

@jrwest
jrwest added a note

the ifdef is more correct but means you have to do testing using beams from the .eunit directory. thats fine, just something to remember. fwiw, this is a pattern we use in several places -- but again I think wrapping it in the ifdef makes sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jrwest jrwest commented on the diff
src/riak_core_bg_manager.erl
@@ -421,11 +440,7 @@ query_resource(Resource, Types) ->
{stop, term()}.
init([]) ->
lager:debug("Background Manager starting up."),
- State = #state{info_table=?BG_INFO_ETS_TABLE,
@jrwest
jrwest added a note

i actually find this more confusing. having the defaults in init makes it more clear imo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jrwest jrwest commented on the diff
src/riak_core_bg_manager.erl
@@ -758,8 +760,6 @@ update_resource_info(Resource, Fun, Default, State=#state{info_table=TableId}) -
State.
%% @doc Throws unregistered for unknown Resource
-resource_info(_Resource, State) when ?NOT_TRANSFERED(State) ->
@jrwest
jrwest added a note

ah good call. all the ?NOT_TRANSFERED stuff is dead code since removing the table manager

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jrwest jrwest commented on the diff
test/bg_manager_tests.erl
((21 lines not shown))
end},
- {"bypass API",
- fun() ->
- %% bypass API
- riak_core_bg_manager:bypass(true),
+ {"lock api", [
@jrwest
jrwest added a note

i dont think we should be adding to these tests. I'm not even sure why they are still here. Can this be extended to the eqc test?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lordnull
Collaborator

This has been punted upon until at least 2.1.

@jrwest

related PR: #535

@jrwest

For now marking as milestone 2.0.1.

@lordnull can this be closed? some of these changes overlapped with other PRs and others, and iirc broke tests or were not necessary.

@jrwest jrwest added this to the 2.0.1 milestone
@lordnull
Collaborator

This can be closed mainly since it ended up duplicating some effort.

@lordnull lordnull closed this
@seancribbs seancribbs deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 28, 2014
  1. @lordnull

    Removed unneeded macro.

    lordnull authored
    The table is not given to the bg_manager. The supervising process
    creates the tables named and public. The bg_manager always refers to
    the tables by name, so there's waiting for a transfer.
  2. @lordnull
  3. @lordnull

    Prefer unlinking in test rather than test only start.

    lordnull authored
    The more similar the code used in test is to production, the better.
    If the test needs to survive killing the manager, unlink can be used.
  4. @lordnull
  5. @lordnull

    Tests and changes to boost coverage and correct specs.

    lordnull authored
    Most tests are there to ensure the external api remains consistent and
    cover the interesting cases. This has boosted coverage from 60's to 90's.
    This also caught incorrect specs for some return values.
This page is out of date. Refresh to see the latest.
View
84 src/riak_core_bg_manager.erl
@@ -103,9 +103,7 @@
get_token/2,
get_token/3,
token_info/0,
- token_info/1,
- %% Testing
- start/0
+ token_info/1
]).
%% reporting
@@ -129,8 +127,6 @@
-define(SERVER, ?MODULE).
--define(NOT_TRANSFERED(S), S#state.info_table == undefined orelse S#state.entry_table == undefined).
-
%%%===================================================================
%%% API
%%%===================================================================
@@ -141,11 +137,6 @@ start_link() ->
_ = maybe_create_ets(),
gen_server:start_link({local, ?SERVER}, ?MODULE, [], []).
-%% Test entry point to start stand-alone server
-start() ->
@jrwest
jrwest added a note

this is useful for local testing

@lordnull Collaborator

As is using the function that will be used in production and unlinking to it. The fewer differences between production and testing, the better. If this is useful in testing, and intended only for testing, it should be wrapped in an -ifdef.

@jrwest
jrwest added a note

the ifdef is more correct but means you have to do testing using beams from the .eunit directory. thats fine, just something to remember. fwiw, this is a pattern we use in several places -- but again I think wrapping it in the ifdef makes sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- _ = maybe_create_ets(),
- gen_server:start({local, ?SERVER}, ?MODULE, [], []).
-
%% @doc Global kill switch - causes all locks/tokens to be given out freely without limits.
%% Nothing will be tracked or recorded.
-spec bypass(boolean()) -> ok.
@@ -176,22 +167,42 @@ enabled() ->
%% has not already been registered, this will have no effect.
-spec enable(bg_resource()) -> enabled | unregistered | bypassed.
enable(Resource) ->
- gen_server:call(?SERVER, {enable, Resource}).
+ case gen_server:call(?SERVER, {enable, Resource}) of
+ {unregistered, Resource} ->
+ unregistered;
+ Else ->
+ Else
+ end.
%% @doc Disable handing out resource of the given kind.
-spec disable(bg_resource()) -> disabled | unregistered | bypassed.
disable(Resource) ->
- gen_server:call(?SERVER, {disable, Resource}).
+ case gen_server:call(?SERVER, {disable, Resource}) of
+ {unregistered, Resource} ->
+ unregistered;
@jrwest
jrwest added a note

what's the benefit of changing the API here. I understand the information is redundant but is there a reason? Also, this code path seems repeated in many places.

@lordnull Collaborator

The spec did not match the actual return value. Looking at where this was used, it seemed the better option was to change the code so it matched the spec rather than the other way around. This was discovered while writing the unit test for the function.

@jrwest
jrwest added a note

hrm, neither dialyzer [1] nor the eqc test caught this. one should...once again, i'd like to move away from the unit tests (and see them removed once any non-redundant parts are part of the eqc test). the unit tests caused several problems along the way during review.

[1] https://github.com/basho/riak_core/pull/528/files#diff-9

@lordnull Collaborator

Double checking, this breaks the eqc, so fixing the spec would been the right way to go.

@jrwest
jrwest added a note

ok wfm. i'm guessing btw that dialyzer can't figure it out bc of the gen_server message boundary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ Else ->
+ Else
+ end.
-spec enabled(bg_resource()) -> enabled | disabled | bypassed.
enabled(Resource) ->
- gen_server:call(?SERVER, {enabled, Resource}).
+ case gen_server:call(?SERVER, {enabled, Resource}) of
+ {unregistered, Resource} ->
+ unregistered;
+ Else ->
+ Else
+ end.
%% @doc Disable handing out resource of the given kind. If kill == true,
%% processes that currently hold the given resource will be killed.
-spec disable(bg_resource(), boolean()) -> disabled | unregistered | bypassed.
disable(Resource, Kill) ->
- gen_server:call(?SERVER, {disable, Resource, Kill}).
+ case gen_server:call(?SERVER, {disable, Resource, Kill}) of
+ {unregistered, Resource} ->
+ unregistered;
+ Else ->
+ Else
+ end.
%%%%%%%%%%%
%% Lock API
@@ -201,7 +212,12 @@ disable(Resource, Kill) ->
%% If the background manager is unavailable, undefined is returned.
-spec concurrency_limit(bg_lock()) -> bg_concurrency_limit() | undefined.
concurrency_limit(Lock) ->
- gen_server:call(?MODULE, {concurrency_limit, Lock}, infinity).
+ case gen_server:call(?MODULE, {concurrency_limit, Lock}, infinity) of
+ {unregistered, Lock} ->
+ unregistered;
+ Else ->
+ Else
+ end.
%% @doc same as `set_concurrency_limit(Type, Limit, false)'
-spec set_concurrency_limit(bg_lock(), bg_concurrency_limit()) ->
@@ -403,10 +419,13 @@ query_resource(Resource, Types) ->
%%%
-record(state,
- {info_table:: ets:tid(), %% TableID of ?BG_INFO_ETS_TABLE
- entry_table:: ets:tid(), %% TableID of ?BG_ENTRY_ETS_TABLE
- enabled :: boolean(), %% Global enable/disable switch, true at startup
- bypassed:: boolean() %% Global kill switch. false at startup
+ {info_table = ?BG_INFO_ETS_TABLE :: ets:tid(),
+ entry_table = ?BG_ENTRY_ETS_TABLE :: ets:tid(),
+ enabled = true :: boolean(),
+ % if false, all tokens/locks are considered at max concurrency.
+ bypassed = false :: boolean()
+ % if true, no tracking is done and locks/tokens given away as
+ % as requested.
}).
%%%===================================================================
@@ -421,11 +440,7 @@ query_resource(Resource, Types) ->
{stop, term()}.
init([]) ->
lager:debug("Background Manager starting up."),
- State = #state{info_table=?BG_INFO_ETS_TABLE,
@jrwest
jrwest added a note

i actually find this more confusing. having the defaults in init makes it more clear imo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- entry_table=?BG_ENTRY_ETS_TABLE,
- enabled=true,
- bypassed=false},
- State2 = validate_holds(State),
+ State2 = validate_holds(#state{}),
State3 = restore_enabled(true, State2),
State4 = restore_bypassed(false, State3),
reschedule_token_refills(State4),
@@ -564,15 +579,11 @@ validate_hold(_Obj, _TableId) -> %% tokens don't monitor processes
ok.
%% @doc Update state with bypassed status and store to ETS
-update_bypassed(_Bypassed, State) when ?NOT_TRANSFERED(State) ->
- State;
update_bypassed(Bypassed, State=#state{info_table=TableId}) ->
ets:insert(TableId, {bypassed, Bypassed}),
State#state{bypassed=Bypassed}.
%% @doc Update state with enabled status and store to ETS
-update_enabled(_Enabled, State) when ?NOT_TRANSFERED(State) ->
- State;
update_enabled(Enabled, State=#state{info_table=TableId}) ->
ets:insert(TableId, {enabled, Enabled}),
State#state{enabled=Enabled}.
@@ -608,8 +619,6 @@ do_handle_call_exception(Function, Args, State) ->
end.
%% @doc Throws {unregistered, Resource} for unknown Lock.
-do_disable_lock(_Lock, _Kill, State) when ?NOT_TRANSFERED(State) ->
- {noreply, State};
do_disable_lock(Lock, Kill, State) ->
Info = resource_info(Lock, State),
enforce_type_or_throw(Lock, lock, Info),
@@ -637,9 +646,6 @@ do_set_token_rate(Token, Rate, State) ->
{reply, Error, State}
end.
-do_get_type_info(_Type, State) when ?NOT_TRANSFERED(State) ->
- %% Table not trasnferred yet.
- [];
do_get_type_info(Type, State) ->
S = fun({R,_T,E,L}) -> {R,E,L} end,
Resources = all_registered_resources(Type, State),
@@ -647,8 +653,6 @@ do_get_type_info(Type, State) ->
{reply, Infos, State}.
%% Returns empty if the ETS table has not been transferred to us yet.
-do_resource_limit(_Type, _Resource, State) when ?NOT_TRANSFERED(State) ->
- {reply, undefined, state};
do_resource_limit(_Type, Resource, State) ->
Info = resource_info(Resource, State),
Rate = ?resource_limit(Info),
@@ -700,8 +704,6 @@ limit(#resource_info{type=token, limit={_Period,MaxCount}}) -> MaxCount.
%% @private
%% @doc Release the resource associated with the given reference. This is mostly
%% meaningful for locks.
-release_resource(_Ref, State) when ?NOT_TRANSFERED(State) ->
- State;
release_resource(Ref, State=#state{entry_table=TableId}) ->
%% There should only be one instance of the object, but we'll zap all that match.
Given = [Obj || Obj <- ets:match_object(TableId, {{given, '_'},'_'})],
@@ -758,8 +760,6 @@ update_resource_info(Resource, Fun, Default, State=#state{info_table=TableId}) -
State.
%% @doc Throws unregistered for unknown Resource
-resource_info(_Resource, State) when ?NOT_TRANSFERED(State) ->
@jrwest
jrwest added a note

ah good call. all the ?NOT_TRANSFERED stuff is dead code since removing the table manager

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- throw(table_id_undefined);
resource_info(Resource, #state{info_table=TableId}) ->
Key = {info,Resource},
case ets:lookup(TableId, Key) of
@@ -783,8 +783,6 @@ reschedule_token_refills(State) ->
[schedule_refill_tokens(Token, State) || Token <- Tokens].
%% Schedule a timer event to refill tokens of given type
-schedule_refill_tokens(_Token, State) when ?NOT_TRANSFERED(State) ->
- ok;
schedule_refill_tokens(Token, State) ->
case ?resource_limit(resource_info(Token, State)) of
undefined ->
@@ -842,10 +840,6 @@ try_get_resource(true, Resource, Type, Pid, Meta, State) ->
{reply, max_concurrency, #state{}}
| {reply, {ok, #state{}}}
| {reply, {{ok, reference()}, #state{}}}.
-do_get_resource(_Resource, _Type, _Pid, _Meta, State) when ?NOT_TRANSFERED(State) ->
- %% Table transfer has not occurred yet. Reply "max_concurrency" so that callers
- %% will try back later, hopefully when we have our table back.
- {reply, max_concurrency, State};
%% @doc When the API is bypassed, we ignore concurrency limits.
do_get_resource(Resource, Type, Pid, Meta, State=#state{bypassed=true}) ->
{Result, State2} = try_get_resource(true, Resource, Type, Pid, Meta, State),
View
11 test/bg_manager_eqc.erl
@@ -54,6 +54,10 @@
tokens :: [{bg_eqc_type(), non_neg_integer()}]
}).
+% for eunit happiness
+run_eqc_test() ->
+ {timeout, 10000, ?_assert(run_eqc())}.
+
run_eqc() ->
run_eqc(100).
@@ -462,7 +466,9 @@ revive_next(S, _Value, _Args) ->
%% @doc revive command
revive() ->
- {ok, _BgMgr} = riak_core_bg_manager:start().
+ {ok, BgMgr} = Out = riak_core_bg_manager:start_link(),
+ unlink(BgMgr),
+ Out.
%% @doc revive_post - Postcondition for revive
revive_post(_S, _Args, _Res) ->
@@ -803,7 +809,8 @@ prop_bgmgr() ->
?TRAPEXIT(
begin
stop_pid(whereis(riak_core_bg_manager)),
- {ok, _BgMgr} = riak_core_bg_manager:start(),
+ {ok, BgMgr} = riak_core_bg_manager:start_link(),
+ unlink(BgMgr),
{H, S, Res} = run_commands(?MODULE,Cmds),
InfoTable = ets:tab2list(?BG_INFO_ETS_TABLE),
EntryTable = ets:tab2list(?BG_ENTRY_ETS_TABLE),
View
206 test/bg_manager_tests.erl
@@ -65,35 +65,199 @@ bg_mgr_test_() ->
Period = 5000000,
?BG_MGR:set_token_rate(T, {Period, Max}),
?assertEqual(ok, ?BG_MGR:get_token(T)),
+ Locker = spawn(fun() -> receive _ -> ok end end),
+ Locker2 = spawn(fun() -> receive _ -> ok end end),
+ ?assertMatch({ok, _}, ?BG_MGR:get_lock(lock_a, Locker)),
+ ?assertMatch({ok, _}, ?BG_MGR:get_lock(lock_a, Locker2)),
?assertEqual(1, length(?BG_MGR:all_tokens(T))),
- crash_and_restart_token_manager(),
+ kill_bg_mgr(),
+ Locker2 ! ok,
+ timer:sleep(100),
+ start_bg_mgr(),
+ %crash_and_restart_token_manager(),
?assertEqual(ok, ?BG_MGR:get_token(T)),
?assertEqual(2, length(?BG_MGR:all_tokens(T))),
- ?assertEqual(max_concurrency, ?BG_MGR:get_token(T))
+ ?assertEqual(max_concurrency, ?BG_MGR:get_token(T)),
+ ?assertEqual(1, length(?BG_MGR:all_locks(lock_a))),
+ Locker ! ok
end},
- {"bypass API",
- fun() ->
- %% bypass API
- riak_core_bg_manager:bypass(true),
+ {"lock api", [
@jrwest
jrwest added a note

i dont think we should be adding to these tests. I'm not even sure why they are still here. Can this be extended to the eqc test?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ {"get limit for unregistered lock", ?_assertEqual(unregistered, riak_core_bg_manager:concurrency_limit(noexists))},
+
+ {"set concurrency limit for new lock", ?_assertEqual(undefined, riak_core_bg_manager:set_concurrency_limit(lock_api, 2))},
+
+ {"get concurrency limit", ?_assertEqual(2, riak_core_bg_manager:concurrency_limit(lock_api))},
+
+ {"concurrency limit reached", ?_assertNot(riak_core_bg_manager:concurrency_limit_reached(lock_api))},
+
+ {"get lock with options", spawn, ?_assertMatch({ok, _}, riak_core_bg_manager:get_lock(lock_api, []))},
+
+ {"current count", ?_assertEqual(0, riak_core_bg_manager:lock_count(lock_api))},
+
+ {"lock info", fun() ->
+ Expected = lists:sort([
+ {lock_a, true, 52},
+ {lock_api, true, 2}
+ ]),
+ Info = lists:sort(riak_core_bg_manager:lock_info()),
+ ?assertEqual(Expected, Info)
+ end},
+
+ {"lock info for a specific lock", fun() ->
+ Expected = {true, 2},
+ ?assertEqual(Expected, riak_core_bg_manager:lock_info(lock_api))
+ end}
+
+ ]},
+
+ {"token api", [
+
+ {"set token rate", ?_assertEqual(undefined, riak_core_bg_manager:set_token_rate(token_api, {1000, 2}))},
+
+ {"get token sans pid", ?_assertEqual(ok, riak_core_bg_manager:get_token(token_api, []))},
+
+ {"all token info", fun() ->
+ % sleep to let tokens refill
+ UnsortedGot = riak_core_bg_manager:token_info(),
+ Got = lists:sort(UnsortedGot),
+ Expected = lists:sort(
+ [{TName, true, Rate} || {TName, Rate} <- some_token_rates()] ++ [
+ {token_api, true, {1000, 2}},
+ {token_a, true, {1,5}},
+ {token_b, true, {5000000,2}}
+ ]),
+ ?assertEqual(Expected, Got)
+ end},
+
+ {"a specific token's info", ?_assertEqual({true, {1000, 2}}, riak_core_bg_manager:token_info(token_api))}
+
+ ]},
+
+ {"reporting api", [
+
+ {"all resources", fun() ->
+ Self = self(),
+ Rec = {bg_stat_live, token_b, token, {Self, []}},
+ Expected = [Rec, Rec],
+ % wait for most tokens
+ timer:sleep(1100),
+ Got = riak_core_bg_manager:all_resources(),
+ ?assertEqual(Expected, Got)
+ end},
- %% reduce token rate to zero and ensure we still get one
- riak_core_bg_manager:set_token_rate(token_a, {1,0}),
- ok = riak_core_bg_manager:get_token(token_a),
+ {"all resources of a given name", ?_assertEqual([], riak_core_bg_manager:all_resources(noexists))},
- %% reduce lock limit to zero and ensure we still get one
+ {"all locks", fun() ->
+ Self = self(),
+ Pids = lists:map(fun(_) ->
+ spawn(fun() ->
+ _ = riak_core_bg_manager:get_lock(lock_api),
+ Self ! continue,
+ receive _ -> ok end
+ end)
+ end, lists:seq(1,2)),
+ Expected = lists:map(fun(P) ->
+ {bg_stat_live, lock_api, lock, {P, []}}
+ end, Pids),
+ lists:foreach(fun(_) -> receive continue -> ok end end, Pids),
+ Got = riak_core_bg_manager:all_locks(),
+ ?assertEqual(Expected, Got)
+ end},
+
+ {"locks of a given name", ?_assertEqual([], riak_core_bg_manager:all_locks(noexists))},
+
+ {"all tokens", fun() ->
+ Self = self(),
+ Rec = {bg_stat_live, token_b, token, {Self, []}},
+ Expected = [Rec, Rec],
+ Got = riak_core_bg_manager:all_tokens(),
+ ?assertEqual(Expected, Got)
+ end}
+
+ ]},
+
+ {"enable/disable API", [
+ {"get current global enabled", ?_assertEqual(enabled, riak_core_bg_manager:enabled())},
+
+ {"global diable returns disabled", ?_assertEqual(disabled, riak_core_bg_manager:disable())},
+
+ {"specific resource during global disable returns disabled", ?_assertEqual(disabled, riak_core_bg_manager:enabled(token1))},
+
+ {"getting a token indicates max concurrency", ?_assertEqual(max_concurrency, riak_core_bg_manager:get_token(token1))},
+
+ {"global enable returns enabled", ?_assertEqual(enabled, riak_core_bg_manager:enable())},
+
+ {"global enable query", ?_assertEqual(enabled, riak_core_bg_manager:enabled())},
+
+ {"no explosion diabling an unregistered token", ?_assertEqual(unregistered, riak_core_bg_manager:disable(noexists))},
+
+ {"no explosion enabling an unregistered token", ?_assertEqual(unregistered, riak_core_bg_manager:enable(noexists))},
+
+ {"no explosion checking status of unregistered token", ?_assertEqual(unregistered, riak_core_bg_manager:enabled(noexists))},
+
+ {"no explosion disabling an unregistred lock with kill", ?_assertEqual(unregistered, riak_core_bg_manager:disable(noexists, true))},
+
+ {"disable a specific token", ?_assertEqual(disabled, riak_core_bg_manager:disable(token1))},
+
+ {"reports correct status for disabled token", ?_assertEqual(disabled, riak_core_bg_manager:enabled(token1))},
+
+ {"enable a specific token", ?_assertEqual(enabled, riak_core_bg_manager:enable(token1))},
+
+ {"disable with true kill exits a locking process", fun() ->
+ Self = self(),
+ {Locker, Mon} = spawn_monitor(fun() ->
+ {ok, _} = riak_core_bg_manager:get_lock(lock_a),
+ Self ! continue,
+ receive _ -> ok end
+ end),
+ receive continue -> ok end,
+ disabled = riak_core_bg_manager:disable(lock_a, true),
+ Got = receive
+ {'DOWN', Mon, process, Locker, max_concurrency} ->
+ true
+ after 3000 ->
+ false
+ end,
+ ?assert(Got)
+ end}
+
+ ]},
+
+ {"bypass API", [
+ {"set bypass to true", fun() ->
+ ok = riak_core_bg_manager:bypass(true),
+ ?assert(riak_core_bg_manager:bypassed())
+ end},
+
+ {"enabled returns bypassed", ?_assertEqual(bypassed, riak_core_bg_manager:enabled())},
+
+ {"reduce token rate to zero and still get a token", fun() ->
+ riak_core_bg_manager:set_token_rate(token_a, {1,0}),
+ ?assertEqual(ok, riak_core_bg_manager:get_token(token_a))
+ end},
+
+ {"reduce lock limiit to zero and still get one", fun() ->
riak_core_bg_manager:set_concurrency_limit(lock_a, 0),
- {ok, _Ref1} = riak_core_bg_manager:get_lock(lock_a),
+ ?assertMatch({ok, _Ref1}, riak_core_bg_manager:get_lock(lock_a))
+ end},
+
+ {"bypass over-rides resource specific disable", fun() ->
+ ?assertEqual(bypassed, riak_core_bg_manager:disable(lock_a))
+ end},
- %% even if globally disabled, we get a lock
- riak_core_bg_manager:disable(),
- {ok, _Ref2} = riak_core_bg_manager:get_lock(lock_a),
+ {"bypass over-rides global disable", fun() ->
+ ?assertEqual(bypassed, riak_core_bg_manager:disable()),
+ ?assertMatch({ok, _Ref2}, riak_core_bg_manager:get_lock(lock_a))
+ end},
- %% turn it back on
- ?BG_MGR:bypass(false),
- ?BG_MGR:enable(),
- ?assertEqual(max_concurrency, ?BG_MGR:get_lock(lock_a))
- end}
+ {"max_concurrenty once bypass set to false", fun() ->
+ ?BG_MGR:bypass(false),
+ ?BG_MGR:enable(),
+ ?assertEqual(max_concurrency, ?BG_MGR:get_lock(lock_a))
+ end}
+ ]}
] end}
}.
@@ -146,12 +310,14 @@ verify_token_rates() ->
%% doesn't take down our test too.
start_bg_mgr() ->
%% setup with history window to 1 seconds
- ?BG_MGR:start(),
+ {ok, Pid} = ?BG_MGR:start_link(),
+ unlink(Pid),
timer:sleep(100).
kill_bg_mgr() ->
Pid = erlang:whereis(?BG_MGR),
?assertNot(Pid == undefined),
+ unlink(Pid),
erlang:exit(Pid, kill).
crash_and_restart_token_manager() ->
Something went wrong with that request. Please try again.