Implement cluster CLI commands #659
Conversation
Remove all usage of direct io:format in favor of the riak_cli status types. This will hopefully simplify output.
| T2 = riak_cli_status:text( | ||
| "Key: (C) = Claimant; availability marked with '!' is unexpected"), | ||
| Out = riak_cli_status:alert([T0,T1,Table,T2]), | ||
| riak_cli:print([Out]). |
There was a problem hiding this comment.
I've been thinking about this a bit. I didn't really intend for implementers to constantly call riak_cli:print/1. I intended for it to be called automatically and use the proper writer module given global --csv flags etc..., but of course I didn't implement it that way. I spent a lot of time focusing on config the last few weeks and if you look in riak_cli.erl you'll see that the config ops (show, describe, set) wrap the output in a print statement, and the callbacks themselves return errors tuples or status types. Returning errors works fine for config because it's all internal and riak_cli_error knows how to parse the riak_cli specific errors and generate alerts from them. However, since that requires modifying riak_cli itself, outside users of riak_cli should only ever return status types in callbacks.
This was a mistake in implementation that I will fix on Monday. We can then modify each PR to do the right thing and only return a status(). In your case it will just be removing the riak_cli:print/1 calls. Hopefully the same elsewhere.
There was a problem hiding this comment.
OK, gotcha. Now that basho/clique#14 has been merged, I will fix it up.
After basho/clique#14 was merged, callbacks are expected to implement riak_cli_status types as their return values.
|
@andrewjstone I've pushed a new commit responding to your code review. |
| {pending, future_claim_percentage(Changes, Ring, Node)} | ||
| ]. | ||
|
|
||
| is_claimant(Node, Node) -> |
There was a problem hiding this comment.
This is totally a bikeshed comment and won't hold up a +1 of this PR. I was just thinking that we should align this better. Either put the (c) after the Node name, since it's left aligned, or put 3 spaces before the non-claimant nodes so it is right - aligned.
|
Ping @andrewjstone |
| node_availability(Node, Down, MarkedDown) -> | ||
| case {lists:member(Node, Down), lists:member(Node, MarkedDown)} of | ||
| {false, false} -> " up "; % put common case at the top | ||
| {true, true} -> " down "; |
There was a problem hiding this comment.
Can you line this clause up please?
|
@andrewjstone think 3e28706 is a winner :) |
|
I'm seeing the following behavior, but I believe it should be fixed in riak_cli. In this case |
Implement cluster CLI commands Reviewed-by: andrewjstone
|
@borshop merge |
Implement the following commands using riak_cli:
TODO: After merge, some code clean up and boundary checking.
Depends on basho/riak_ee#295