Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(99)

Issue 99670044: Add output for ensure-availability command.

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 6 months ago by domas
Modified:
1 week, 5 days ago
Reviewers:
mp+221549, fwereade, rog
CC:
Nate Finch
Visibility:
Public.

Description

Add output for ensure-availability command. Add output for ensure-availability command. The ensure-availability command will notify the user, how many state servers were added to the environment. $ juju ensure-availability -n 7 maintaining machines: "0" adding machines: "1", "2", "3", "4", "5", "6" https://code.launchpad.net/~tasdomas/juju-core/ensure-availability-output/+merge/221549 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : Add output for ensure-availability command. #

Total comments: 13

Patch Set 3 : Add output for ensure-availability command. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+319 lines, -43 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/ensureavailability.go View 1 2 5 chunks +67 lines, -2 lines 0 comments Download
M cmd/juju/ensureavailability_test.go View 1 2 7 chunks +76 lines, -14 lines 0 comments Download
M state/api/client.go View 1 2 1 chunk +5 lines, -3 lines 0 comments Download
M state/api/params/params.go View 1 2 1 chunk +13 lines, -4 lines 0 comments Download
M state/apiserver/client/client.go View 1 2 2 chunks +36 lines, -10 lines 1 comment Download
M state/apiserver/client/client_test.go View 1 2 7 chunks +119 lines, -10 lines 0 comments Download
M state/apiserver/client/export_test.go View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 9
domas
Please take a look.
8 years, 6 months ago (2014-05-30 14:12:51 UTC) #1
domas
Please take a look.
8 years, 6 months ago (2014-05-30 14:20:16 UTC) #2
domas
On 2014/05/30 14:12:51, domas wrote: > Please take a look. This is a rewrite of ...
8 years, 6 months ago (2014-05-30 14:34:32 UTC) #3
fwereade
Definitely do the formatters, but you might want to hold off on hitting the API ...
8 years, 6 months ago (2014-06-02 09:50:04 UTC) #4
domas
Please take a look.
8 years, 6 months ago (2014-06-02 17:07:01 UTC) #5
domas
Thanks for taking the time to review this. > https://codereview.appspot.com/99670044/diff/20001/cmd/juju/ensureavailability.go#newcode91 > cmd/juju/ensureavailability.go:91: } > I ...
8 years, 6 months ago (2014-06-02 17:12:29 UTC) #6
rog
This looks fine for what it's trying to do, thanks, but I think we should ...
8 years, 6 months ago (2014-06-03 07:55:50 UTC) #7
domas
On 2014/06/03 07:55:50, rog wrote: > This looks fine for what it's trying to do, ...
8 years, 6 months ago (2014-06-03 08:30:34 UTC) #8
rog
8 years, 6 months ago (2014-06-03 11:06:25 UTC) #9
On 3 June 2014 09:30,  <domas.monkus@canonical.com> wrote:
> On 2014/06/03 07:55:50, rog wrote:
>>
>> This looks fine for what it's trying to do, thanks, but I think we
>
> should get
>>
>> state.EnsureAvailability to report what it's done directly, rather
>
> than
>>
>> inferring it by looking at the resulting state servers. And one more
>
> suggestion
>>
>> below.
>
> Me and Matthew had implemented it this way initially, but were advised
> to pursue this approach instead.

From the review, it looks to me as if the main thing was not that
we couldn't use results from the EnsureAvailability call, but just
that we don't use the results type *directly* from the EnsureAvailability
call in the API.

But I did miss this:

: or, I think better, leave this method alone and just figure
: out what changed in the course of the call by diffing two
: what-are-the-state-servers calls.

I still think this is not a great approach. For one thing, it doesn't
actually say what that EnsureAvailability call did - it just finds
out what has happened  recently. So if two people do an EnsureAvailability
call at the same moment, the results may be confusing and
hard to understand, because they may pertain to the logic
run twice.

Another reason I'm not keen on it is that it second-guesses the
EnsureAvailability logic - it means that the API server
has to know everything that the underlying call
might have done, inferring it from before-and-after state.

We have the info, but we then throw it away and try to reconstruct it.

FWIW the initial implementation looked fine to me. With
respect to the push-back, the params package
is specifically about types that are used in the API, so you *know*
if you're changing a type in state/api/params that you will be changing
API call parameters. And the state.EnsureAvailability method is just
there in the service of the api server, so adding an extra layer
seems to me unnecessary and counterproductive. I appreciate
that others have different views on this though.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b