aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Mark Chu-Carroll" <mchucarr...@twopensource.com>
Subject Re: Review Request 25255: Implement server-driven update commands.
Date Mon, 15 Sep 2014 17:25:36 GMT


> 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