[GitHub] [couchdb] jaydoane commented on a diff in pull request #4194: Introduce {rmin, R} option for fabric:handle_response/4,5

[GitBox <gi...@apache.org>]

jaydoane commented on code in PR #4194:
URL: https://github.com/apache/couchdb/pull/4194#discussion_r985272360


##########
src/fabric/src/fabric_ring.erl:
##########
@@ -118,6 +117,14 @@ handle_response(Shard, Response, Workers, Responses, RingOpts) ->
 %    ring, where all copies at the start of the range and end of the range must
 %    have the same boundary values.
 %
+%  * When RingOpts is [{rmin, R}], where R is an integer, accumulate responses
+%    until the ring can be completed at least R times. If shards had not been

Review Comment:
   maybe s/had/have/ ?



##########
src/fabric/src/fabric_ring.erl:
##########
@@ -118,6 +117,14 @@ handle_response(Shard, Response, Workers, Responses, RingOpts) ->
 %    ring, where all copies at the start of the range and end of the range must
 %    have the same boundary values.
 %
+%  * When RingOpts is [{rmin, R}], where R is an integer, accumulate responses
+%    until the ring can be completed at least R times. If shards had not been
+%    split, this would be the equivalent waiting until there are at least R
+%    responses for each shard range. Also of note is that [] is not exactly
+%    equivalent with [{rmin, 1}], as with [{rmin, 1}] it's possible that some

Review Comment:
   s/equivalent with/equivalent to/



##########
src/fabric/src/fabric_ring.erl:
##########
@@ -546,6 +613,64 @@ node_down_test() ->
 
     ?assertEqual(error, node_down(n3, Workers5, Responses3)).
 
+% Check that cleanup callback for fabric:handle_response/* gets called both to
+% kill workers which haven't returned or results which where not used (for

Review Comment:
   s/where/were/ ?



##########
src/fabric/src/fabric_ring.erl:
##########
@@ -133,6 +140,10 @@ handle_response(Shard, Response, Workers, Responses, RingOpts, CleanupCb) ->
             #shard{range = [B, E]} = Shard,
             Responses1 = [{{B, E}, Shard, Response} | Responses],
             handle_response_ring(Workers1, Responses1, CleanupCb);
+        [{rmin, RMin}] when is_integer(RMin), RMin >= 1 ->

Review Comment:
   is this the only place we restrict RMin to be an integer >= 1? Can it be e.g. 0 here, and if so crash with a bad match?



##########
src/fabric/src/fabric_ring.erl:
##########
@@ -118,6 +117,14 @@ handle_response(Shard, Response, Workers, Responses, RingOpts) ->
 %    ring, where all copies at the start of the range and end of the range must
 %    have the same boundary values.
 %
+%  * When RingOpts is [{rmin, R}], where R is an integer, accumulate responses
+%    until the ring can be completed at least R times. If shards had not been
+%    split, this would be the equivalent waiting until there are at least R
+%    responses for each shard range. Also of note is that [] is not exactly
+%    equivalent with [{rmin, 1}], as with [{rmin, 1}] it's possible that some
+%    shard ranges would return more than R copies while waiting for other
+%    ranges respond.

Review Comment:
   ranges _to_ respond ?



-- 
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: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org