You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by "nickva (via GitHub)" <gi...@apache.org> on 2024/08/21 23:49:57 UTC
[PR] Fix badmatch in find_next_node/0 [couchdb]
nickva opened a new pull request, #5193:
URL: https://github.com/apache/couchdb/pull/5193
Previously, `find_next_node/0` didn't handle odd cases such as current node not being in the `mem3:nodes()` or having an empty `mem3:nodes()` and just crashed with a badmatch.
Make sure to handle those cases and return `node()` for them. We already handled `node()` target as a no-op in `push/2`. We just have to also add it to `maybe_resubmit/2`.
Also, to avoid confusion between "live" nodes and "all" nodes, in the helper function opt to just use `Mem3Nodes` variable name to make it clear what we're dealing with.
Fix #5191
--
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
Re: [PR] Fix badmatch in find_next_node/0 [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #5193:
URL: https://github.com/apache/couchdb/pull/5193#discussion_r1725981421
##########
src/mem3/src/mem3_sync.erl:
##########
@@ -302,12 +304,31 @@ sync_push(ShardName, N) ->
gen_server:call(mem3_sync, {push, #job{name = ShardName, node = N}}, infinity).
find_next_node() ->
- LiveNodes = [node() | nodes()],
- AllNodes0 = lists:sort(mem3:nodes()),
- AllNodes1 = [X || X <- AllNodes0, lists:member(X, LiveNodes)],
- AllNodes = AllNodes1 ++ [hd(AllNodes1)],
- [_Self, Next | _] = lists:dropwhile(fun(N) -> N =/= node() end, AllNodes),
- Next.
+ Self = node(),
+ LiveNodes = [Self | nodes()],
+ Mem3Nodes = mem3:nodes(),
+ find_next_node(Self, LiveNodes, Mem3Nodes).
+
+find_next_node(Self, LiveNodes, Mem3Nodes) ->
+ SortedMem3Nodes = lists:sort(Mem3Nodes),
+ LiveMem3Nodes = [N || N <- SortedMem3Nodes, lists:member(N, LiveNodes)],
+ case LiveMem3Nodes of
+ [] ->
+ % No live mem3 nodes. Strange. Return self as we can always
+ % replicate to ourselves, which is just a no-op.
+ Self;
+ [H | _] ->
+ % Append head to the end we have always have a "next"
Review Comment:
Can you clarify this comment a little?
--
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
Re: [PR] Fix badmatch in find_next_node/0 [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva merged PR #5193:
URL: https://github.com/apache/couchdb/pull/5193
--
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
Re: [PR] Fix badmatch in find_next_node/0 [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on PR #5193:
URL: https://github.com/apache/couchdb/pull/5193#issuecomment-2304903172
It is a bit cute to use a replicate to self as a fallback. I was mainly relying on a an existing "replicate to self" being a no-op in
https://github.com/apache/couchdb/blob/637fb79f56af6371f4522eb34467601be726c94c/src/mem3/src/mem3_sync.erl#L79-L82 with nonode would we have had to add both special cases one for self one for `nonode` in various places.
I built a small test module to play with with the previous logic wondering the same thing, why we didn't see this before?
```erlang
-module(nn).
-export([
find_next_node/3
]).
find_next_node(Self, LiveNodes, Mem3Nodes) ->
AllNodes0 = lists:sort(Mem3Nodes),
AllNodes1 = [X || X <- AllNodes0, lists:member(X, LiveNodes)],
AllNodes = AllNodes1 ++ [hd(AllNodes1)],
[_Self, Next | _] = lists:dropwhile(fun(N) -> N =/= Self end, AllNodes),
Next.
```
```erlang
> c(nn).
> nn:find_next_node(n, [a,n], [a]).
** exception error: no match of right hand side value []
in function nn:find_next_node/3 (nn.erl, line 11)
```
So one case where we'd trigger this is if the node we're on removes itself from the nodes list. Then the user reported the logs being filled and the machine was "frozen". It must have happened between the initial sync started with the node in the mem3:nodes() list then it was removed and an error happened, where initial_sync crashed.
```
[error] 2024-08-21T19:02:06.680089Z couchdb@192.168.10.235 emulator -------- Error in process <0.30202.42> on node 'couchdb@192.168.10.235' with exit value:
{{badmatch,[]},[{mem3_sync,find_next_node,0,[{file,"src/mem3_sync.erl"},{line,309}]},{mem3_sync,sync_nodes_and_dbs,0,[{file,"src/mem3_sync.erl"},{line,265}]},{mem3_sync,initial_sync,1,[{file,"src/mem3_sync.erl"},{line,272}]}]}
```
On a crash we end up restarting it https://github.com/apache/couchdb/blob/d38f14f7d777b7cda79b9862ee304150ad3418ea/src/mem3/src/mem3_sync_nodes.erl#L75-L78 and from then on it will just keep crashing. So I opted to fold both the self and the "odd" cases like that into the already existing "no-op" case so the crash cycle due to this reason doesn't happen.
--
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
Re: [PR] Fix badmatch in find_next_node/0 [couchdb]
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on PR #5193:
URL: https://github.com/apache/couchdb/pull/5193#issuecomment-2303913319
an explicit `nonode` result would be clearer, relying on the caller to decide to do nothing if it gets back `node()` is a bit cute. still, we need to handle these edge cases, it's interesting that we've never encountered this before.
--
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
Re: [PR] Fix badmatch in find_next_node/0 [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5193:
URL: https://github.com/apache/couchdb/pull/5193#discussion_r1726006710
##########
src/mem3/src/mem3_sync.erl:
##########
@@ -302,12 +304,31 @@ sync_push(ShardName, N) ->
gen_server:call(mem3_sync, {push, #job{name = ShardName, node = N}}, infinity).
find_next_node() ->
- LiveNodes = [node() | nodes()],
- AllNodes0 = lists:sort(mem3:nodes()),
- AllNodes1 = [X || X <- AllNodes0, lists:member(X, LiveNodes)],
- AllNodes = AllNodes1 ++ [hd(AllNodes1)],
- [_Self, Next | _] = lists:dropwhile(fun(N) -> N =/= node() end, AllNodes),
- Next.
+ Self = node(),
+ LiveNodes = [Self | nodes()],
+ Mem3Nodes = mem3:nodes(),
+ find_next_node(Self, LiveNodes, Mem3Nodes).
+
+find_next_node(Self, LiveNodes, Mem3Nodes) ->
+ SortedMem3Nodes = lists:sort(Mem3Nodes),
+ LiveMem3Nodes = [N || N <- SortedMem3Nodes, lists:member(N, LiveNodes)],
+ case LiveMem3Nodes of
+ [] ->
+ % No live mem3 nodes. Strange. Return self as we can always
+ % replicate to ourselves, which is just a no-op.
+ Self;
+ [H | _] ->
+ % Append head to the end we have always have a "next"
Review Comment:
I tried to expand the comment a bit explaining how it's supposed to work. dropwhile usage is a bit tricky.
--
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
Re: [PR] Fix badmatch in find_next_node/0 [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5193:
URL: https://github.com/apache/couchdb/pull/5193#discussion_r1726003743
##########
src/mem3/src/mem3_sync.erl:
##########
@@ -302,12 +304,31 @@ sync_push(ShardName, N) ->
gen_server:call(mem3_sync, {push, #job{name = ShardName, node = N}}, infinity).
find_next_node() ->
- LiveNodes = [node() | nodes()],
- AllNodes0 = lists:sort(mem3:nodes()),
- AllNodes1 = [X || X <- AllNodes0, lists:member(X, LiveNodes)],
- AllNodes = AllNodes1 ++ [hd(AllNodes1)],
- [_Self, Next | _] = lists:dropwhile(fun(N) -> N =/= node() end, AllNodes),
- Next.
+ Self = node(),
+ LiveNodes = [Self | nodes()],
+ Mem3Nodes = mem3:nodes(),
+ find_next_node(Self, LiveNodes, Mem3Nodes).
+
+find_next_node(Self, LiveNodes, Mem3Nodes) ->
+ SortedMem3Nodes = lists:sort(Mem3Nodes),
+ LiveMem3Nodes = [N || N <- SortedMem3Nodes, lists:member(N, LiveNodes)],
+ case LiveMem3Nodes of
+ [] ->
+ % No live mem3 nodes. Strange. Return self as we can always
+ % replicate to ourselves, which is just a no-op.
+ Self;
+ [H | _] ->
+ % Append head to the end we have always have a "next"
Review Comment:
Oh definitely, I'll update it. It's very confusingly written.
--
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