Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored leveled_sst gen_fsm into gen_statem #401

Closed

Conversation

ThomasArts
Copy link
Contributor

No description provided.

Copy link
Owner

@martinsumner martinsumner left a comment

Choose a reason for hiding this comment

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

There are some code coverage issues. I'm loathe to give up on 100% code coverage, as I'm worried about missing a regression in code coverage if we tolerate anything less. There is some code here which I think can never be triggered, but also one case (update_blockindex_cache being cast in a race in a switch to delete_pending) which probably needs a test to get coverage.

The casting of timeout sin leveled_sst I think will end when we merge in the latest fix from develop-3.0

cdb_clerkcomplete(Pid) ->
gen_fsm:send_all_state_event(Pid, clerk_complete).
gen_statem:cast(Pid, clerk_complete).

Copy link
Owner

Choose a reason for hiding this comment

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

The clerk_complete message can only be sent when in the reader state. It is send only by the leveled_iclerk, and the leveled_iclerk only examines files in this state. The state cannot be transitioned to delete_pending without the leveled_iclerk first having called clerk_complete - so there should be no race. So handling this message should be done in the reader state only.

There is an issue otherwise with code coverage, in that there will no longer be 100% code coverage (and so spotting regression of code coverage will be hard).

{keep_state_and_data, [hibernate]};
starting(info, _Msg, _State) ->
{keep_state_and_data, []}.

Copy link
Owner

Choose a reason for hiding this comment

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

If there are no info messages supported should we remove handling of them. Again code coverage being the issue.

end.
end;
starting(cast, {update_blockindex_cache, BIC}, State) ->
handle_update_blockindex_cache(BIC, State);

Copy link
Owner

Choose a reason for hiding this comment

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

This message cannot be received in the startup state, as it is prompted from get_slots which can only occur in the reader state

{stop, normal, State};

delete_pending(cast, {update_blockindex_cache, BIC}, State) ->
handle_update_blockindex_cache(BIC, State);
Copy link
Owner

Choose a reason for hiding this comment

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

This is interesting. get_slots may be prompted which results in update_blockindex_cache but a set_for_delete message may happen in-between. Hence we do need support for update_blockindex_cache in delete_pending state - but this is never triggered in test, so we perhaps need a unit test to explicitly trigger it.

@martinsumner martinsumner mentioned this pull request Mar 13, 2023
@martinsumner
Copy link
Owner

Replaced by #404

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants