Mailing-List: contact reviews-help@aurora.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@aurora.incubator.apache.org
Content-Type: multipart/alternative;
 boundary="===============8287400698028504071=="
MIME-Version: 1.0
Subject: Re: Review Request 20521: Extend the client configuration plugin
 architecture.
From: "Maxim Khutornenko" <maxim@apache.org>
To: "David Robinson" <drobinson@twitter.com>,
 "Maxim Khutornenko" <maxim@apache.org>
Cc: "Aurora" <reviews@aurora.incubator.apache.org>,
 "Mark Chu-Carroll" <mchucarroll@twopensource.com>
Date: Mon, 21 Apr 2014 17:40:42 -0000
Message-ID: <20140421174042.19744.49108@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Maxim Khutornenko" <noreply@reviews.apache.org>
References: <20140421165309.19744.68427@reviews.apache.org>
In-Reply-To: <20140421165309.19744.68427@reviews.apache.org>
Reply-To: "Maxim Khutornenko" <maxim@apache.org>

--===============8287400698028504071==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


> On April 21, 2014, 4:53 p.m., Mark Chu-Carroll wrote:
> > src/main/python/apache/aurora/client/cli/__init__.py, line 337
> > <https://reviews.apache.org/r/20521/diff/1/?file=563067#file563067line337>
> >
> >     The pre-execution plugin takes the same command-context as the actual verb implementation. It behaves exactly as if it's part of the command implementation - same context, same error-signalling methods.
> >     
> >     The post-execution plugin method shouldn't be throwing exceptions at all. It's just a cleanup - that's why it isn't supposed to change the return code.
> >     
> >     Python being python, we can't enforce the fact that the post-execution method doesn't throw context.CommandException. But the intention is as it's written in the code: no attempt to catch any exception from it, because it's not supposed to throw any.
> >     
> >     Since it's not clear, I'll update the code documentation to reflect this.
> >
> 
> Maxim Khutornenko wrote:
>     Perhaps I missing something but is there anything preventing all plugins from raising a ConfigurationPlugin.Error instead of CommandError on hook validation failure? This would remove ambiguity on the error handling side and enable a plugin-specific error messaging/handling.
> 
> Mark Chu-Carroll wrote:
>     Preventing it? No.
>     
>     I just think that as a design point, it's clearer to say that the pre-execution method of a plugin runs as a part of the verb implementation, and so it signals errors in the same way. 
>     
>     The only reason that there's a plugin exception type at all is because the pre-dispatch method doesn't have a context object, and you need a context to throw a Context.CommandError.
>

"...runs as a part of the verb implementation..." - completely agree. 
"...signals errors in the same way..." - not sure what benefits this design choice actually brings. On the downside though, it makes it much harder to treat hook errors in a special way if needed after different causes are multiplexed and the original context is lost. It's always easier to treat two different errors the same way though if custom processing is not needed (yet).

I am curious what others think here.


- Maxim


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20521/#review40887
-----------------------------------------------------------


On April 21, 2014, 4:53 p.m., Mark Chu-Carroll wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20521/
> -----------------------------------------------------------
> 
> (Updated April 21, 2014, 4:53 p.m.)
> 
> 
> Review request for Aurora, David Robinson and Maxim Khutornenko.
> 
> 
> Bugs: aurora-332
>     https://issues.apache.org/jira/browse/aurora-332
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Extend the client configuration plugin architecture.
> 
> Plugins can now run code at three key points:
> - Before arguments are processed and execution is dispatched to a command.
> - After argument processing and dispatch, but before execution.
> - After execution.
> 
> This allows plugins to perform initialization required for argument processing,
> and for post-execution cleanups and synchronizations.
> 
> 
> Diffs
> -----
> 
>   src/main/python/apache/aurora/client/cli/__init__.py f1165a05c7c66d7a06f4733eb65ae4a7de6fad76 
>   src/test/python/apache/aurora/client/cli/test_plugins.py 7cacb02ffda05fdf87a9334ed1de2092efb39f8b 
> 
> Diff: https://reviews.apache.org/r/20521/diff/
> 
> 
> Testing
> -------
> 
> [sun-wukong incubator-aurora (plugin_with_cleanup)]$ ./pants src/test/python/apache/aurora/client/cli:all
> Build operating on targets: OrderedSet([PythonTestSuite(src/test/python/apache/aurora/client/cli/BUILD:all)])
> ============================================== test session starts ===============================================
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> collected 4 items
> 
> src/test/python/apache/aurora/client/cli/test_bridge.py ....
> 
> ============================================ 4 passed in 0.02 seconds ============================================
> ============================================== test session starts ===============================================
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> collected 5 items
> 
> src/test/python/apache/aurora/client/cli/test_help.py .....
> 
> ============================================ 5 passed in 0.76 seconds ============================================
> ============================================== test session starts ===============================================
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> collected 36 items
> 
> src/test/python/apache/aurora/client/cli/test_cancel_update.py ..
> src/test/python/apache/aurora/client/cli/test_create.py ....
> src/test/python/apache/aurora/client/cli/test_diff.py ...
> src/test/python/apache/aurora/client/cli/test_kill.py .........
> src/test/python/apache/aurora/client/cli/test_open.py .....
> src/test/python/apache/aurora/client/cli/test_restart.py ...
> src/test/python/apache/aurora/client/cli/test_status.py .......
> src/test/python/apache/aurora/client/cli/test_update.py ...
> 
> =========================================== 36 passed in 2.02 seconds ============================================
> ============================================== test session starts ===============================================
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> collected 1 items
> 
> src/test/python/apache/aurora/client/cli/test_logging.py .
> 
> ============================================ 1 passed in 0.68 seconds ============================================
> ============================================== test session starts ===============================================
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> collected 2 items
> 
> src/test/python/apache/aurora/client/cli/test_plugins.py ..
> 
> ============================================ 2 passed in 0.63 seconds ============================================
> ============================================== test session starts ===============================================
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> collected 4 items
> 
> src/test/python/apache/aurora/client/cli/test_quota.py ....
> 
> ============================================ 4 passed in 0.61 seconds ============================================
> ============================================== test session starts ===============================================
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> collected 5 items
> 
> src/test/python/apache/aurora/client/cli/test_sla.py .....
> 
> ============================================ 5 passed in 0.59 seconds ============================================
> ============================================== test session starts ===============================================
> platform darwin -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2
> collected 2 items
> 
> src/test/python/apache/aurora/client/cli/test_task_run.py ..
> 
> ============================================ 2 passed in 0.59 seconds ============================================
> src.test.python.apache.aurora.client.cli.bridge                                 .....   SUCCESS
> src.test.python.apache.aurora.client.cli.help                                   .....   SUCCESS
> src.test.python.apache.aurora.client.cli.job                                    .....   SUCCESS
> src.test.python.apache.aurora.client.cli.logging                                .....   SUCCESS
> src.test.python.apache.aurora.client.cli.plugins                                .....   SUCCESS
> src.test.python.apache.aurora.client.cli.quota                                  .....   SUCCESS
> src.test.python.apache.aurora.client.cli.sla                                    .....   SUCCESS
> src.test.python.apache.aurora.client.cli.task                                   .....   SUCCESS
> [sun-wukong incubator-aurora (plugin_with_cleanup)]$
> 
> 
> Thanks,
> 
> Mark Chu-Carroll
> 
>


--===============8287400698028504071==--