You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by eiri <gi...@git.apache.org> on 2016/07/08 19:46:30 UTC
[GitHub] couchdb-fabric pull request #61: Ignore already received replies for same sh...
GitHub user eiri opened a pull request:
https://github.com/apache/couchdb-fabric/pull/61
Ignore already received replies for same shards
It is possible to get a reply for same shard from different node sent before its collector get stopped in `remove_overlapping_shards`.
This running condition leads to a possibility of same info to be aggregated multiple times.
COUCHDB-3053
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloudant/couchdb-fabric 69654-fix-view-info-duplicates
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-fabric/pull/61.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #61
----
commit 4c04e39458b4cb9f9f8ccab8dbf7deaf1414b166
Author: Eric Avdey <ei...@eiri.ca>
Date: 2016-07-08T18:54:47Z
Ignore already received replies for same shards
It is possible to get a reply for same shard
from different node sent before its collector
get stopped in remove_overlapping_shards.
This running condition leads to a possibility
of same info to be aggregated multiple times.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fabric pull request #61: Ignore already received replies for same sh...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-fabric/pull/61#discussion_r70131900
--- Diff: src/fabric_group_info.erl ---
@@ -58,17 +58,23 @@ handle_message({rexi_EXIT, Reason}, Shard, {Counters, Acc, Ushards}) ->
end;
handle_message({ok, Info}, Shard, {Counters0, Acc, Ushards}) ->
- NewAcc = append_result(Info, Shard, Acc, Ushards),
- Counters1 = fabric_dict:store(Shard, ok, Counters0),
- Counters = fabric_view:remove_overlapping_shards(Shard, Counters1),
- case is_complete(Counters) of
- false ->
- {ok, {Counters, NewAcc, Ushards}};
- true ->
- Pending = aggregate_pending(NewAcc),
- Infos = get_infos(NewAcc),
- Results = [{updates_pending, {Pending}} | merge_results(Infos)],
- {stop, Results}
+ case fabric_dict:lookup_element(Shard, Counters0) of
+ undefined ->
+ % already heard from other node in this range
--- End diff --
Something wrong here. [fabric_dict:lookup_element](https://github.com/apache/couchdb-fabric/blob/master/src/fabric_dict.erl#L42) is based on [couch_util:get_value](https://github.com/apache/couchdb-couch/blob/master/src/couch_util.erl#L167-L176) which return `undefined` in case when key is not found by default. Here it seems happens the opposite. And the `nil` below confuses more.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fabric issue #61: Ignore already received replies for same shards
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the issue:
https://github.com/apache/couchdb-fabric/pull/61
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fabric pull request #61: Ignore already received replies for same sh...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-fabric/pull/61#discussion_r70165869
--- Diff: src/fabric_group_info.erl ---
@@ -58,17 +58,23 @@ handle_message({rexi_EXIT, Reason}, Shard, {Counters, Acc, Ushards}) ->
end;
handle_message({ok, Info}, Shard, {Counters0, Acc, Ushards}) ->
- NewAcc = append_result(Info, Shard, Acc, Ushards),
- Counters1 = fabric_dict:store(Shard, ok, Counters0),
- Counters = fabric_view:remove_overlapping_shards(Shard, Counters1),
- case is_complete(Counters) of
- false ->
- {ok, {Counters, NewAcc, Ushards}};
- true ->
- Pending = aggregate_pending(NewAcc),
- Infos = get_infos(NewAcc),
- Results = [{updates_pending, {Pending}} | merge_results(Infos)],
- {stop, Results}
+ case fabric_dict:lookup_element(Shard, Counters0) of
+ undefined ->
+ % already heard from other node in this range
--- End diff --
I got it. Thank you!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fabric pull request #61: Ignore already received replies for same sh...
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on a diff in the pull request:
https://github.com/apache/couchdb-fabric/pull/61#discussion_r70139806
--- Diff: src/fabric_group_info.erl ---
@@ -58,17 +58,23 @@ handle_message({rexi_EXIT, Reason}, Shard, {Counters, Acc, Ushards}) ->
end;
handle_message({ok, Info}, Shard, {Counters0, Acc, Ushards}) ->
- NewAcc = append_result(Info, Shard, Acc, Ushards),
- Counters1 = fabric_dict:store(Shard, ok, Counters0),
- Counters = fabric_view:remove_overlapping_shards(Shard, Counters1),
- case is_complete(Counters) of
- false ->
- {ok, {Counters, NewAcc, Ushards}};
- true ->
- Pending = aggregate_pending(NewAcc),
- Infos = get_infos(NewAcc),
- Results = [{updates_pending, {Pending}} | merge_results(Infos)],
- {stop, Results}
+ case fabric_dict:lookup_element(Shard, Counters0) of
+ undefined ->
+ % already heard from other node in this range
--- End diff --
It can for the shards we haven't received a response yet.
It goes like this - we are initiating `Counters` dict with all the shards as keys and values as `nil`. Shard is a combination of range and node, so even though they are all unique, ranges (and accordingly their view_info) are not. We are doing this because we can't guess which node with a given range will respond first or respond at all.
Now, once we are starting to receive responses, we are updating our `Counters0` dict with `ok` for the received range+node, with gives as `Counters1`, and then removing all the duplicated ranges for the rest of the nodes, which gives as `Counters`.
Once our `Counters` dict doesn't have any `nil` left (`is_complete/1` is a check for that) we know that we've received the responses for all the ranges and aggregating info we've kept on `NewAcc ` into final response.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fabric pull request #61: Ignore already received replies for same sh...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/couchdb-fabric/pull/61
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fabric pull request #61: Ignore already received replies for same sh...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-fabric/pull/61#discussion_r70132063
--- Diff: src/fabric_group_info.erl ---
@@ -58,17 +58,23 @@ handle_message({rexi_EXIT, Reason}, Shard, {Counters, Acc, Ushards}) ->
end;
handle_message({ok, Info}, Shard, {Counters0, Acc, Ushards}) ->
- NewAcc = append_result(Info, Shard, Acc, Ushards),
- Counters1 = fabric_dict:store(Shard, ok, Counters0),
- Counters = fabric_view:remove_overlapping_shards(Shard, Counters1),
- case is_complete(Counters) of
- false ->
- {ok, {Counters, NewAcc, Ushards}};
- true ->
- Pending = aggregate_pending(NewAcc),
- Infos = get_infos(NewAcc),
- Results = [{updates_pending, {Pending}} | merge_results(Infos)],
- {stop, Results}
+ case fabric_dict:lookup_element(Shard, Counters0) of
+ undefined ->
+ % already heard from other node in this range
+ {ok, {Counters0, Acc, Ushards}};
+ nil ->
+ NewAcc = append_result(Info, Shard, Acc, Ushards),
+ Counters1 = fabric_dict:store(Shard, ok, Counters0),
--- End diff --
It seems you want to match by `ok`, not `nil` for case clause.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fabric pull request #61: Ignore already received replies for same sh...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-fabric/pull/61#discussion_r70134929
--- Diff: src/fabric_group_info.erl ---
@@ -58,17 +58,23 @@ handle_message({rexi_EXIT, Reason}, Shard, {Counters, Acc, Ushards}) ->
end;
handle_message({ok, Info}, Shard, {Counters0, Acc, Ushards}) ->
- NewAcc = append_result(Info, Shard, Acc, Ushards),
- Counters1 = fabric_dict:store(Shard, ok, Counters0),
- Counters = fabric_view:remove_overlapping_shards(Shard, Counters1),
- case is_complete(Counters) of
- false ->
- {ok, {Counters, NewAcc, Ushards}};
- true ->
- Pending = aggregate_pending(NewAcc),
- Infos = get_infos(NewAcc),
- Results = [{updates_pending, {Pending}} | merge_results(Infos)],
- {stop, Results}
+ case fabric_dict:lookup_element(Shard, Counters0) of
+ undefined ->
+ % already heard from other node in this range
--- End diff --
Oh, a bit complicated. Thanks for explanation! So `Counters0` here cannot contains any else values except `nil`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-fabric pull request #61: Ignore already received replies for same sh...
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on a diff in the pull request:
https://github.com/apache/couchdb-fabric/pull/61#discussion_r70134251
--- Diff: src/fabric_group_info.erl ---
@@ -58,17 +58,23 @@ handle_message({rexi_EXIT, Reason}, Shard, {Counters, Acc, Ushards}) ->
end;
handle_message({ok, Info}, Shard, {Counters0, Acc, Ushards}) ->
- NewAcc = append_result(Info, Shard, Acc, Ushards),
- Counters1 = fabric_dict:store(Shard, ok, Counters0),
- Counters = fabric_view:remove_overlapping_shards(Shard, Counters1),
- case is_complete(Counters) of
- false ->
- {ok, {Counters, NewAcc, Ushards}};
- true ->
- Pending = aggregate_pending(NewAcc),
- Infos = get_infos(NewAcc),
- Results = [{updates_pending, {Pending}} | merge_results(Infos)],
- {stop, Results}
+ case fabric_dict:lookup_element(Shard, Counters0) of
+ undefined ->
+ % already heard from other node in this range
--- End diff --
You are missing `remove_overlapping_shards` bit here. It finds overlapping shards, sends their rexi workers a kill signal and then removes them from Counters dict.
So we can have either element with a key `Shard` and value `nil` (we haven't heard from that range from any of the nodes yet) or no element (and hence `undefined`), because we already got a response from concurrent shard on a different node, updated it with `ok` and then removed _this_ Shard from Counters. This answers your next question, why we match on `nil` and not on `ok`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---