Skip to content
This repository has been archived by the owner on Sep 19, 2019. It is now read-only.

Support mem3_shards hot upgrade #84

Merged
merged 3 commits into from Jun 19, 2017
Merged

Support mem3_shards hot upgrade #84

merged 3 commits into from Jun 19, 2017

Conversation

jaydoane
Copy link
Contributor

@jaydoane jaydoane commented Jun 16, 2017

I tested the upgrade and downgrade paths by first making sure mem3_shards was compiled from tag 3.9.2, then updating the source with this patch, recompiling, then in a remsh

f(_change_code), _change_code = fun(Module, OldVsn) ->
    ok = sys:suspend(Module),
    {module, Module} = l(Module),
    ok = sys:change_code(Module, Module, OldVsn, []),
    ok = sys:resume(Module)
end.

f(_table_exists), _table_exists = fun(Tab) ->
    try ets:tab2list(Tab) of
        Val ->
            true
    catch
        error:badarg ->
            false
    end
end.

f(_state_size), _state_size = fun(Name) ->
    size(sys:get_state(Name)) - 1 % exclude tag
end.

f(_validate), _validate = fun
    (pre) ->
        false = _table_exists(mem3_openers),
        3 == _state_size(mem3_shards),
        ok;
    (post)->
        true = _table_exists(mem3_openers),
        5 == _state_size(mem3_shards),
        ok
end.

ok = _validate(pre).
ok = _change_code(mem3_shards, 1).
ok = _validate(post).
ok = _change_code(mem3_shards, {down, 2}).
ok = _validate(pre).

@jaydoane jaydoane requested review from davisp and rnewson June 16, 2017 08:20
Copy link
Contributor

@rnewson rnewson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your emulation must be wrong, or my understanding of release handler is wrong (could genuinely be either).

What should happen, I think, is that, when downgrading, the current version of the module is called with code_change with {down, 2}, this returns a state that the old version (1) can understand.

If it isn't that, I don't see how you're supposed to write downgrade clauses.

twig:log(notice, "~p code_change ~p ~p", [?MODULE, OldVsn, St]),
upgrade(MaxSize, CurSize, ChangesPid);

code_change(OldVsn, {st, MaxSize, CurSize, ChangesPid, _, _} = St, _Extra) ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given this matches the arity of the current state record, this clause will be executed on all future hot upgrades, activating the downgrade code and crashing. You'll need to match the OldVsn here as {down, 2} to be sure we only do this on actual downgrade.

@@ -242,7 +242,48 @@ terminate(_Reason, #st{changes_pid=Pid}) ->
exit(Pid, kill),
ok.

code_change(_OldVsn, #st{}=St, _Extra) ->
code_change(OldVsn, {st, MaxSize, CurSize, ChangesPid} = St, _Extra) ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would, for safety, match OldVsn as 1.

@jaydoane
Copy link
Contributor Author

@rnewson @robfraz this seems to be passing manual testing now, after I improved my understanding of sys:change_code/4

@@ -242,7 +242,49 @@ terminate(_Reason, #st{changes_pid=Pid}) ->
exit(Pid, kill),
ok.

code_change(_OldVsn, #st{}=St, _Extra) ->
code_change(1 = OldVsn, {st, MaxSize, CurSize, ChangesPid} = St, _Extra) ->
twig:log(notice, "~p code_change ~p ~p", [?MODULE, OldVsn, St]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this log line was just for you while figuring this out or is it useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was helpful for debugging, and since it should only happen once, I figured I might as well leave it in. Do you want me to remove the logging?

code_change({down, 2} = OldVsn, St, _Extra) ->
{st, MaxSize, CurSize, ChangesPid, _, _} = St,
twig:log(notice, "~p code_change ~p ~p", [?MODULE, OldVsn, St]),
downgrade(MaxSize, CurSize, ChangesPid).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's keep a code change clause that matches on #st{} so we don't screw ourselves over on the next release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call -- PR updated with default clause

@jaydoane jaydoane merged commit 30142dd into master Jun 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants