Re: Review Request 25255: Implement server-driven update commands.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25255/ --- (Updated Sept. 17, 2014, 1:06 p.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
Re: Review Request 25255: Implement server-driven update commands.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25255/ --- (Updated Sept. 17, 2014, 2:44 p.m.) Review request for Aurora. Changes --- rebase to master. 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 (updated) - src/main/python/apache/aurora/client/cli/BUILD 4ed8f8a3e29010282c5cb0cc461980bcfc6ae478 src/main/python/apache/aurora/client/cli/context.py 51c7d24dca664e476e62f1864d095416dfab70e4 src/main/python/apache/aurora/client/cli/jobs.py aeb78303fc2071ccad848b1de4512e8ca585bf06 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 70e704d2a3c334eaa8ba905eb3c75c2e9ee152bc src/test/python/apache/aurora/client/cli/test_restart.py 377ecfe6180785e57a389f6e1bc8b184bca0dd6c src/test/python/apache/aurora/client/cli/test_supdate.py PRE-CREATION src/test/python/apache/aurora/client/cli/util.py ebabe529966cea503f11404f37961c5d577f00b7 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
Re: Review Request 25255: Implement server-driven update commands.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25255/#review53554 --- Ship it! Mark informed me that he intends for this command to not be exposed until we have all the required code in place. With that assumption, and the assumption that we are not bound to these nouns/verbs long-term, i'm willing to skip the naming discussion. - Bill Farner On Sept. 11, 2014, 2:55 p.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, 2:55 p.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
Re: Review Request 25255: Implement server-driven update commands.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25255/#review53347 --- src/main/python/apache/aurora/client/cli/standalone_client.py https://reviews.apache.org/r/25255/#comment92980 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. - Maxim Khutornenko On Sept. 11, 2014, 2:55 p.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, 2:55 p.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
Re: Review Request 25255: Implement server-driven update commands.
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
Re: Review Request 25255: Implement server-driven update commands.
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 wfar...@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 mchucarr...@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.
Re: Review Request 25255: Implement server-driven update commands.
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 mchucarr...@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 wfar...@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 mchucarr...@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:
Re: Review Request 25255: Implement server-driven update commands.
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 ma...@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 mchucarr...@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 wfar...@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 mchucarr...@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
Re: Review Request 25255: Implement server-driven update commands.
On Sept. 3, 2014, 3:58 p.m., Maxim Khutornenko wrote: src/main/python/apache/aurora/client/cli/jobs.py, line 667 https://reviews.apache.org/r/25255/diff/1/?file=673959#file673959line667 How about a BROWSER_OPTION for all update commands (start/pause/resume/abort)? It will, eventually, but we don't have a set URL for it yet. That can be added when the UI is more locked down. On Sept. 3, 2014, 3:58 p.m., Maxim Khutornenko wrote: src/main/python/apache/aurora/client/cli/jobs.py, lines 704-706 https://reviews.apache.org/r/25255/diff/1/?file=673959#file673959line704 Is this a result of sharing the verb definition with start_update? Any chance to avoid sharing the option set here? With the way that the noun/verb framework works right now, no, there's not really any good way. There are three choices: (1) Have an action parameter as a selector (what this change does); (2) Have a collection of verbs, update_start, update_pause, update_resume, update_abort. (3) Add a noun for an in-progress update, in which case the commands become aurora update start, aurora update pause, etc. I really hate (2), and I've been going back and forth between (1) and (3). - Mark --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25255/#review52220 --- On Sept. 2, 2014, 12:36 p.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25255/ --- (Updated Sept. 2, 2014, 12:36 p.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/context.py 51c7d24dca664e476e62f1864d095416dfab70e4 src/main/python/apache/aurora/client/cli/jobs.py ebc22aaa5a8aed311897b3ce9632b6f7175b6080 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
Re: Review Request 25255: Implement server-driven update commands.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25255/#review53030 --- I was about to ping about this, when I discovered that my replies to the comments never got sent - guessing chrome timed out, and I didn't notice. Please don't re-review this yet - I'm reworking some of the code; I'll send an update when it's ready. - Mark Chu-Carroll On Sept. 2, 2014, 12:36 p.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25255/ --- (Updated Sept. 2, 2014, 12:36 p.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/context.py 51c7d24dca664e476e62f1864d095416dfab70e4 src/main/python/apache/aurora/client/cli/jobs.py ebc22aaa5a8aed311897b3ce9632b6f7175b6080 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
Re: Review Request 25255: Implement server-driven update commands.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25255/#review52220 --- src/main/python/apache/aurora/client/cli/jobs.py https://reviews.apache.org/r/25255/#comment90978 Why not reuse the existing update verb? Having supdate just to avoid naming collision with the existing verb that will go away soon does not seem well justified. src/main/python/apache/aurora/client/cli/jobs.py https://reviews.apache.org/r/25255/#comment90962 How about a BROWSER_OPTION for all update commands (start/pause/resume/abort)? src/main/python/apache/aurora/client/cli/jobs.py https://reviews.apache.org/r/25255/#comment90972 Just realized we are missing cron-check on the server side (https://reviews.apache.org/r/25311/). You might want to do a similar check here to short circuit server call and provide a custom error message hinting users to use aurora cron commands. src/main/python/apache/aurora/client/cli/jobs.py https://reviews.apache.org/r/25255/#comment90974 Is this a result of sharing the verb definition with start_update? Any chance to avoid sharing the option set here? - Maxim Khutornenko On Sept. 2, 2014, 4:36 p.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25255/ --- (Updated Sept. 2, 2014, 4:36 p.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/context.py 51c7d24dca664e476e62f1864d095416dfab70e4 src/main/python/apache/aurora/client/cli/jobs.py ebc22aaa5a8aed311897b3ce9632b6f7175b6080 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
Re: Review Request 25255: Implement server-driven update commands.
On Sept. 2, 2014, 1:32 p.m., Joshua Cohen wrote: Is the command name supdate up for debate? I'm not in love with it ;). It's a temporary thing. (Came up with it during a discussion with Bill.) If we start adding this stuff to the existing update command, then existing users are going to see things like pause, and resume that don't make any sense, and that are only related to a component/service that they can't use. We don't want to screw up or confuse users of the existing command during the development of the update service - so for that transition, we're keeping update/supdate separate. I'm not in love with supdate as the name, but I didn't want to waste time coming up with something better when it's just temporary anyway. On Sept. 2, 2014, 1:32 p.m., Joshua Cohen wrote: src/main/python/apache/aurora/client/cli/jobs.py, lines 664-665 https://reviews.apache.org/r/25255/diff/1/?file=673959#file673959line664 This doesn't read very well to me. Maybe something like: Update action to start an update or pause, resume or abort an in-progress update. I think that wording is worse... The help should suggest what the value of the parameter is. Update action to start an update or pause... blurs that. The argument specifies the action that the user wants, which can be update, pause, resume, or abort. - Mark --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25255/#review52043 --- On Sept. 2, 2014, 12:36 p.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25255/ --- (Updated Sept. 2, 2014, 12:36 p.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/context.py 51c7d24dca664e476e62f1864d095416dfab70e4 src/main/python/apache/aurora/client/cli/jobs.py ebc22aaa5a8aed311897b3ce9632b6f7175b6080 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
Review Request 25255: Implement server-driven update commands.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25255/ --- 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/context.py 51c7d24dca664e476e62f1864d095416dfab70e4 src/main/python/apache/aurora/client/cli/jobs.py ebc22aaa5a8aed311897b3ce9632b6f7175b6080 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
Re: Review Request 25255: Implement server-driven update commands.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25255/#review52043 --- Is the command name supdate up for debate? I'm not in love with it ;). src/main/python/apache/aurora/client/cli/jobs.py https://reviews.apache.org/r/25255/#comment90770 This doesn't read very well to me. Maybe something like: Update action to start an update or pause, resume or abort an in-progress update. src/main/python/apache/aurora/client/cli/jobs.py https://reviews.apache.org/r/25255/#comment90771 This general form (get cluster api, call api method, check response, report, exit) is repeated for each of the update related methods, the only bits changing are the method to call on the api and the messages that are printed. Consider refactoring to this logic is shared? src/test/python/apache/aurora/client/cli/test_supdate.py https://reviews.apache.org/r/25255/#comment90773 Similarly to the impl above, there's a lot of commonality between these success test cases that could be refactored to reduce boilerplate. - Joshua Cohen On Sept. 2, 2014, 4:36 p.m., Mark Chu-Carroll wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25255/ --- (Updated Sept. 2, 2014, 4:36 p.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/context.py 51c7d24dca664e476e62f1864d095416dfab70e4 src/main/python/apache/aurora/client/cli/jobs.py ebc22aaa5a8aed311897b3ce9632b6f7175b6080 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
