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 2022/10/01 03:57:55 UTC
[GitHub] [couchdb] nickva opened a new pull request, #4194: Introduce {rmin, R} option for fabric:handle_response/4,5
nickva opened a new pull request, #4194:
URL: https://github.com/apache/couchdb/pull/4194
This option is useful when we want to accumulate at least R result copies for each document. Typically R=2 in a default 3 node cluster. Without shard splitting, this would simply be waiting for at least R copies returned from each range. However, in the case when shard copies don't have exactly the same range boundaries, the correct logic is to try to completely cover the ring at least R times. The algorithm for checking if the ring can be covered is already implemented, we just add it as an option to fabric:handle_response/4,5.
This is essentially another preparatory PR to implement the optimized `fabric:bulk_get(...)` API as described in issue #4183 and it's just split out as a separate commit as it's a fairly standalone change.
Additionally, had noticed that the unit tests in fabric_ring didn't cover the cleanup callback function, so added tests to make sure we exercise that code path.
Emacs + erlang_ls also noticed the unused header included, so that's why that was removed.
--
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
[GitHub] [couchdb] nickva commented on a diff in pull request #4194: Introduce {rmin, R} option for fabric:handle_response/4,5
Posted by GitBox <gi...@apache.org>.
nickva commented on code in PR #4194:
URL: https://github.com/apache/couchdb/pull/4194#discussion_r985919864
##########
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:
Since this is an internal function, the assumption is that by now the value of R has been checked above somewhere and this is more of an assert. It also goes along with the pattern in this function where we'd blow up with a case clause on unexpected `RingOpts` values.
In general we could conceive of a case where R=0 means "return even if we we haven't completed the ring even once" but it's more likely that we've just forgot to validate its value and that's not something we meant to do.
--
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
[GitHub] [couchdb] nickva commented on a diff in pull request #4194: Introduce {rmin, R} option for fabric:handle_response/4,5
Posted by GitBox <gi...@apache.org>.
nickva commented on code in PR #4194:
URL: https://github.com/apache/couchdb/pull/4194#discussion_r985919864
##########
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:
Since this is an internal function, the assumption is that by now the value of R has been checked above somewhere and this is more of an assert. It also goes along with the pattern in this function where we'd blow up with a case clause on unexpected `RingOpts` values.
In general we could conceive of a case where R=0 means returns even if we we haven't completed the ring even once but it's more likely that we've just forgot to validate its value and that's not something we meant to do.
--
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
[GitHub] [couchdb] jaydoane commented on a diff in pull request #4194: Introduce {rmin, R} option for fabric:handle_response/4,5
Posted by 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
[GitHub] [couchdb] nickva merged pull request #4194: Introduce {rmin, R} option for fabric:handle_response/4,5
Posted by GitBox <gi...@apache.org>.
nickva merged PR #4194:
URL: https://github.com/apache/couchdb/pull/4194
--
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