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 2021/08/24 22:59:12 UTC
[GitHub] [couchdb] nickva opened a new pull request #3711: Avoid change feed rewinds on shard moves
nickva opened a new pull request #3711:
URL: https://github.com/apache/couchdb/pull/3711
When shards are moved, attempt to match missing nodes and ranges from the old changes sequence by inspecting current shard uuids.
Previously, if a node and range was missing, we randomly picked a node in the appropriate range, so 1/3 of the time we might have hit the exact node, but 2/3 of the times we would end up with a complete changes feed rewind.
Since update sequences contain uuids of each of the shards, and assuming shard moves preserve uuid (they are just plain file copies), it is possible to match based on that instead of guessing.
Unfortunately, this involves a fabric worker scatter gather operation to all shard copies. This should only happen when we get an incomplete sequence. We rely on that happening rarely, mostly right after the shards move, and in case users stashed change feed sequence from the old shard map configuration. Afterwards, users should start using the new last_seq values which would have the new shard map configuration with the new nodes.
--
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 change in pull request #3711: Avoid change feed rewinds on shard moves
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3711:
URL: https://github.com/apache/couchdb/pull/3711#discussion_r696901337
##########
File path: src/fabric/src/fabric_view_changes.erl
##########
@@ -495,6 +497,59 @@ get_old_seq(#shard{range=R}=Shard, SinceSeqs) ->
end.
+get_db_uuids(DbName) ->
Review comment:
That might look better, good point
--
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 change in pull request #3711: Avoid change feed rewinds on shard moves
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3711:
URL: https://github.com/apache/couchdb/pull/3711#discussion_r696904428
##########
File path: src/fabric/src/fabric_view_changes.erl
##########
@@ -495,6 +497,59 @@ get_old_seq(#shard{range=R}=Shard, SinceSeqs) ->
end.
+get_db_uuids(DbName) ->
+ % Need to use an isolated process as we are performing a fabric call from
+ % another fabric call and there is a good chance we'd polute the mailbox
+ % with returned messages
+ Timeout = fabric_util:request_timeout(),
+ IsolatedFun = fun() -> fabric:db_uuids(DbName) end,
+ try fabric_util:isolate(IsolatedFun, Timeout) of
+ {ok, Uuids} ->
+ % Trim uuids so we match exactly based on the currently configured
+ % uuid_prefix_len. The assumption is that we are getting an older
+ % sequence from the same cluster and we didn't tweak that
+ % relatively obscure config option in the meantime.
+ PrefixLen = config:get_integer("fabric", "uuid_prefix_len", 7),
Review comment:
We are reusing the same config defaults from the fabric_rpc. Perhaps it might be better to move them to fabric_util so to they both can call fabric_util:get_uuid_prefix().
The reason to re-use the config is because we can then do exact matching in the map. We could do a prefix match, with a filter loop, but I figured since this config value is almost never changed just rely assuming it didn't change since previous changes sequence was generated until after the shard moved
--
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 change in pull request #3711: Avoid change feed rewinds on shard moves
Posted by GitBox <gi...@apache.org>.
jaydoane commented on a change in pull request #3711:
URL: https://github.com/apache/couchdb/pull/3711#discussion_r696198604
##########
File path: src/fabric/src/fabric_view_changes.erl
##########
@@ -495,6 +497,59 @@ get_old_seq(#shard{range=R}=Shard, SinceSeqs) ->
end.
+get_db_uuids(DbName) ->
+ % Need to use an isolated process as we are performing a fabric call from
+ % another fabric call and there is a good chance we'd polute the mailbox
+ % with returned messages
+ Timeout = fabric_util:request_timeout(),
+ IsolatedFun = fun() -> fabric:db_uuids(DbName) end,
+ try fabric_util:isolate(IsolatedFun, Timeout) of
+ {ok, Uuids} ->
+ % Trim uuids so we match exactly based on the currently configured
+ % uuid_prefix_len. The assumption is that we are getting an older
+ % sequence from the same cluster and we didn't tweak that
+ % relatively obscure config option in the meantime.
+ PrefixLen = config:get_integer("fabric", "uuid_prefix_len", 7),
+ maps:fold(fun(Uuid, Shard, Acc) ->
+ TrimmedUuid = binary:part(Uuid, {0, PrefixLen}),
+ Acc#{TrimmedUuid => Shard}
+ end, #{}, Uuids);
+ {error, Error} ->
+ % Since we are doing a best-effort approach to match moved shards,
+ % tollerate and log errors. This should also handle cases when the
+ % cluster is partially upgraded, as some nodes will not have the
+ % newer get_uuid fabric_rpc handler.
+ ErrMsg = "~p : could not get db_uuids for Db:~p Error:~p",
+ couch_log:error(ErrMsg, [?MODULE, DbName, Error]),
+ #{}
+ catch
+ _Tag:Error ->
+ ErrMsg = "~p : could not get db_uuids for Db:~p Error:~p",
+ couch_log:error(ErrMsg, [?MODULE, DbName, Error]),
+ #{}
+ end.
+
+
+%% Determine if the missing shard moved to a new node. Do that by matching the
+%% uuids from the current shard map. If we cannot find a moved shard, we return
+%% the original node and range as a shard and hope for the best.
+replace_moved_shard(Node, Range, Seq, #{} = _Uuids) when is_number(Seq) ->
+ % Cannot figure out shard moves wouthout uuid matching
+ #shard{node = Node, range = Range};
+replace_moved_shard(Node, Range, {Seq, Uuid}, #{} = Uuids) ->
+ % Compatibility case for an old seq format which didn't have epoch nodes
+ replace_moved_shard(Node, Range, {Seq, Uuid, Node}, Uuids);
+replace_moved_shard(Node, Range, {_Seq, Uuid, _EpochNode}, #{} = Uuids) ->
+ case Uuids of
+ #{Uuid := #shard{range = Range} = Shard} ->
+ % Found a moved shard by matchign both the uuid and the range
Review comment:
s/matchign/matching/
##########
File path: src/fabric/src/fabric_db_uuids.erl
##########
@@ -0,0 +1,67 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(fabric_db_uuids).
+
+
+-export([go/1]).
+
+
+-include_lib("fabric/include/fabric.hrl").
+-include_lib("mem3/include/mem3.hrl").
+
+
+go(DbName) when is_binary(DbName) ->
+ Shards = mem3:live_shards(DbName, [node() | nodes()]),
+ Workers = fabric_util:submit_jobs(Shards, get_uuid, []),
+ RexiMon = fabric_util:create_monitors(Shards),
+ Acc0 = {fabric_dict:init(Workers, nil), []},
+ try fabric_util:recv(Workers, #shard.ref, fun handle_message/3, Acc0) of
+ {timeout, {WorkersDict, _}} ->
Review comment:
indentation?
##########
File path: src/fabric/src/fabric_view_changes.erl
##########
@@ -495,6 +497,59 @@ get_old_seq(#shard{range=R}=Shard, SinceSeqs) ->
end.
+get_db_uuids(DbName) ->
Review comment:
Since this function seems to return a mapping of `UUID => Shard`, would a name like e.g. `db_uuid_shards` be more descriptive?
##########
File path: src/fabric/src/fabric_view_changes.erl
##########
@@ -495,6 +497,59 @@ get_old_seq(#shard{range=R}=Shard, SinceSeqs) ->
end.
+get_db_uuids(DbName) ->
+ % Need to use an isolated process as we are performing a fabric call from
+ % another fabric call and there is a good chance we'd polute the mailbox
+ % with returned messages
+ Timeout = fabric_util:request_timeout(),
+ IsolatedFun = fun() -> fabric:db_uuids(DbName) end,
+ try fabric_util:isolate(IsolatedFun, Timeout) of
+ {ok, Uuids} ->
+ % Trim uuids so we match exactly based on the currently configured
+ % uuid_prefix_len. The assumption is that we are getting an older
+ % sequence from the same cluster and we didn't tweak that
+ % relatively obscure config option in the meantime.
+ PrefixLen = config:get_integer("fabric", "uuid_prefix_len", 7),
+ maps:fold(fun(Uuid, Shard, Acc) ->
+ TrimmedUuid = binary:part(Uuid, {0, PrefixLen}),
+ Acc#{TrimmedUuid => Shard}
+ end, #{}, Uuids);
+ {error, Error} ->
+ % Since we are doing a best-effort approach to match moved shards,
+ % tollerate and log errors. This should also handle cases when the
+ % cluster is partially upgraded, as some nodes will not have the
+ % newer get_uuid fabric_rpc handler.
+ ErrMsg = "~p : could not get db_uuids for Db:~p Error:~p",
+ couch_log:error(ErrMsg, [?MODULE, DbName, Error]),
+ #{}
+ catch
+ _Tag:Error ->
+ ErrMsg = "~p : could not get db_uuids for Db:~p Error:~p",
+ couch_log:error(ErrMsg, [?MODULE, DbName, Error]),
+ #{}
+ end.
+
+
+%% Determine if the missing shard moved to a new node. Do that by matching the
+%% uuids from the current shard map. If we cannot find a moved shard, we return
+%% the original node and range as a shard and hope for the best.
+replace_moved_shard(Node, Range, Seq, #{} = _Uuids) when is_number(Seq) ->
+ % Cannot figure out shard moves wouthout uuid matching
+ #shard{node = Node, range = Range};
+replace_moved_shard(Node, Range, {Seq, Uuid}, #{} = Uuids) ->
Review comment:
Maybe something like `UuidShards` instead of `Uuids`?
##########
File path: src/fabric/test/eunit/fabric_moved_shards_seq_tests.erl
##########
@@ -0,0 +1,123 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(fabric_moved_shards_seq_tests).
+
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+-include_lib("mem3/include/mem3.hrl").
+
+
+-define(TDEF(A), {atom_to_list(A), fun A/0}).
+
+
+main_test_() ->
+ {
+ setup,
+ fun setup/0,
+ fun teardown/1,
+ [
+ ?TDEF(t_shard_moves_avoid_sequence_rewinds)
+ ]
+ }.
+
+
+setup() ->
+ test_util:start_couch([fabric]).
+
+
+
+teardown(Ctx) ->
+ meck:unload(),
+ test_util:stop_couch(Ctx).
+
+
+t_shard_moves_avoid_sequence_rewinds() ->
+ DocCnt = 30,
+ DbName = ?tempdb(),
+
+ ok = fabric:create_db(DbName, [{q,1}, {n,1}]),
+ lists:foreach(fun(I) ->
+ update_doc(DbName, #doc{id = erlang:integer_to_binary(I)})
+ end, lists:seq(1, DocCnt)),
+
+ {ok, _, Seq1, 0} = changes(DbName, #changes_args{limit = 1, since ="now"}),
+ [{_, Range, {Seq, Uuid, _}}] = seq_decode(Seq1),
+
+ % Transform Seq1 pretending it came from a fake source node, before the
+ % shard was moved to the current node.
+ SrcNode = 'srcnode@srchost',
+ Seq2 = seq_encode([{SrcNode, Range, {Seq, Uuid, SrcNode}}]),
+
+ % First, check when the shard file epoch is mismatched epoch and the
+ % sequence would rewind. This ensures the epoch and uuid check protection
+ % in couch_db works as intended.
+ ResBadEpoch = changes(DbName, #changes_args{limit = 1, since = Seq2}),
Review comment:
Does the `Res` prefix here and below signify Reshard? I think using the full word would be more clear, since "Res" could also mean e.g. Result.
##########
File path: src/fabric/test/eunit/fabric_moved_shards_seq_tests.erl
##########
@@ -0,0 +1,123 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(fabric_moved_shards_seq_tests).
+
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+-include_lib("mem3/include/mem3.hrl").
+
+
+-define(TDEF(A), {atom_to_list(A), fun A/0}).
+
+
+main_test_() ->
+ {
+ setup,
+ fun setup/0,
+ fun teardown/1,
+ [
+ ?TDEF(t_shard_moves_avoid_sequence_rewinds)
+ ]
+ }.
+
+
+setup() ->
+ test_util:start_couch([fabric]).
+
Review comment:
extra space
##########
File path: src/fabric/src/fabric_view_changes.erl
##########
@@ -495,6 +497,59 @@ get_old_seq(#shard{range=R}=Shard, SinceSeqs) ->
end.
+get_db_uuids(DbName) ->
+ % Need to use an isolated process as we are performing a fabric call from
+ % another fabric call and there is a good chance we'd polute the mailbox
+ % with returned messages
+ Timeout = fabric_util:request_timeout(),
+ IsolatedFun = fun() -> fabric:db_uuids(DbName) end,
+ try fabric_util:isolate(IsolatedFun, Timeout) of
+ {ok, Uuids} ->
+ % Trim uuids so we match exactly based on the currently configured
+ % uuid_prefix_len. The assumption is that we are getting an older
+ % sequence from the same cluster and we didn't tweak that
+ % relatively obscure config option in the meantime.
+ PrefixLen = config:get_integer("fabric", "uuid_prefix_len", 7),
+ maps:fold(fun(Uuid, Shard, Acc) ->
+ TrimmedUuid = binary:part(Uuid, {0, PrefixLen}),
+ Acc#{TrimmedUuid => Shard}
+ end, #{}, Uuids);
+ {error, Error} ->
+ % Since we are doing a best-effort approach to match moved shards,
+ % tollerate and log errors. This should also handle cases when the
Review comment:
s/tollerate/tolerate/
##########
File path: src/fabric/src/fabric_view_changes.erl
##########
@@ -495,6 +497,59 @@ get_old_seq(#shard{range=R}=Shard, SinceSeqs) ->
end.
+get_db_uuids(DbName) ->
+ % Need to use an isolated process as we are performing a fabric call from
+ % another fabric call and there is a good chance we'd polute the mailbox
+ % with returned messages
+ Timeout = fabric_util:request_timeout(),
+ IsolatedFun = fun() -> fabric:db_uuids(DbName) end,
+ try fabric_util:isolate(IsolatedFun, Timeout) of
+ {ok, Uuids} ->
+ % Trim uuids so we match exactly based on the currently configured
+ % uuid_prefix_len. The assumption is that we are getting an older
+ % sequence from the same cluster and we didn't tweak that
+ % relatively obscure config option in the meantime.
+ PrefixLen = config:get_integer("fabric", "uuid_prefix_len", 7),
+ maps:fold(fun(Uuid, Shard, Acc) ->
+ TrimmedUuid = binary:part(Uuid, {0, PrefixLen}),
+ Acc#{TrimmedUuid => Shard}
+ end, #{}, Uuids);
+ {error, Error} ->
+ % Since we are doing a best-effort approach to match moved shards,
+ % tollerate and log errors. This should also handle cases when the
+ % cluster is partially upgraded, as some nodes will not have the
+ % newer get_uuid fabric_rpc handler.
Review comment:
this is a very nice feature!
##########
File path: src/fabric/src/fabric_view_changes.erl
##########
@@ -495,6 +497,59 @@ get_old_seq(#shard{range=R}=Shard, SinceSeqs) ->
end.
+get_db_uuids(DbName) ->
+ % Need to use an isolated process as we are performing a fabric call from
+ % another fabric call and there is a good chance we'd polute the mailbox
+ % with returned messages
+ Timeout = fabric_util:request_timeout(),
+ IsolatedFun = fun() -> fabric:db_uuids(DbName) end,
+ try fabric_util:isolate(IsolatedFun, Timeout) of
+ {ok, Uuids} ->
+ % Trim uuids so we match exactly based on the currently configured
+ % uuid_prefix_len. The assumption is that we are getting an older
+ % sequence from the same cluster and we didn't tweak that
+ % relatively obscure config option in the meantime.
+ PrefixLen = config:get_integer("fabric", "uuid_prefix_len", 7),
Review comment:
This seems pretty subtle. Is there any reason to not just pick a number like `7` here, to avoid the (unlikely) possibility of that config changing, or perhaps worse, being different on each node?
##########
File path: src/fabric/test/eunit/fabric_moved_shards_seq_tests.erl
##########
@@ -0,0 +1,123 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(fabric_moved_shards_seq_tests).
+
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+-include_lib("mem3/include/mem3.hrl").
+
+
+-define(TDEF(A), {atom_to_list(A), fun A/0}).
+
+
+main_test_() ->
+ {
+ setup,
+ fun setup/0,
+ fun teardown/1,
+ [
+ ?TDEF(t_shard_moves_avoid_sequence_rewinds)
+ ]
+ }.
+
+
+setup() ->
+ test_util:start_couch([fabric]).
+
+
+
+teardown(Ctx) ->
+ meck:unload(),
+ test_util:stop_couch(Ctx).
+
+
+t_shard_moves_avoid_sequence_rewinds() ->
+ DocCnt = 30,
+ DbName = ?tempdb(),
+
+ ok = fabric:create_db(DbName, [{q,1}, {n,1}]),
+ lists:foreach(fun(I) ->
+ update_doc(DbName, #doc{id = erlang:integer_to_binary(I)})
+ end, lists:seq(1, DocCnt)),
+
+ {ok, _, Seq1, 0} = changes(DbName, #changes_args{limit = 1, since ="now"}),
Review comment:
missing space `since ="now"`
##########
File path: src/fabric/test/eunit/fabric_moved_shards_seq_tests.erl
##########
@@ -0,0 +1,123 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(fabric_moved_shards_seq_tests).
+
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+-include_lib("mem3/include/mem3.hrl").
+
+
+-define(TDEF(A), {atom_to_list(A), fun A/0}).
+
+
+main_test_() ->
+ {
+ setup,
+ fun setup/0,
+ fun teardown/1,
+ [
+ ?TDEF(t_shard_moves_avoid_sequence_rewinds)
+ ]
+ }.
+
+
+setup() ->
+ test_util:start_couch([fabric]).
+
+
+
+teardown(Ctx) ->
+ meck:unload(),
+ test_util:stop_couch(Ctx).
+
+
+t_shard_moves_avoid_sequence_rewinds() ->
+ DocCnt = 30,
+ DbName = ?tempdb(),
+
+ ok = fabric:create_db(DbName, [{q,1}, {n,1}]),
+ lists:foreach(fun(I) ->
+ update_doc(DbName, #doc{id = erlang:integer_to_binary(I)})
+ end, lists:seq(1, DocCnt)),
+
+ {ok, _, Seq1, 0} = changes(DbName, #changes_args{limit = 1, since ="now"}),
+ [{_, Range, {Seq, Uuid, _}}] = seq_decode(Seq1),
+
+ % Transform Seq1 pretending it came from a fake source node, before the
+ % shard was moved to the current node.
+ SrcNode = 'srcnode@srchost',
+ Seq2 = seq_encode([{SrcNode, Range, {Seq, Uuid, SrcNode}}]),
+
+ % First, check when the shard file epoch is mismatched epoch and the
+ % sequence would rewind. This ensures the epoch and uuid check protection
+ % in couch_db works as intended.
+ ResBadEpoch = changes(DbName, #changes_args{limit = 1, since = Seq2}),
+ ?assertMatch({ok, _, _, _}, ResBadEpoch),
+ {ok, _, _, PendingBadEpoch} = ResBadEpoch,
Review comment:
This is similarly confusing, since the suffix `Epoch` is being used first for a tuple, and then for an integer. Maybe you could use the suffix `Changes` for the tuple structure and `Epoch` for the integer? e.g.:
```
ResBadChanges = changes(DbName, #changes_args{limit = 1, since = Seq2}),
{ok, _, _, PendingBadEpoch} = ResBadChanges,
```
##########
File path: src/fabric/src/fabric_view_changes.erl
##########
@@ -495,6 +497,59 @@ get_old_seq(#shard{range=R}=Shard, SinceSeqs) ->
end.
+get_db_uuids(DbName) ->
+ % Need to use an isolated process as we are performing a fabric call from
+ % another fabric call and there is a good chance we'd polute the mailbox
+ % with returned messages
+ Timeout = fabric_util:request_timeout(),
+ IsolatedFun = fun() -> fabric:db_uuids(DbName) end,
+ try fabric_util:isolate(IsolatedFun, Timeout) of
+ {ok, Uuids} ->
+ % Trim uuids so we match exactly based on the currently configured
+ % uuid_prefix_len. The assumption is that we are getting an older
+ % sequence from the same cluster and we didn't tweak that
+ % relatively obscure config option in the meantime.
+ PrefixLen = config:get_integer("fabric", "uuid_prefix_len", 7),
+ maps:fold(fun(Uuid, Shard, Acc) ->
+ TrimmedUuid = binary:part(Uuid, {0, PrefixLen}),
+ Acc#{TrimmedUuid => Shard}
+ end, #{}, Uuids);
+ {error, Error} ->
+ % Since we are doing a best-effort approach to match moved shards,
+ % tollerate and log errors. This should also handle cases when the
+ % cluster is partially upgraded, as some nodes will not have the
+ % newer get_uuid fabric_rpc handler.
+ ErrMsg = "~p : could not get db_uuids for Db:~p Error:~p",
+ couch_log:error(ErrMsg, [?MODULE, DbName, Error]),
+ #{}
+ catch
+ _Tag:Error ->
+ ErrMsg = "~p : could not get db_uuids for Db:~p Error:~p",
+ couch_log:error(ErrMsg, [?MODULE, DbName, Error]),
+ #{}
+ end.
+
+
+%% Determine if the missing shard moved to a new node. Do that by matching the
+%% uuids from the current shard map. If we cannot find a moved shard, we return
+%% the original node and range as a shard and hope for the best.
+replace_moved_shard(Node, Range, Seq, #{} = _Uuids) when is_number(Seq) ->
+ % Cannot figure out shard moves wouthout uuid matching
Review comment:
s/wouthout/without/
--
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 #3711: Avoid change feed rewinds on shard moves
Posted by GitBox <gi...@apache.org>.
nickva merged pull request #3711:
URL: https://github.com/apache/couchdb/pull/3711
--
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 change in pull request #3711: Avoid change feed rewinds on shard moves
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3711:
URL: https://github.com/apache/couchdb/pull/3711#discussion_r696899708
##########
File path: src/fabric/test/eunit/fabric_moved_shards_seq_tests.erl
##########
@@ -0,0 +1,123 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(fabric_moved_shards_seq_tests).
+
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+-include_lib("mem3/include/mem3.hrl").
+
+
+-define(TDEF(A), {atom_to_list(A), fun A/0}).
+
+
+main_test_() ->
+ {
+ setup,
+ fun setup/0,
+ fun teardown/1,
+ [
+ ?TDEF(t_shard_moves_avoid_sequence_rewinds)
+ ]
+ }.
+
+
+setup() ->
+ test_util:start_couch([fabric]).
+
+
+
+teardown(Ctx) ->
+ meck:unload(),
+ test_util:stop_couch(Ctx).
+
+
+t_shard_moves_avoid_sequence_rewinds() ->
+ DocCnt = 30,
+ DbName = ?tempdb(),
+
+ ok = fabric:create_db(DbName, [{q,1}, {n,1}]),
+ lists:foreach(fun(I) ->
+ update_doc(DbName, #doc{id = erlang:integer_to_binary(I)})
+ end, lists:seq(1, DocCnt)),
+
+ {ok, _, Seq1, 0} = changes(DbName, #changes_args{limit = 1, since ="now"}),
+ [{_, Range, {Seq, Uuid, _}}] = seq_decode(Seq1),
+
+ % Transform Seq1 pretending it came from a fake source node, before the
+ % shard was moved to the current node.
+ SrcNode = 'srcnode@srchost',
+ Seq2 = seq_encode([{SrcNode, Range, {Seq, Uuid, SrcNode}}]),
+
+ % First, check when the shard file epoch is mismatched epoch and the
+ % sequence would rewind. This ensures the epoch and uuid check protection
+ % in couch_db works as intended.
+ ResBadEpoch = changes(DbName, #changes_args{limit = 1, since = Seq2}),
Review comment:
Res is result, I should rename it to avoid the confusion
--
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 change in pull request #3711: Avoid change feed rewinds on shard moves
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3711:
URL: https://github.com/apache/couchdb/pull/3711#discussion_r696900416
##########
File path: src/fabric/test/eunit/fabric_moved_shards_seq_tests.erl
##########
@@ -0,0 +1,123 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(fabric_moved_shards_seq_tests).
+
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+-include_lib("mem3/include/mem3.hrl").
+
+
+-define(TDEF(A), {atom_to_list(A), fun A/0}).
+
+
+main_test_() ->
+ {
+ setup,
+ fun setup/0,
+ fun teardown/1,
+ [
+ ?TDEF(t_shard_moves_avoid_sequence_rewinds)
+ ]
+ }.
+
+
+setup() ->
+ test_util:start_couch([fabric]).
+
+
+
+teardown(Ctx) ->
+ meck:unload(),
+ test_util:stop_couch(Ctx).
+
+
+t_shard_moves_avoid_sequence_rewinds() ->
+ DocCnt = 30,
+ DbName = ?tempdb(),
+
+ ok = fabric:create_db(DbName, [{q,1}, {n,1}]),
+ lists:foreach(fun(I) ->
+ update_doc(DbName, #doc{id = erlang:integer_to_binary(I)})
+ end, lists:seq(1, DocCnt)),
+
+ {ok, _, Seq1, 0} = changes(DbName, #changes_args{limit = 1, since ="now"}),
+ [{_, Range, {Seq, Uuid, _}}] = seq_decode(Seq1),
+
+ % Transform Seq1 pretending it came from a fake source node, before the
+ % shard was moved to the current node.
+ SrcNode = 'srcnode@srchost',
+ Seq2 = seq_encode([{SrcNode, Range, {Seq, Uuid, SrcNode}}]),
+
+ % First, check when the shard file epoch is mismatched epoch and the
+ % sequence would rewind. This ensures the epoch and uuid check protection
+ % in couch_db works as intended.
+ ResBadEpoch = changes(DbName, #changes_args{limit = 1, since = Seq2}),
+ ?assertMatch({ok, _, _, _}, ResBadEpoch),
+ {ok, _, _, PendingBadEpoch} = ResBadEpoch,
Review comment:
Good idea. I went with calling the whole results `Result1`, `Result2` and then Pending values `PendingNoRewind` and `PendingRewind`. The idea being that the result shape didn't change, we expect an `{ok, _, _, _}` in both cases.
--
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 change in pull request #3711: Avoid change feed rewinds on shard moves
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3711:
URL: https://github.com/apache/couchdb/pull/3711#discussion_r696900416
##########
File path: src/fabric/test/eunit/fabric_moved_shards_seq_tests.erl
##########
@@ -0,0 +1,123 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(fabric_moved_shards_seq_tests).
+
+
+-include_lib("couch/include/couch_eunit.hrl").
+-include_lib("couch/include/couch_db.hrl").
+-include_lib("mem3/include/mem3.hrl").
+
+
+-define(TDEF(A), {atom_to_list(A), fun A/0}).
+
+
+main_test_() ->
+ {
+ setup,
+ fun setup/0,
+ fun teardown/1,
+ [
+ ?TDEF(t_shard_moves_avoid_sequence_rewinds)
+ ]
+ }.
+
+
+setup() ->
+ test_util:start_couch([fabric]).
+
+
+
+teardown(Ctx) ->
+ meck:unload(),
+ test_util:stop_couch(Ctx).
+
+
+t_shard_moves_avoid_sequence_rewinds() ->
+ DocCnt = 30,
+ DbName = ?tempdb(),
+
+ ok = fabric:create_db(DbName, [{q,1}, {n,1}]),
+ lists:foreach(fun(I) ->
+ update_doc(DbName, #doc{id = erlang:integer_to_binary(I)})
+ end, lists:seq(1, DocCnt)),
+
+ {ok, _, Seq1, 0} = changes(DbName, #changes_args{limit = 1, since ="now"}),
+ [{_, Range, {Seq, Uuid, _}}] = seq_decode(Seq1),
+
+ % Transform Seq1 pretending it came from a fake source node, before the
+ % shard was moved to the current node.
+ SrcNode = 'srcnode@srchost',
+ Seq2 = seq_encode([{SrcNode, Range, {Seq, Uuid, SrcNode}}]),
+
+ % First, check when the shard file epoch is mismatched epoch and the
+ % sequence would rewind. This ensures the epoch and uuid check protection
+ % in couch_db works as intended.
+ ResBadEpoch = changes(DbName, #changes_args{limit = 1, since = Seq2}),
+ ?assertMatch({ok, _, _, _}, ResBadEpoch),
+ {ok, _, _, PendingBadEpoch} = ResBadEpoch,
Review comment:
Good idea
--
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