diff --git a/src/ns_bucket.erl b/src/ns_bucket.erl index 34492e347f..5667e3b956 100644 --- a/src/ns_bucket.erl +++ b/src/ns_bucket.erl @@ -17,6 +17,7 @@ -include("ns_common.hrl"). -include("ns_config.hrl"). +-include("cut.hrl"). -ifdef(TEST). -include_lib("eunit/include/eunit.hrl"). @@ -27,6 +28,7 @@ bucket_nodes/1, bucket_type/1, replication_type/1, + replica_change/1, create_bucket/3, credentials/1, delete_bucket/1, @@ -712,7 +714,14 @@ set_fast_forward_map(Bucket, Map) -> set_map(Bucket, Map) -> - true = mb_map:is_valid(Map), + case mb_map:is_valid(Map) of + true -> + ok; + different_length_chains -> + %% Never expect to set map with different_length_chains + %% pre-madhatter. + true = cluster_compat_mode:is_cluster_madhatter() + end, update_bucket_config( Bucket, fun (OldConfig) -> @@ -833,6 +842,16 @@ past_vbucket_maps(Config) -> false -> [] end. +replica_change(BucketConfig) -> + replica_change(num_replicas(BucketConfig), + proplists:get_value(map, BucketConfig)). + +replica_change(_NumReplicas, undefined) -> + false; +replica_change(NumReplicas, Map) -> + ExpectedChainLength = NumReplicas + 1, + lists:any(?cut(ExpectedChainLength =/= length(_)), Map). + needs_rebalance(BucketConfig, Nodes) -> Servers = proplists:get_value(servers, BucketConfig, []), case proplists:get_value(type, BucketConfig) of @@ -843,6 +862,7 @@ needs_rebalance(BucketConfig, Nodes) -> _ -> Map = proplists:get_value(map, BucketConfig), Map =:= undefined orelse + replica_change(BucketConfig) orelse lists:sort(Nodes) =/= lists:sort(Servers) orelse ns_rebalancer:map_options_changed(BucketConfig) orelse (ns_rebalancer:unbalanced(Map, BucketConfig) andalso diff --git a/src/ns_janitor.erl b/src/ns_janitor.erl index a32a6e9e3b..018b8d0b9c 100644 --- a/src/ns_janitor.erl +++ b/src/ns_janitor.erl @@ -363,7 +363,17 @@ compute_vbucket_map_fixup(Bucket, BucketConfig, States) -> ignore -> OldChain; _ -> NewChain end || {NewChain, OldChain} <- lists:zip(MapUpdates, Map)], - NewAdjustedMap = maybe_adjust_chain_size(NewMap, BucketConfig), + NewAdjustedMap = case cluster_compat_mode:is_cluster_madhatter() of + true -> + %% Defer adjusting chain length to rebalance, at + %% the time of writing this code the logic is in, + %% ns_rebalancer:do_rebalance_membase_bucket. + NewMap; + false -> + NumReplicas = ns_bucket:num_replicas(BucketConfig), + ns_janitor_map_recoverer:align_replicas(Map, + NumReplicas) + end, NewBucketConfig = case NewAdjustedMap =:= Map of true -> BucketConfig; @@ -374,10 +384,6 @@ compute_vbucket_map_fixup(Bucket, BucketConfig, States) -> end, {NewBucketConfig, IgnoredVBuckets}. -maybe_adjust_chain_size(Map, BucketConfig) -> - NumReplicas = ns_bucket:num_replicas(BucketConfig), - ns_janitor_map_recoverer:align_replicas(Map, NumReplicas). - construct_vbucket_states(VBucket, Chain, States) -> NodeStates = [{N, S} || {N, V, S} <- States, V == VBucket], ChainStates = [{N, proplists:get_value(N, @@ -565,4 +571,29 @@ enumerate_chains_test() -> EnumeratedChains2 = enumerate_chains(Map, undefined), [{0, [a, b, c], []}, {1, [b, c, a], []}] = EnumeratedChains2. +sanify_addition_of_replicas_test() -> + [a, b] = do_sanify_chain("B", [{a, 0, active}, + {b, 0, replica}], + [a, b], [a, b, c], 0), + [a, b] = do_sanify_chain("B", [{a, 0, active}, + {b, 0, replica}, + {c, 0, replica}], + [a, b], [a, b, c], 0), + + %% replica addition with possible move. + [a, b] = do_sanify_chain("B", [{a, 0, dead}, + {b, 0, replica}, + {c, 0, pending}], + [a, b], [c, a, b], 0), + [c, d, a] = do_sanify_chain("B", [{a, 0, dead}, + {b, 0, replica}, + {c, 0, active}, + {d, 0, replica}], + [a, b], [c, d, a], 0), + [c, d, a] = do_sanify_chain("B", [{a, 0, replica}, + {b, 0, replica}, + {c, 0, active}, + {d, 0, replica}], + [a, b], [c, d, a], 0). + -endif. diff --git a/src/ns_rebalancer.erl b/src/ns_rebalancer.erl index 7f17323f6c..402acabfc3 100644 --- a/src/ns_rebalancer.erl +++ b/src/ns_rebalancer.erl @@ -871,10 +871,21 @@ run_janitor_pre_rebalance(BucketName) -> do_rebalance_membase_bucket(Bucket, Config, KeepNodes, ProgressFun, DeltaRecoveryBuckets) -> Map = proplists:get_value(map, Config), + AdjustedMap = case cluster_compat_mode:is_cluster_madhatter() of + true -> + NumReplicas = ns_bucket:num_replicas(Config), + ns_janitor_map_recoverer:align_replicas(Map, + NumReplicas); + false -> + %% Expect equal length map pre mad-hatter, as the + %% janitor fixes it for us. + %% See fun ns_janitor:compute_vbucket_map_fixup. + Map + end, {FastForwardMap, MapOptions} = case lists:keyfind(Bucket, 1, DeltaRecoveryBuckets) of false -> - generate_vbucket_map(Map, KeepNodes, Config); + generate_vbucket_map(AdjustedMap, KeepNodes, Config); {_, _, V} -> V end, @@ -1180,7 +1191,8 @@ build_delta_recovery_buckets_loop(MappedConfigs, DeltaRecoveryBuckets, Acc) -> [{Bucket, BucketConfig, RecoverResult0} | RestMapped] = MappedConfigs, NeedBucket = lists:member(Bucket, DeltaRecoveryBuckets), - RecoverResult = case NeedBucket of + RecoverResult = case NeedBucket andalso + not ns_bucket:replica_change(BucketConfig) of true -> RecoverResult0; false -> @@ -1691,8 +1703,12 @@ membase_delta_recovery_buckets_test() -> ["b1", "b3"] = membase_delta_recovery_buckets(all, MembaseBuckets). build_delta_recovery_buckets_loop_test() -> - MappedConfigs = [{"b1", conf1, {map, opts}}, - {"b2", conf2, false}], + %% Fake num_replicas so that we don't crash in + %% build_delta_recovery_buckets_loop. + Conf1 = [{num_replicas, 1}, conf1], + Conf2 = [{num_replicas, 1}, conf2], + MappedConfigs = [{"b1", Conf1, {map, opts}}, + {"b2", Conf2, false}], All = membase_delta_recovery_buckets(all, [{"b1", conf}, {"b2", conf}]), {ok, []} = build_delta_recovery_buckets_loop([], All, []), @@ -1700,8 +1716,8 @@ build_delta_recovery_buckets_loop_test() -> {error, not_possible} = build_delta_recovery_buckets_loop(MappedConfigs, ["b2"], []), {error, not_possible} = build_delta_recovery_buckets_loop(MappedConfigs, ["b1", "b2"], []), {ok, []} = build_delta_recovery_buckets_loop(MappedConfigs, [], []), - ?assertEqual({ok, [{"b1", conf1, {map, opts}}]}, + ?assertEqual({ok, [{"b1", Conf1, {map, opts}}]}, build_delta_recovery_buckets_loop(MappedConfigs, ["b1"], [])), - ?assertEqual({ok, [{"b1", conf1, {map, opts}}]}, + ?assertEqual({ok, [{"b1", Conf1, {map, opts}}]}, build_delta_recovery_buckets_loop([hd(MappedConfigs)], All, [])). -endif. diff --git a/src/ns_vbucket_mover.erl b/src/ns_vbucket_mover.erl index 942e8e067f..6fdff43d9c 100644 --- a/src/ns_vbucket_mover.erl +++ b/src/ns_vbucket_mover.erl @@ -92,6 +92,7 @@ is_swap_rebalance(OldMap, NewMap) -> length(OldNodes) =/= length(NewNodes) andalso erlang:throw(not_swap), lists:foldl( fun ({_VB, OldChain, NewChain}, Dict0) -> + length(OldChain) =/= length(NewChain) andalso throw(not_swap), Changed = [Pair || {From, To} = Pair <- lists:zip(OldChain, NewChain), From =/= To, From =/= undefined,