aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Brian Wickman" <wick...@apache.org>
Subject Re: Review Request 31451: Port thermos observer to the path detector interface
Date Tue, 03 Mar 2015 22:58:23 GMT


> On March 1, 2015, 8:47 p.m., Joe Smith wrote:
> > src/main/python/apache/aurora/executor/common/resource_manager.py, line 96
> > <https://reviews.apache.org/r/31451/diff/6/?file=880982#file880982line96>
> >
> >     Why is this moving to kwargs? (My normal assumption is to stick with actual
arguments)

this is mostly so we don't have to duplicate all the options for TaskResourceMonitor.  These
options are really just around for mocking.


> On March 1, 2015, 8:47 p.m., Joe Smith wrote:
> > src/main/python/apache/thermos/observer/detector.py, line 8
> > <https://reviews.apache.org/r/31451/diff/6/?file=880992#file880992line8>
> >
> >     You think something like this might help for a docstring?
> >     
> >     ```
> >     class ObserverTaskDetector(object):
> >       """The canonical way to watch (and be notified) of tasks transitioning state
from active -> finished -> removed."""
> >     ```

"""ObserverTaskDetector turns on-disk thermos task transitions into callback events."""


> On March 1, 2015, 8:47 p.m., Joe Smith wrote:
> > src/test/python/apache/aurora/executor/common/test_resource_manager_integration.py,
line 79
> > <https://reviews.apache.org/r/31451/diff/6/?file=880998#file880998line79>
> >
> >     Maybe add a TODO not to write to the filesystem?

No.  It is an integration test.


> On March 1, 2015, 8:47 p.m., Joe Smith wrote:
> > src/test/python/apache/thermos/observer/test_detector.py, line 1
> > <https://reviews.apache.org/r/31451/diff/6/?file=881004#file881004line1>
> >
> >     add apache copywrite header?

fixed


> On March 1, 2015, 8:47 p.m., Joe Smith wrote:
> > src/test/python/apache/thermos/observer/test_detector.py, line 70
> > <https://reviews.apache.org/r/31451/diff/6/?file=881004#file881004line70>
> >
> >     putting this in a `setUp` method would remove the need for remembering `on_finished.reset_mock()`
right?

this would be true if each contextmanager block could live in its own test_... function, but
because the state of the ObserverTaskDetector needs to be carried from context to context
(e.g. active_tasks=TASK1, then finished_tasks=TASK1 to ensure that on_finished is called,
etc) we can't tear down all test state, and instead need to reset the mocks.


> On March 1, 2015, 8:47 p.m., Joe Smith wrote:
> > src/test/python/apache/thermos/observer/test_detector.py, line 76
> > <https://reviews.apache.org/r/31451/diff/6/?file=881004#file881004line76>
> >
> >     I think it's worthy to make this (and others) explicitly:
> >     
> >     assert on_active.mock_calls == [mock.call(TASK1[0], TASK1[1]]

on_active.assert_called_once_with(X) is the same as assert on_active.mock_calls == [mock.call(X)],
right?


- Brian


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


On Feb. 27, 2015, 11:36 p.m., Brian Wickman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31451/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2015, 11:36 p.m.)
> 
> 
> Review request for Aurora, Joe Smith and Zameer Manji.
> 
> 
> Bugs: AURORA-1026
>     https://issues.apache.org/jira/browse/AURORA-1026
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This creates a new abstraction, the ObserverTaskDetector, which is responsible for managing
state transitions for tasks for the observer.  Adds some tests and better debug logging.
> 
> 
> Diffs
> -----
> 
>   src/main/python/apache/aurora/executor/common/BUILD 8cef41d04aff9edc7da7053e05341274a9bd0834

>   src/main/python/apache/aurora/executor/common/resource_manager.py 08e02e41b581f275f070228bb23c4cf2a0489f9a

>   src/main/python/apache/aurora/executor/thermos_task_runner.py 7a28e3255842e3e13a0866d6ad1bfc4cb64781e1

>   src/main/python/apache/thermos/bin/thermos.py 0853a9892399824385bee9e72db4c108f46fceda

>   src/main/python/apache/thermos/common/path.py 846f507e2e097fc04fe0098a7250b40fefcfc6e2

>   src/main/python/apache/thermos/monitoring/disk.py 175ed3af6515e6107e297d91d4e30cbb3034faf7

>   src/main/python/apache/thermos/monitoring/monitor.py 11423bc1764c8380d8de4ad095c1e2748ebb78f8

>   src/main/python/apache/thermos/monitoring/resource.py b4cb881c87a09bb90a740f369a7a5fc5d75dbf04

>   src/main/python/apache/thermos/observer/BUILD ee65f3a46e1d339620e76cadae92c6678fc3510f

>   src/main/python/apache/thermos/observer/bin/BUILD 15a03f74f204f58856f0843b9db05e83b89d1138

>   src/main/python/apache/thermos/observer/bin/thermos_observer.py effa8c19f963bf2792497f4a06049214ae30dfa5

>   src/main/python/apache/thermos/observer/detector.py PRE-CREATION 
>   src/main/python/apache/thermos/observer/http/file_browser.py 87ef9c8a29689c78a5e39a46cc53e4675e36a381

>   src/main/python/apache/thermos/observer/observed_task.py f33aecbc8f3c0a461ae3dba66fbd4986f544dc04

>   src/main/python/apache/thermos/observer/task_observer.py cd528dcca3f5a330359cf38005f3a1a0329a4886

>   src/test/python/apache/aurora/executor/BUILD 2ee9b1233e9db47455ddccccffbc48691d379222

>   src/test/python/apache/aurora/executor/common/BUILD 7b73f693d161cfd205435e4acb398f553b92389f

>   src/test/python/apache/aurora/executor/common/test_resource_manager_integration.py
8f288f6115ab52265dfaffffda3f41d81271c55a 
>   src/test/python/apache/aurora/executor/test_thermos_executor.py 8dbfb1db5eb7a6548820ff7cf82a9c7092f61d28

>   src/test/python/apache/thermos/bin/test_thermos.py 2d9d33397ae01b31ab011d958f5457311ef7ef20

>   src/test/python/apache/thermos/core/test_staged_kill.py faa23ae6afcaa443d23c65e5e027902961a0e52b

>   src/test/python/apache/thermos/monitoring/test_resource.py 52d813946379bdc70c40ad079c74b54f60bd4b41

>   src/test/python/apache/thermos/observer/BUILD PRE-CREATION 
>   src/test/python/apache/thermos/observer/test_detector.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/31451/diff/
> 
> 
> Testing
> -------
> 
> Manually launched observer and ran some thermos tasks.
> +
> mba=aurora=; ./pants test src/test/python/apache/thermos/observer/::
> 
> 
> Thanks,
> 
> Brian Wickman
> 
>


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