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

storage: Fix for leo-project/leofs#553 #948

Merged
merged 1 commit into from
Dec 7, 2017

Conversation

windkit
Copy link
Contributor

@windkit windkit commented Dec 5, 2017

No description provided.

@windkit windkit changed the base branch from master to develop December 5, 2017 07:34
@windkit windkit requested a review from mocchira December 5, 2017 07:44
@windkit windkit self-assigned this Dec 5, 2017
Copy link
Member

@mocchira mocchira left a comment

Choose a reason for hiding this comment

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

Overall structure of code/tests LGTM. a few suggestions to improve efficiency here so please check those out.

@@ -1180,9 +1180,11 @@ get_active_redundancies(_, []) ->
get_active_redundancies(Quorum, Redundancies) ->
AvailableNodes = [RedundantNode ||
#redundant_node{available = true} = RedundantNode <- Redundancies],
UnavailableNodes = [RedundantNode ||
#redundant_node{available = false} = RedundantNode <- Redundancies],
Copy link
Member

Choose a reason for hiding this comment

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

Why not just having the flag that indicates if there is unavailable node instead of list UnavailableNodes?

case (Quorum =< erlang:length(AvailableNodes)) of
true ->
{ok, AvailableNodes};
{ok, AvailableNodes, UnavailableNodes};
Copy link
Member

Choose a reason for hiding this comment

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

As described above, just return bool (true/false) instead of list.

{error, Error};
read_and_repair_1(ReadParams, [Node|Rest], AvailableNodes, Errors) ->
%%read_and_repair_1(_,[],_,[Error|_]) ->
%% {error, Error};
Copy link
Member

Choose a reason for hiding this comment

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

Remove the comment above

%%read_and_repair_1(_,[],_,[Error|_]) ->
%% {error, Error};

read_and_repair_1(_,[],_,[], Errors) ->
Copy link
Member

Choose a reason for hiding this comment

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

Match against false instead of the empty list.

@@ -1200,19 +1202,34 @@ get_active_redundancies(Quorum, Redundancies) ->
Cause::any()).
read_and_repair(#read_parameter{quorum = Q} = ReadParams, Redundancies) ->
case get_active_redundancies(Q, Redundancies) of
{ok, AvailableNodes} ->
read_and_repair_1(ReadParams, AvailableNodes, AvailableNodes, []);
{ok, AvailableNodes, UnavailableNodes} ->
Copy link
Member

Choose a reason for hiding this comment

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

the name also should be renamed if it become the flag like HasUnavailableNodes.

@windkit windkit force-pushed the issue_553 branch 2 times, most recently from a24b423 to 930b1d4 Compare December 6, 2017 01:42
@mocchira mocchira merged commit 327f9ea into leo-project:develop Dec 7, 2017
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