migration sweep key boundary code review

[tpodowd]

I'm currently reviewing some code around the migration sweep keys. I've noticed some inconsistency for 2 particular edge cases below. As far as I can see, they are not critical issues and don't cause bad behavior.

1. brick_ets.erl (lines 1050 to 1062)

   {_, {SweepA, SweepZ}} ->
       %% Hint: we know Rs /= [].
       %%
       %% Now we need to check ... if the first key is =< than SweepA,
       %% and if the last key is greater than SweepZ, then we need to
       %% filter out all keys >= SweepZ.  This is case 'F', see above.
       Ks = [element(1, Tuple) || Tuple <- Rs],
       case {hd(Ks) =< SweepA, lists:last(Ks) >= SweepZ} of
           {true, true} ->
               Rs2 = lists:takewhile(
                       fun(T) when element(1, T) =< SweepZ -> true;
                          (_)                              -> false
                       end, Rs),
   

The comment and the code are different. Note that SweepA refers to the last key that has been fully migrated and SweepZ is the last key in the range of keys currently being swept. Ie, keys being swept are those greater than SweepA and less than or equal to SweepZ.

With this in mind, I think the comment should read:
- %% filter out all keys >= SweepZ. This is case 'F', see above.
+ %% filter out all keys > SweepZ. This is case 'F', see above.

And the code should be changed as follows
- case {hd(Ks) =< SweepA, lists:last(Ks) >= SweepZ} of
+ case {hd(Ks) =< SweepA, lists:last(Ks) > SweepZ} of

1. brick_server.erl (lines 4273 to 4284)

   chain_all_keys_outside_sweep_zone(RdWr_Keys, S) ->
   if not is_record(S#state.sweepstate, sweep_r) ->
   ?DBG_MIGRATEx({chain_all_keys_outside_sweep_zone, S#state.name,
   not_sweeping, RdWr_Keys}),
   {true, RdWr_Keys};
   true ->
   {SweepA, SweepZ} = get_sweep_start_and_end(S),
   ?DBG_MIGRATEx({chain_all_keys_outside_sweep_zone, S#state.name,
   SweepA, RdWr_Keys, SweepZ}),
   case [x || {_RdWr, K} <- RdWr_Keys, SweepA =< K, K =< SweepZ] of
   [] ->
   {true, RdWr_Keys};

This code should generate the empty list in the case list comprehension at 4282 if all the keys are outside of the sweep range.

I think the below change is more accurate.

-            case [x || {_RdWr, K} <- RdWr_Keys, SweepA =< K, K =< SweepZ] of
+           case [x || {_RdWr, K} <- RdWr_Keys, SweepA < K, K =< SweepZ] of

Note: both of these cases don't seem to cause any problems as far as I can see. Ie, no data loss is going to happen. But someone else should also take a look.

Tom.