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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
