Fix Dialyzer warnings #528

Closed
wants to merge 78 commits into
from

Projects

None yet

7 participants

@seancribbs
Contributor

Getting a clean slate. Only remaining warnings may be:

Unknown functions:
  cluster_info:format/3
  cluster_info:register_app/1

Those will disappear if cluster_info is on the code path.

@jrwest jrwest and 1 other commented on an outdated diff Feb 18, 2014
src/riak_core_claimant.erl
@@ -400,8 +398,8 @@ maybe_commit_staged(State) ->
maybe_commit_staged(Ring, State=#state{changes=Changes, seed=Seed}) ->
Changes2 = filter_changes(Changes, Ring),
case compute_next_ring(Changes2, Seed, Ring) of
- {legacy, _} ->
- {ignore, legacy};
+ {error, invalid_resize_claim} ->
+ lager:error("invalid_resize_claim BUG");
jrwest
jrwest Feb 18, 2014 Contributor

this probably doesn't need to log. just surface the error to the user? or does this path not bubble back up that far...need to look...

jrwest
jrwest Feb 18, 2014 Contributor

ok we should be able to bubble this up to the user like so (sorry this is going to be a little verbose because im working through it as i type):

seancribbs
seancribbs Feb 18, 2014 Contributor

How's this?

io:format("Unable to commit staged ring changes while ring is resizing.~n"
          "Check that there are no pending changes in 'riak-admin ring-status'~n"
          "If there are, try again once they are completed,~n"
          "Otherwise try again shortly.~n");
jrwest
jrwest Feb 20, 2014 Contributor

sorry for the delay:

"Unable to commit staged ring changes while ring is resizing" -> "Unable to commit staged ring changes"

The issue is not that the ring is resizing but that another operation took place (e.g. unstaged join) between the plan and the commit. Not sure how to best explain that, so figure its easier just to be short.

Contributor
  • Revert lists:foreach into LCs which are faster, underscore match result.
  • Break out riak_core_vnode:vnode_forward.
  • Address #528 (comment)
  • Restore deleted annotations to riak_core_apl functions.
seancribbs added some commits Feb 6, 2014
@seancribbs seancribbs Fix unknown types caused by typos 0fa9183
@seancribbs seancribbs Match some expected return values that are important
Some of these should really cause crashes when errors are returned,
since later expressions expect them to have succeeded.
032988e
@seancribbs seancribbs RIP Legacy gossip
Removed the related dead code.
064a5b5
@seancribbs seancribbs Background manager types were incorrectly specified 092ed75
@seancribbs seancribbs Split resolve/2 into to two functions which have disjoint signatures. acb7455
@seancribbs seancribbs Fix some type warnings related to timers 8f8970b
@seancribbs seancribbs Correct the type of human_time/3, was too narrow f0f5531
@seancribbs seancribbs Remove overly verbose annotations that can be inferrred 2de80a1
@seancribbs seancribbs Remove case clauses in riak_core_security that will never match a959718
@seancribbs seancribbs orddict:orddict/0, not orddict:orddict/2 87d5862
@seancribbs seancribbs Simplify the return type of maybe_ensure_vnodes_started/1 so callers …
…don't have to match.
7717aeb
@seancribbs seancribbs Comment out this unused expression.
TODO: Ask @jonmeredith @jtuple what this was for
8f3a142
@seancribbs seancribbs equal_cstate/3 is never called with true (except in tests) and not ex…
…ported.

The case where it returns verbose information is only useful for the
test, and should probably instead be tested with EQC.
1941854
@seancribbs seancribbs Fix two obscure unmatched returns. 64e0348
@seancribbs seancribbs Correct case for missing clause related to ring resizing.
TODO: This seems like a real bug that should be addressed. Ask @jrwest
a467c04
@seancribbs seancribbs Remove commented code. 5fd11b8
@seancribbs seancribbs Unify the type of case expression so we need not match it. d220fa7
@seancribbs seancribbs Remove more legacy gossip warnings 3d52d75
@seancribbs seancribbs Match the return value of rpc_every_member. d0753a6
@seancribbs seancribbs The function() type refers to funs, not function names. d695207
@seancribbs seancribbs Value is returned along with unit. 0435a9c
@seancribbs seancribbs Match the return value of riak_core:join when rejoining.
Since we expect this to succeed (some other node just asked us to
rejoin), logging an error when it doesn't seems most reasonable.
0921f89
@seancribbs seancribbs Match the return value of timer:apply_after. 8157681
@seancribbs seancribbs Fix type for dynamic handoff size status.
riak_core_handoff_manager.erl:392: The pattern {F, 'dynamic'} can never match the type 'undefined' | {non_neg_integer(),'bytes' | 'objects'}
97cbeb1
@seancribbs seancribbs Add callback info for gen_nb_server. 4f8ea20
@seancribbs seancribbs Add callback info for riak_core_gen_server.
riak_core_handoff_receiver.erl:25: Callback info about the riak_core_gen_server behaviour is not available
75a0756
@seancribbs seancribbs Match the return value of update_existing_metric. dc5d247
@seancribbs seancribbs Fix missing type. 4bb4c86
@seancribbs seancribbs Fix type declaration on record.
ets table names can be used as tids when given the 'named_table'
option.
ad1dedd
@seancribbs seancribbs Replace use of tuple funs with module funs and fix type of system_ter…
…minate/4
241b526
@seancribbs seancribbs Byte size is allowed to be a float when formatting.
Also, match return value of lager:info call (not sure why?)
e25259e
@seancribbs seancribbs Match the return value of code:load_binary 6e777ef
@seancribbs seancribbs Match return value of random:seed/1 5b983e4
@seancribbs seancribbs Thread through state changes and match return values of cancel_timer 619bd93
@seancribbs seancribbs Use io:write where formatting isn't needed and thread state changes t…
…hrough.
d8b9ff9
@seancribbs seancribbs Match return values in riak_core_ring_manager 370ebd7
@seancribbs seancribbs Remove specs that were causing warnings in riak_core_vnode d72bf1b
@seancribbs seancribbs Add FIXME for vnode forwarding.
riak_core_vnode.erl:383: Expression produces a value of type {'next_state','active',#state{index::'undefined' | non_neg_integer(),mod::atom() | tuple(),forward::atom() | [any()],handoff_target::'none' | {_,_},handoff_pid::'undefined' | pid(),handoff_type::'hinted_handoff' | 'ownership_transfer' | 'resize_transfer' | 'undefined',pool_pid::'undefined' | pid(),pool_config::'undefined' | tuple(),manager_event_timer::'undefined' | reference(),inactivity_timeout::'undefined' | non_neg_integer()},'undefined' | non_neg_integer()}, but this value is unmatched
e7e9139
@seancribbs seancribbs Loosen the type for entries in chstate() that nearly_equal/2 modifies. 8f68ecc
@seancribbs seancribbs Fix types in vclock.
* vc_entry is spurious
* Only one opaque type was needed, dot()
* Empty lists are vclocks too! No need to widen the type.
b242c2c
@seancribbs seancribbs Fix misuse of orddict:merge (discovered via unmatched return).
NB: orddict:merge/3 does not call the fun for all values in the merged
orddict, but only for ones that are in both. The function previously
padded each orddict with 'undefined' values to ensure that the entire
list of members on both sides was represented. It is less clean
looking, but much less computationally complex to sort the members
status and do a two-way recursive iteration.
54290f7
@seancribbs seancribbs Whitespace cleanup 0afefd0
@seancribbs seancribbs More timer junk, have to match the return value even if unused. 36a298c
@seancribbs seancribbs Match return value of start_link.
If it's not {ok, Pid}, we won't be able to trace anyway.
4635189
@seancribbs seancribbs Simplify test in case clause via extraction.
NB @Vagabond: There were several possibilities tested that according
to the code and type signatures could never occur. There is possibly
an unintended outcome here that needs to be addressed.
e867dc4
@seancribbs seancribbs Whitespace cleanup 97d2a78
@seancribbs seancribbs Backport fixes from later supervisor versions and match cancel_timer …
…return.
caa03b5
@seancribbs seancribbs riak_core_pw_auth:hash_password/1 never returns {error, _}. 19846f7
@seancribbs seancribbs Remove more legacy gossip cruft. bc8a30f
@seancribbs seancribbs Fix some types around coverage requests.
* Coverage requests to multiple vnodes reconstruct the original
  sender() to include the {partition(), node()} in the "reference".
  Widened the type to include that case.
* Added an actual type for keyspace specifications.
ad74add
@seancribbs seancribbs Fix type annotation on write_buffer 05a6498
@seancribbs seancribbs There's no path to send_a_cast/2, remove it. 870df5a
@seancribbs seancribbs Cleanup comments, Andrew says those code paths were speculative. f27d621
@seancribbs seancribbs dtrace module was only in prerelease/custom OTP versions, now dyntrace. 0c46fed
@seancribbs seancribbs In tests, do_write_ringfile returns 'nop'. 5b3be1d
@seancribbs seancribbs Fix failing test in riak_core_ring.
The debugging info of "which reasons why these weren't equal" is only
used in testing. As long as this test has been passing for a long
time, I don't see a reason why to keep the other form.
460a987
@seancribbs seancribbs Remove the implicit continue/1 call in vnode_forward.
Make the callers use continue/1,2 explicitly instead.
c6f8559
@seancribbs seancribbs Ignore-match return values from LCs, keeping them LCs where possible. 8c1855a
Contributor

Sorry for the commit rearrangement, I interactively removed the big foreach commit to change them back to LCs, then added a commit to remove the warnings.

@jrwest
Contributor
jrwest commented on 7cd9d9b Feb 20, 2014

thanks!

@jrwest jrwest commented on the diff Feb 21, 2014
src/hashtree.erl
@@ -631,7 +631,7 @@ get_disk_bucket(Level, Bucket, #state{id=Id, ref=Ref}) ->
set_disk_bucket(Level, Bucket, Val, State=#state{id=Id, ref=Ref}) ->
HKey = encode_bucket(Id, Level, Bucket),
Bin = term_to_binary(Val),
- eleveldb:put(Ref, HKey, Bin, []),
+ ok = eleveldb:put(Ref, HKey, Bin, []),
jrwest
jrwest Feb 21, 2014 Contributor

@jtuple are we ok w/ semantic change here that we may crash if failing to write to leveldb instead of silently ignoring it? I think its ok, but wanted to double check. we could also just change ok = to _ =...

seancribbs
seancribbs Feb 21, 2014 Contributor

Indeed, my main reason for introducing this is to raise the questions:

  • Should the process continue if the put fails? Will this lead to incorrect results?
  • If it is safe to ignore error conditions, why?
jtuple
jtuple Feb 27, 2014 Contributor

I'm generally against changing behavior that works today and has been in production for a year. So, I'm a bit torn.

In any case, AAE is designed to be lossy. Dropping updates/etc is no big deal. However, if I can't put to a LevelDB instance, something's seriously wrong. I imagine future attempts to iterate over the LevelDB instance (eg. for an exchange) would fail as well. Hard to imagine a scenario where eleveldb:put fails, but other operations against the tree work just fine. So, this is seems to be an error we might as well fail against.

If it somehow ends up breaks things in production, it's a 1 line change in a 2.0.1, right? ;)

reiddraper
reiddraper Feb 27, 2014 Contributor

We can always pattern match on the output, or put it in a try/catch too.

seancribbs
seancribbs Feb 27, 2014 Contributor

I'm happy to switch it to a case clause and have it spew the error to the logs.

@jrwest

are we sure that stop_proxy/2 won't be called multiple times for the same Mod/Index pair? If so, I could see the match here being a problem. Since the goal here is to ensure the child is terminated and deleted, if either function returns {error, not_found} isn't that, also, sufficient?

Contributor

Fixed in a5c2443

@jrwest

i wonder if it wouldn't be safer to just use _ = ... here...since gen.erl isn't an official API, and using it is a bit iffy. We could run into some fun debugging sessions when upgrading to a newer version of Erlang...or maybe that's useful? shrug

Contributor

Maybe, but honestly this hasn't changed since at least R13B03, which may have only been when they imported OTP into git. I doubt such a fundamental and pervasive API in stdlib (even if unofficial) would change frequently.

Contributor

while it hasn't been changed the OTP team doesn't document internal modules that may change at anytime without notice: http://erlang.2086793.n4.nabble.com/gen-module-documentation-availability-td2111296.html (see third reply). The same can be said for prim_inet fwiw.

@jrwest

similar comment here regarding multiple calls and potential missing child specs. should we _ = here too?

Contributor

Fixed in a5c2443

@jrwest
Contributor

Fixed in a5c2443

@jrwest jrwest commented on an outdated diff Feb 21, 2014
src/riak_core_ring.erl
@@ -1095,15 +1095,10 @@ complete_resize_transfers(State, Source, Mod) ->
deletion_complete(State, Idx, Mod) ->
transfer_complete(State, Idx, Mod).
--spec resize_transfers(chstate(), {integer(), term()}) ->
jrwest
jrwest Feb 21, 2014 Contributor

i'd rather see these specs corrected if they are wrong. as someone who wrote these functions and forgets what they return regularly the spec, as stands, is extremely helpful.

Contributor
jrwest commented Feb 21, 2014

i'd like to see 2de80a1 reverted. Most of these functions are exported and many of those specs are useful. Some of them should be modified to not include the variable names for brevity.

@jrwest jrwest commented on the diff Feb 21, 2014
src/riak_core_security.erl
@@ -409,12 +410,7 @@ add_revoke(all, Bucket, Revokes) ->
%% all is always valid
case validate_permissions(Revokes) of
ok ->
- case add_revoke_int([all], Bucket, revokes) of
jrwest
jrwest Feb 21, 2014 Contributor

these changes are no longer valid i don't think. perhaps one more rebase?

Contributor

Reverting 2de80a1 introduces some more warnings, will fix.

riak_core_capability.erl:412: Invalid type specification for function riak_core_capability:preferred_modes/4. The success typing is ([any()],[{atom(),[any()]}],[{atom(),{_,_,_,_}}],_) -> [{{_,_},_}]
riak_core_stat_cache.erl:87: The created fun has no local return
riak_core_stat_cache.erl:88: The call riak_core_stat_cache:register_mod(App::any(),{{_,'produce_stats',[]},non_neg_integer()},Registered::any()) breaks the contract (atom(),registered_app(),orddict:orddict()) -> orddict:orddict()
riak_core_vnode_manager.erl:371: Expression produces a value of type pid() | [pid()], but this value is unmatched
riak_core_vnode_manager.erl:854: Expression produces a value of type pid() | [pid()], but this value is unmatched
Unknown functions:
  cluster_info:format/3
  cluster_info:register_app/1
Unknown types:
  ordsets:ordset/0
Contributor

So I was unable to rebase cleanly again with the security changes, but I just copied the riak_core_security module over from develop. After reinstating some of my previous changes, here's a warning I can't untangle, which seems to have been introduced in the separation of roles/groups and users:

riak_core_security.erl:489: The call riak_core_security:add_grant_int(['all'],Bucket::any(),Grants::maybe_improper_list()) will never return since it differs in the 1st argument from the success typing arguments: ([{_,'group' | 'user'}],any(),maybe_improper_list())
riak_core_security.erl:516: The call riak_core_security:add_revoke_int(['all'],Bucket::any(),'revokes') will never return since it differs in the 1st argument from the success typing arguments: ([{_,'group' | 'user'}],any(),'revokes' | maybe_improper_list())

Essentially, calling add_grant_int/3 or add_revoke_int/3 with [all] as the first argument will never match. Is this code path even valid?

/cc @Vagabond @macintux

Member

Huh. I kept focusing on users & groups and forgot to watch carefully what happened to ‘all’, and a quick experiment does lead me to believe I screwed that up nicely.

-John

On Feb 22, 2014, at 12:18 PM, Sean Cribbs notifications@github.com wrote:

So I was unable to rebase cleanly again with the security changes, but I just copied the riak_core_security module over from develop. After reinstating some of my previous changes, here's a warning I can't untangle, which seems to have been introduced in the separation of roles/groups and users:

riak_core_security.erl:489: The call riak_core_security:add_grant_int(['all'],Bucket::any(),Grants::maybe_improper_list()) will never return since it differs in the 1st argument from the success typing arguments: ([{,'group' | 'user'}],any(),maybe_improper_list())
riak_core_security.erl:516: The call riak_core_security:add_revoke_int(['all'],Bucket::any(),'revokes') will never return since it differs in the 1st argument from the success typing arguments: ([{
,'group' | 'user'}],any(),'revokes' | maybe_improper_list())
Essentially, calling add_grant_int/3 or add_revoke_int/3 with [all] as the first argument will never match. Is this code path even valid?

/cc @Vagabond @macintux


Reply to this email directly or view it on GitHub.

Contributor

There seems to be a race or something in this test. From buildbot:

  riak_core_vnode_proxy: overload_test_ (should not discard in normal operation)...*failed*
in function riak_core_vnode_proxy:'-overload_test_/0-fun-5-'/2 (src/riak_core_vnode_proxy.erl, line 368)
**error:{assertEqual_failed,[{module,riak_core_vnode_proxy},
                     {line,368},
                     {expression,"Count"},
                     {expected,50001},
                     {value,47842}]}
Contributor

Yep. We definitely discussed removing that since it's covered by r_t. No
idea why it fails in particular locally. After 2-3 weeks of not touching
it, I think I'm just going to remove it.

On Sat, Feb 22, 2014 at 2:03 PM, Sean Cribbs notifications@github.comwrote:

There seems to be a race or something in this testhttps://github.com/basho/riak_core/blob/bugfix/realtalk/dialyze/src/riak_core_vnode_proxy.erl#L357-L372.
From buildbot:

riak_core_vnode_proxy: overload_test_ (should not discard in normal operation)...failed
in function riak_core_vnode_proxy:'-overload_test_/0-fun-5-'/2 (src/riak_core_vnode_proxy.erl, line 368)
**error:{assertEqual_failed,[{module,riak_core_vnode_proxy},
{line,368},
{expression,"Count"},
{expected,50001},
{value,47842}]}

Reply to this email directly or view it on GitHubhttps://github.com/basho/riak_core/pull/528#issuecomment-35811112
.

@jrwest jrwest commented on the diff Feb 27, 2014
src/riak_core_dtrace.erl
@@ -127,12 +127,7 @@ put_tag(Tag) ->
true ->
FTag = iolist_to_binary(Tag),
put(?DTRACE_TAG_KEY, FTag),
- case get(?MAGIC) of
- dtrace ->
- dtrace:put_utag(FTag);
jrwest
jrwest Feb 27, 2014 Contributor

this change seems to be removing some functionality? put_utag vs. put_tag?

seancribbs
seancribbs Feb 27, 2014 Contributor

dtrace module was from Scott's patch before it was fixed-up/merged into OTP. There is no such module anymore.

@jrwest jrwest commented on the diff Feb 27, 2014
src/gen_nb_server.erl
@@ -111,7 +125,7 @@ handle_info({inet_async, ListSock, _Ref, {ok, CliSocket}}, #state{cb=Callback, s
inet_db:register_socket(CliSocket, inet_tcp),
case Callback:new_connection(CliSocket, ServerState) of
{ok, NewServerState} ->
- prim_inet:async_accept(ListSock, -1),
+ {ok, _} = prim_inet:async_accept(ListSock, -1),
jrwest
jrwest Feb 27, 2014 Contributor

I'm not sure it's ok to crash here if async_accept/2 returns {error, _}.

note: this behaviour is used by riak_core_handoff_listener.

jrwest
jrwest Feb 27, 2014 Contributor

if it does crash, the handoff listener, for example, would be restarted and all would be fine. but previously we ignored the error, so just want to make sure this semantic change is ok.

seancribbs
seancribbs Feb 27, 2014 Contributor

I do remember looking into prim_inet, I did not see any reason why an async accept would not return {ok, integer()}, but dialyzer complained because the integer() would vary.

@jrwest jrwest commented on an outdated diff Feb 27, 2014
src/riak_core.erl
@@ -81,15 +81,10 @@ join(false, _, Node, Rejoin, Auto) ->
pang ->
{error, not_reachable};
pong ->
- case false of
- true ->
- legacy_join(Node);
- _ ->
- %% Failure due to trying to join older node that
- %% doesn't define legacy_gossip will be handled
- %% in standard_join based on seeing a legacy ring.
- standard_join(Node, Rejoin, Auto)
- end
+ %% Failure due to trying to join older node that
jrwest
jrwest Feb 27, 2014 Contributor

if we remove all of legacy gossip this comment no longer applies

@jrwest jrwest and 1 other commented on an outdated diff Feb 27, 2014
src/riak_core_apl.erl
@@ -42,16 +42,17 @@
-type preflist2() :: [{{index(), node()}, primary|fallback}].
-type iterator() :: term().
-type chashbin() :: term().
+-type docidx() :: <<_:160>>.
jrwest
jrwest Feb 27, 2014 Contributor

we have a lot of types defined for this same thing. one, less accurate example, https://github.com/basho/riak_core/blob/develop/src/chash.erl#L68

seancribbs
seancribbs Feb 27, 2014 Contributor

Addressed in ceec2d9

@jrwest jrwest and 1 other commented on an outdated diff Feb 27, 2014
src/riak_core_bg_manager.erl
@@ -403,10 +403,10 @@ query_resource(Resource, Types) ->
%%%
-record(state,
- {info_table:: ets:tid(), %% TableID of ?BG_INFO_ETS_TABLE
- entry_table:: ets:tid(), %% TableID of ?BG_ENTRY_ETS_TABLE
- enabled :: boolean(), %% Global enable/disable switch, true at startup
- bypassed:: boolean() %% Global kill switch. false at startup
+ {info_table:: ets:tid() | atom(), %% TableID of ?BG_INFO_ETS_TABLE
jrwest
jrwest Feb 27, 2014 Contributor

iirc ets:tab() is equivalent to ets:tid() | atom()

seancribbs
seancribbs Feb 27, 2014 Contributor

Addressed in e320072

@jrwest jrwest and 1 other commented on an outdated diff Feb 27, 2014
src/riak_core_gossip_legacy.erl
@@ -153,7 +153,8 @@ code_change(_OldVsn, State, _Extra) ->
schedule_next_gossip() ->
jrwest
jrwest Feb 27, 2014 Contributor

when legacy gossip is gone maybe this file can be removed?

seancribbs
seancribbs Feb 27, 2014 Contributor

Indeed.

@jrwest jrwest commented on the diff Feb 27, 2014
src/riak_core_node_watcher.erl
@@ -325,8 +325,7 @@ handle_info({'DOWN', Mref, _, _Pid, _Info}, State) ->
%% Update our list of active services and ETS table
Services = ordsets:del_element(Id, State#state.services),
- S3 = S2#state { services = Services },
- local_update(S3),
+ S3 = local_update(S2#state { services = Services }),
jrwest
jrwest Feb 27, 2014 Contributor

I think there is a case here where we can lose the correct state because local_update/1 may return ok [1]

[1] https://github.com/basho/riak_core/blob/bugfix/realtalk/dialyze/src/riak_core_node_watcher.erl#L487-L490

seancribbs
seancribbs Feb 27, 2014 Contributor

Notice that broadcast is called after the case statement in that function.

jrwest
jrwest Feb 27, 2014 Contributor

ah, yes. my mistake

evanmcc
evanmcc Feb 27, 2014 Contributor

Was just looking at the top of the thread, and wanted to note that you can
add -compile(inline_list_funcs). to the top to get the LC optimizations
added to standard lists module calls.

seancribbs
seancribbs Feb 27, 2014 Contributor

@evanmcc Interesting. Should we be doing that globally?

seancribbs
seancribbs Feb 27, 2014 Contributor

Also, the other argument for LCs even when the return value is not used is clarity/visual noise reduction.

evanmcc
evanmcc Feb 27, 2014 Contributor

I don't know. It's something that I'd need to test. Maybe we can enable
it at the start of the next dev cycle so we have a long time to hopefully
run into any issues.

@jrwest jrwest and 1 other commented on an outdated diff Feb 28, 2014
src/riak_core_apl.erl
@@ -35,23 +35,24 @@
-include_lib("eunit/include/eunit.hrl").
-endif.
--type index() :: non_neg_integer().
+-type index() :: chash:index_as_int().
-type n_val() :: non_neg_integer().
-type ring() :: riak_core_ring:riak_core_ring().
-type preflist() :: [{index(), node()}].
-type preflist2() :: [{{index(), node()}, primary|fallback}].
jrwest
jrwest Feb 28, 2014 Contributor

we're probably nearing the point where we should try to stop making new changes, but couldn't help but comment that i really wish this were something like preflist_ann() or annotated_preflist() rather than preflist2().

seancribbs
seancribbs Feb 28, 2014 Contributor

That makes sense, will fix.

Sean Cribbs

On Feb 27, 2014, at 7:06 PM, Jordan West notifications@github.com wrote:

In src/riak_core_apl.erl:

@@ -35,23 +35,24 @@
-include_lib("eunit/include/eunit.hrl").
-endif.

--type index() :: non_neg_integer().
+-type index() :: chash:index_as_int().
-type n_val() :: non_neg_integer().
-type ring() :: riak_core_ring:riak_core_ring().
-type preflist() :: [{index(), node()}].
-type preflist2() :: [{{index(), node()}, primary|fallback}].
we're probably nearing the point where we should try to stop making new changes, but couldn't help but comment that i really wish this were something like preflist_ann() or annotated_preflist() rather than preflist2().


Reply to this email directly or view it on GitHub.

seancribbs
seancribbs Feb 28, 2014 Contributor

Addressed in 00086a7

@jrwest jrwest commented on the diff Feb 28, 2014
src/riak_core_ring.erl
%% ===================================================================
%% Public API
%% ===================================================================
-%% @doc Returns true if the given ring is a legacy ring.
-legacy_ring(#chstate{}) ->
- true;
-legacy_ring(_) ->
- false.
-
%% @doc Upgrade old ring structures to the latest format.
jrwest
jrwest Feb 28, 2014 Contributor

it might be worth adding a comment to upgrade/1 and downgrade/1 as to why the functions do basically nothing. long term it would probably be nice to remove them (as any future upgrade/downgrade may work differently)

@jrwest jrwest commented on the diff Feb 28, 2014
src/riak_core_ring_manager.erl
@@ -625,8 +618,8 @@ prune_write_notify_ring(Ring, State) ->
prune_write_ring(Ring, State) ->
riak_core_ring:check_tainted(Ring, "Error: Persisting tainted ring"),
- riak_core_ring_manager:prune_ringfiles(),
- do_write_ringfile(Ring),
+ ok = riak_core_ring_manager:prune_ringfiles(),
+ _ = do_write_ringfile(Ring),
jrwest
jrwest Feb 28, 2014 Contributor

nitpick note: above we match on ok = and here we do not. seems like in this case its equally reasonable to crash.

@jrwest jrwest commented on the diff Feb 28, 2014
src/riak_core_vnode.erl
@@ -375,6 +376,11 @@ vnode_handoff_command(Sender, Request, ForwardTo,
riak_core_vnode_worker_pool:handle_work(Pool, Work, From),
continue(State, NewModState);
{forward, NewModState} ->
+ %% FIXME: vnode_forward returns a gen_fsm state return
jrwest
jrwest Feb 28, 2014 Contributor

i believe this was discussed and addressed. does this comment still apply?

Contributor
jrwest commented Feb 28, 2014

seeing some issues running verify_dynamic_ring against this branch. lots of log messages like the one below. investigating...

20:20:07.354 [error] Error: riak_core_ring/ring_ready called on tainted ring

update 1:

same issue with verify_build_cluster, in this case, the log messages are in the riak logs only.

update 2:

I think i've isolated the issue to some of the recent legacy gossip removal changes. the errors do not appear when rolling back to a5c2443

Contributor

Closing this in favor of a PR that excludes:

  • Changes to riak_core_security, since that changed several times during the course of this work.
  • Removal of anything related to legacy gossip, which should be separate.
@seancribbs seancribbs closed this Feb 28, 2014
This was referenced Feb 28, 2014
@seancribbs seancribbs added a commit that referenced this pull request Mar 1, 2014
@seancribbs seancribbs Port dialyzer warning fixes from #528. 3d80525
@seancribbs seancribbs deleted the bugfix/realtalk/dialyze branch Mar 1, 2014
@jrwest jrwest referenced this pull request Mar 24, 2014
Closed

Fix bad specification. #283

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