aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Chu-Carroll <mchucarr...@apache.org>
Subject Re: Review Request 25255: Implement server-driven update commands.
Date Mon, 15 Sep 2014 22:11:28 GMT
As I said - the version with the sub-actions fouls up parameter checking.
(That's the "action" parameter option. It doesn't matter whether it's
"--action=start" or "--start"; in fact, the --action at least lets the
parameter parser do a little bit of checking.)

It's possible to add another layer of dispatch to the framework, but it's a
fair bit of work. But that's not a problem. The problem there is that for
users, "noun/verb" is a very natural idea. When you start going to extra
layers of dispatch, the complexity of the command to users changes pretty
significantly. It's no longer "what do I want to interact with, and what do
I want to do to it", but "How many layers of commands do I need for this?"

I think that adding layers changes the cognitive load of working with the
system in a very unfortunate way.

I still think that of the options, the "update" noun is still the best.

The distinction between nouns is always going to be somewhat arbitrary -
why is "ssh" part of task instead of job? Why is quota a noun and user a
parameter, instead of user a noun and quota a parameter?

There is a sensible distinction in update. The fundamental question behind
nouns is always "what kind of thing do I want to interact with?" And
there's a reasonable answer: "an update process on a server". It can be
explained to users in the one-line usage message, and it's easy to remember.






On Mon, Sep 15, 2014 at 5:16 PM, Maxim Khutornenko <maxim@apache.org> wrote:

> I guess there is also the option you originally proposed: "aurora job
> update --start|--pause|--resume|--abort".
>
> If you don't like the option syntax, would it be too much work to
> support double "verb" (or rather double "noun") syntax, like "aurora
> job update start"?
>
> On Mon, Sep 15, 2014 at 2:07 PM, Mark Chu-Carroll
> <mchucarroll@apache.org> wrote:
> > The choices are:
> >
> > (1) Action parameter: "aurora job update
> --action=start/pause/resume/abort".
> > Pros: it's part of the "job" noun and the familiar "update" verb.
> > Cons:
> > - the action parameter is ugly,
> > - we lose automatic parameter checking, since the different actions take
> > different parameters.
> > - it's difficult to prevent it from interfering with existing users while
> > the server-driven update is under
> >   development and not working. (I see this as an extremely serious
> blocking
> > problem.)
> >
> > (2) Multiple verbs on job: "aurora job update_start", "aurora job
> > update_pause", ...
> > Pros:
> > - it's part of the "job" noun,
> > - parameter checking works.
> > - it's reasonably easy to keep this separated so that it doesn't show up
> > until we're ready.
> > Cons:
> > - it's hideous.
> >
> > (3) Update noun: "aurora update start/pause/resume/abort".
> > Pros:
> > - parameter checking works.
> > - easy to keep under wraps without annoying users.
> > - (arguable) I think it makes sense, because it represents a command that
> > interacts with a different service.
> > Cons:
> > - It's not part of the job noun.
> >
> >
> > I strongly prefer option 3. But I don't want to waste time bikesheding.
> So
> > if you guys strongly disagree with me, fine, just tell me which one
> you're
> > willing to agree on.
> >
> >         -Mark
> >
> > On Mon, Sep 15, 2014 at 2:28 PM, Bill Farner <wfarner@apache.org> wrote:
> >>
> >> In the world of 'nouns' and 'verbs', i imagine a user would think "i
> want
> >> to update my job", not "i want to start an update".  So if the goal of
> >> organizing commands by nouns is to make it intuitive, i think "update
> start"
> >> is less natural than "job update".
> >>
> >> -=Bill
> >>
> >> On Mon, Sep 15, 2014 at 10:25 AM, Mark Chu-Carroll
> >> <mchucarroll@twopensource.com> wrote:
> >>>
> >>>
> >>>
> >>> > On Sept. 15, 2014, 12:04 p.m., Maxim Khutornenko wrote:
> >>> > > src/main/python/apache/aurora/client/cli/standalone_client.py,
line
> >>> > > 121
> >>> > >
> >>> > > <
> https://reviews.apache.org/r/25255/diff/2/?file=685554#file685554line121>
> >>> > >
> >>> > >     I am still not sure why we are going with the top level noun
> for
> >>> > > job updates. It's quite strange to see create/restart under "job"
> but
> >>> > > "update" living on its own. Is this only to avoid collision with
> the
> >>> > > existing client update? If so, it's only there until we iron out
> the async
> >>> > > updates. We could clearly mark new server side update options
as
> "BETA" in
> >>> > > command help to avoid any confusion. I think the long term
> consistency here
> >>> > > is more important than temporary ambiguity.
> >>>
> >>> I thought that it made sense to be a separate noun.
> >>>
> >>> There's two main reasons why I think it makes sense for this to be a
> >>> distinct noun.
> >>>
> >>> (1) All of the "job" commands on the client are client-driven logic.
> This
> >>> isn't. This is server-side logic,
> >>>   but not truly scheduler logic. It's creating an update process on a
> >>> server somewhere, and then allowing
> >>>   you to interact with it. You're not using these commands to interact
> >>> with a job - you're interacting with
> >>>   an update process.  You're interacting with a different kind of
> >>> *thing*, and each kind of thing is
> >>>   handled as a different noun.  (Think about the "task" noun. A task
> is a
> >>> piece of a job, but we handle
> >>>   it differently. We still use jobkeys to specify them - but for
> >>> operations that work on the running
> >>>   task in a mesos slave, we put them in a different noun than the
> >>> operations that talk to the scheduler
> >>>   about the job itself. I think this is a similar case.)
> >>>
> >>> (2) There's a collection of sub-commands, and the subcommands are
> >>> behaviorally very much the kinds of
> >>>   operations you find on nouns elsewhere in the client. Putting all of
> >>> this under the "update" verb
> >>>   on jobs results in a lot of ugliness - you end up with the "action"
> >>> parameter from the earlier version
> >>>   of this review, and you lose hte ability to have the option parser do
> >>> parameter checking for the
> >>>   different actions. You end up with a better syntax, and clearer
> >>> semantics when you treat start/pause/resume/abort/status as verbs on an
> >>> update noun.
> >>>
> >>> Finally, I very strongly disagree that it's OK to introduce the
> >>> server-driven updates as options to the existing "update" command.
> >>> Disrupting users with unsupported, non-working test functionality is
> *not*
> >>> acceptable in a production tool.
> >>>
> >>> One fringe benefit of this approach is that it's really easy to leave
> it
> >>> out of the production executable until it's ready to go.
> >>>
> >>>        -Mark
> >>>
> >>>
> >>> - Mark
> >>>
> >>>
> >>> -----------------------------------------------------------
> >>> This is an automatically generated e-mail. To reply, visit:
> >>> https://reviews.apache.org/r/25255/#review53347
> >>> -----------------------------------------------------------
> >>>
> >>>
> >>> On Sept. 11, 2014, 10:55 a.m., Mark Chu-Carroll wrote:
> >>> >
> >>> > -----------------------------------------------------------
> >>> > This is an automatically generated e-mail. To reply, visit:
> >>> > https://reviews.apache.org/r/25255/
> >>> > -----------------------------------------------------------
> >>> >
> >>> > (Updated Sept. 11, 2014, 10:55 a.m.)
> >>> >
> >>> >
> >>> > Review request for Aurora.
> >>> >
> >>> >
> >>> > Repository: aurora
> >>> >
> >>> >
> >>> > Description
> >>> > -------
> >>> >
> >>> > This change contains the basic commands needed to work with the
> >>> > scheduler-driven updater. (It does not yet cover querying for the
> >>> > status
> >>> > of the update; that will come in a subsequent change.)
> >>> >
> >>> >
> >>> > Diffs
> >>> > -----
> >>> >
> >>> >   src/main/python/apache/aurora/client/cli/BUILD
> >>> > ebe681a0d1735b7cc695dc3b7a14c4292d87ae32
> >>> >   src/main/python/apache/aurora/client/cli/context.py
> >>> > 51c7d24dca664e476e62f1864d095416dfab70e4
> >>> >   src/main/python/apache/aurora/client/cli/jobs.py
> >>> > ebc22aaa5a8aed311897b3ce9632b6f7175b6080
> >>> >   src/main/python/apache/aurora/client/cli/standalone_client.py
> >>> > b7c8de66d6e4664b536911f826e36a984e8d0fef
> >>> >   src/main/python/apache/aurora/client/cli/update.py PRE-CREATION
> >>> >   src/test/python/apache/aurora/client/cli/BUILD
> >>> > e1f9ebf96774b8f5c75de8570c6ba87d953ab649
> >>> >   src/test/python/apache/aurora/client/cli/test_restart.py
> >>> > a1e7a5a94a2d336239df98e2600658b97c546901
> >>> >   src/test/python/apache/aurora/client/cli/test_supdate.py
> PRE-CREATION
> >>> >   src/test/python/apache/aurora/client/cli/util.py
> >>> > 95a2123e127c9811fd2305e71cfc5c7c4376f904
> >>> >
> >>> > Diff: https://reviews.apache.org/r/25255/diff/
> >>> >
> >>> >
> >>> > Testing
> >>> > -------
> >>> >
> >>> > New suite of tests for the new command; all unit tests pass.
> >>> >
> >>> >
> >>> > Thanks,
> >>> >
> >>> > Mark Chu-Carroll
> >>> >
> >>> >
> >>>
> >>
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message