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

Already on GitHub? Sign in to your account

Changes needed to expose FSM timeouts to clients #521

Merged
merged 3 commits into from Apr 3, 2013

Conversation

Projects
None yet
2 participants
Contributor

evanmcc commented Mar 26, 2013

  • change delete in riak_client to honor timeouts passed
    in Options
  • change the various object interfaces to collect and
    pass on the timeout values

requires basho/riak_pb#38

@evanmcc evanmcc Changes needed to expose FSM timeouts to the clients:
  - change delete in riak_client to honor timeouts passed
    in Options
  - change the various object interfaces to collect and
    pass on the timeout values
9ef783e

@evanmcc evanmcc referenced this pull request in basho/riak_test Mar 26, 2013

Merged

Rework and rename verify_mw_timeouts test. #243

Contributor

evanmcc commented Mar 26, 2013

associated riak test: basho/riak_test#243

@ghost ghost assigned evanmcc and seancribbs Mar 26, 2013

@seancribbs seancribbs and 1 other commented on an outdated diff Apr 1, 2013

src/riak_client.erl
@@ -250,7 +250,13 @@ delete(Bucket,Key,Options,Timeout) when is_list(Options) ->
ReqId = mk_reqid(),
riak_kv_delete_sup:start_delete(Node, [ReqId, Bucket, Key, Options, Timeout,
Me, ClientId]),
- wait_for_reqid(ReqId, Timeout);
+ RTimeout = recv_timeout(Options),
+ lager:info("timeouts r ~p t ~p > ~p",
@seancribbs

seancribbs Apr 1, 2013

Contributor

This should be at debug level and probably have a more useful message.

@evanmcc

evanmcc Apr 1, 2013

Contributor

meant to remove.

@seancribbs seancribbs and 1 other commented on an outdated diff Apr 1, 2013

src/riak_kv_wm_object.erl
ensure_doc(Ctx) -> Ctx.
%% @spec delete_resource(reqdata(), context()) -> {true, reqdata(), context()}
%% @doc Delete the document specified.
delete_resource(RD, Ctx=#ctx{bucket=B, key=K, client=C, rw=RW, r=R, w=W,
- pr=PR, pw=PW, dw=DW, timeout=Timeout}) ->
- Options = lists:filter(fun({_, default}) -> false; (_) -> true end,
- [{rw, RW}, {r, R}, {w, W}, {pr, PR}, {pw, PW}, {dw, DW},
- {timeout, Timeout}]),
+ pr=PR, pw=PW, dw=DW}) ->
+ Options0 = lists:filter(fun({_, default}) -> false; (_) -> true end,
@seancribbs

seancribbs Apr 1, 2013

Contributor

Suggested refactoring:

Opts0 = [{rw, RW}, {r, R}, {w, W}, {pr, PR}, {pw, PW}, {dw, DW}, {timeout, Timeout}],
Options = [ {Opt, Val} || {Opt, Val} <- Opts0, Val /= undefined, Val /= default ]

That pattern might also be useful in these other places where you conditionally include the timeout option, e.g.

Opts0 ++ [ {timeout, Timeout} || Timeout /= undefined ]
@evanmcc

evanmcc Apr 1, 2013

Contributor

it's done in three different ways in three different places. Would be happy to refactor it to be uniform, but wasn't sure that this PR was the right place.

@seancribbs

seancribbs Apr 2, 2013

Contributor

I'm ok with adding refactoring all three, or simply making a function to call for the same effect. Cleaning up debt is always good.

Sean Cribbs

On Apr 1, 2013, at 4:29 PM, Evan Vigil-McClanahan notifications@github.com wrote:

In src/riak_kv_wm_object.erl:

ensure_doc(Ctx) -> Ctx.

%% @SPEC delete_resource(reqdata(), context()) -> {true, reqdata(), context()}
%% @doc Delete the document specified.
delete_resource(RD, Ctx=#ctx{bucket=B, key=K, client=C, rw=RW, r=R, w=W,

  •    pr=PR, pw=PW, dw=DW, timeout=Timeout}) ->
    
  • Options = lists:filter(fun({, default}) -> false; () -> true end,
  •    [{rw, RW}, {r, R}, {w, W}, {pr, PR}, {pw, PW}, {dw, DW}, 
    
  •     {timeout, Timeout}]),
    
  •    pr=PR, pw=PW, dw=DW}) ->
    
  • Options0 = lists:filter(fun({, default}) -> false; () -> true end,
    it's done in three different ways in three different places. Would be happy to refactor it to be uniform, but wasn't sure that this PR was the right place.


Reply to this email directly or view it on GitHub.

@seancribbs seancribbs and 1 other commented on an outdated diff Apr 1, 2013

src/riak_client.erl
@@ -295,7 +301,11 @@ delete_vclock(Bucket,Key,VClock,Options,Timeout) when is_list(Options) ->
ReqId = mk_reqid(),
riak_kv_delete_sup:start_delete(Node, [ReqId, Bucket, Key, Options, Timeout,
Me, ClientId, VClock]),
- wait_for_reqid(ReqId, Timeout);
+ RTimeout = recv_timeout(Options),
+ case RTimeout > Timeout of
@seancribbs

seancribbs Apr 1, 2013

Contributor

Why not:

wait_for_reqid(ReqId, erlang:max(Timeout, RTimeout))
@evanmcc

evanmcc Apr 1, 2013

Contributor

erlang:stdlib() > evan:brain()

@seancribbs seancribbs commented on an outdated diff Apr 3, 2013

src/riak_kv_wm_object.erl
+%% string-encoded integer. Store the integer value
+%% in context() if so.
+malformed_timeout_param(RD, Ctx) ->
+ case wrq:get_qs_value("timeout", none, RD) of
+ none ->
+ {false, RD, Ctx};
+ TimeoutStr ->
+ try
+ Timeout = list_to_integer(TimeoutStr),
+ {false, RD, Ctx#ctx{timeout=Timeout}}
+ catch
+ _:_ ->
+ {true,
+ wrq:append_to_resp_body(io_lib:format("Bad timeout "
+ "value ~p, "
+ "using ~p~n",
@seancribbs

seancribbs Apr 3, 2013

Contributor

If you provide a bad timeout value, it doesn't actually use the default but returns a 400 response.

Contributor

seancribbs commented Apr 3, 2013

I also wonder about the arbitrary 100ms being added to the timeout, which produces some strange results if you set the timeout very low. In a sense, you cannot set a timeout lower than 100ms.

Contributor

seancribbs commented Apr 3, 2013

👍 to merge.

@evanmcc evanmcc added a commit that referenced this pull request Apr 3, 2013

@evanmcc evanmcc Merge pull request #521 from basho/pevm-client-specified-timeouts
Changes needed to expose FSM timeouts to clients
3750a36

@evanmcc evanmcc merged commit 3750a36 into master Apr 3, 2013

1 check failed

default The Travis build failed
Details

@seancribbs seancribbs deleted the pevm-client-specified-timeouts branch Apr 1, 2015

@seancribbs seancribbs removed their assignment May 8, 2015

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