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

Extract out console facing FSM from riak_cs_gc_d #1144

Merged
merged 8 commits into from
May 18, 2015
Merged

Conversation

kuenishi
Copy link
Contributor

@kuenishi kuenishi commented May 7, 2015

riak_cs_gc_d used to have bunch of complex states not only name of
states exposed in gen_fsm, but also many elements in gc_d_state
record. This commit extracts operator-facing states such as idle,
paused, running from riak_cs_gc_d. riak_cs_gc_d is not a long-running
worker under supervision tree any more, but riak_cs_gc_manager is.
The lifecycle of riak_cs_gc_d is now as long as one GC batch - if the
GC has finished then riak_cs_gc_d process terminates.

See comments in both riak_cs_gc_d.erl and riak_cs_gc_manager.erl for
detail state diagram and description.

riak_cs_gc_d used to have bunch of complex states not only name of
states exposed in gen_fsm, but also many elements in gc_d_state
record. This commit extracts operator-facing states such as idle,
paused, running from riak_cs_gc_d. riak_cs_gc_d is not a long-running
worker under supervision tree any more, but riak_cs_gc_manager is.
The lifecycle of riak_cs_gc_d is now as long as one GC batch - if the
GC has finished then riak_cs_gc_d process terminates.

See comments in both riak_cs_gc_d.erl and riak_cs_gc_manager.erl for
detail state diagram and description.
@kuenishi kuenishi added this to the 2.1.0 milestone May 7, 2015
@@ -106,3 +93,14 @@
-define(DEFAULT_GC_BATCH_SIZE, 1000).
-define(DEFAULT_GC_WORKERS, 2).
-define(EPOCH_START, <<"0">>).

-record(gc_manager_state, {
Copy link
Contributor

Choose a reason for hiding this comment

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

This state record is referenced only from manager module, I guess.
Maybe just historical(?) reason, how about putting it into manger module file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because they're also used in quickcheck tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

( ̄ー ̄)b

For now, pause and remove can be replaced with stopping and setting
interval. This simplifies two FSMs drastically in clean way.
%%% Internal functions
%%%===================================================================

maybe_fetch_first_key(?STATE{batch_start=BatchStart,
Copy link
Contributor

Choose a reason for hiding this comment

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

This function fetches keys regardless of any condition. Then, "maybe" is not needed and s/key/keys/.

@shino
Copy link
Contributor

shino commented May 14, 2015

riak-cs-admin script has usage infromation, which should be updated.

                echo "Usage: $SCRIPT gc { batch [<leeway_seconds>] | status | pause | resume | cancel |"
                echo "                          set-interval <interval_seconds> | set-leeway <leeway_seconds> }"

%% start -> idle -(start)-> running
%% ^ |
%% +----(finish)---+
%% +----(stop)-----+
Copy link
Contributor

Choose a reason for hiding this comment

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

s/stop/cancel/

@kuenishi
Copy link
Contributor Author

I don't think any usage of riak-cs-admin has changed?


%% ===================================================================
%% Test API and tests
%% ===================================================================
Copy link
Contributor

Choose a reason for hiding this comment

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

elapsed below is not (only) for testing.

@shino
Copy link
Contributor

shino commented May 15, 2015

I don't think any usage of riak-cs-admin has changed?

True. Sorry for my misunderstanding (again) 😞

@shino
Copy link
Contributor

shino commented May 18, 2015

Eunit, xref , dialyzer succeeded.
Much cleaner code. Great refactoring.

+1 to merge.

borshop added a commit that referenced this pull request May 18, 2015
Extract out console facing FSM from riak_cs_gc_d

Reviewed-by: shino
@shino
Copy link
Contributor

shino commented May 18, 2015

@borshop merge

@Basho-JIRA
Copy link

The pull request was merged Monday, without starting the sprint (Most of the work was finished, so I was working on next work).

_[posted via JIRA by Kota Uenishi]_

@Basho-JIRA
Copy link

No release note needed as this is just a code cleanup.

_[posted via JIRA by Kota Uenishi]_

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

Successfully merging this pull request may close these issues.

None yet

4 participants