> On May 6, 2014, 6:59 p.m., Brian Wickman wrote:
> > src/test/python/apache/aurora/executor/test_thermos_executor.py, line 213
> > <https://reviews.apache.org/r/21055/diff/1/?file=574139#file574139line213>
> >
> >     out of curiosity, how does this ever work?  checker should always be an 
> > instance and not a class, so issubclass will fail:
> >     
> >     >>> f = Foo()
> >     >>> issubclass(f, Foo)
> >     Traceback (most recent call last):
> >       File "<stdin>", line 1, in <module>
> >     TypeError: issubclass() arg 1 must be a class
> >     
> >     the constructor of ChainedStatusChecker also ensures that all inputs 
> > are instances of the StatusChecker interface:
> >     
> >     class ChainedStatusChecker(StatusChecker):
> >       def __init__(self, status_checkers):
> >         self._status_checkers = status_checkers
> >         self._status = None
> >         if not all(isinstance(h_i, StatusChecker) for h_i in 
> > status_checkers):
> >           raise TypeError('ChainedStatusChecker must take an iterable of 
> > StatusCheckers.')
> >         super(ChainedStatusChecker, self).__init__()
> >
> 
> Dan Norris wrote:
>     That's a copy/paste error on my part. It should have been:
>     if issubclass(type(checker), StatusChecker):
>     
>     The whole reason for that line is to ensure that we don't try and call 
> .name() something that was coming back as a string in 
> te._chained_checker._status_checkers. I can send you the test output if 
> you're interested in taking a look.

you want isinstance(checker, StatusChecker) instead (calling type(obj) is an 
antipattern in python.)  however, this invariant is already enforced by 
ChainedStatusChecker, so it superfluous.  it sounds like there is a legitimate 
bug somewhere where somebody is calling ".name" instead of ".name()" since it 
was recently changed from an @abstractproperty to a default method.


- Brian


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


On May 4, 2014, 12:02 a.m., Dan Norris wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21055/
> -----------------------------------------------------------
> 
> (Updated May 4, 2014, 12:02 a.m.)
> 
> 
> Review request for Aurora, Jake Farrell, Kevin Sweeney, Bill Farner, and 
> Brian Wickman.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> AURORA-14: Add jenkins-compatible script to build mesos core egg and run 
> executor-large tests
> 
> I took this over from wickman at his request (old review: 
> https://reviews.apache.org/r/19955/).
> 
> Changes:
> - Use mesos 0.18.0 instead of 0.17.0
> - Download mesos source from apache.org
> - Conditionally build mesos .egg
> - Fix a test in test_thermos_executor.py that was broken
> 
> 
> Diffs
> -----
> 
>   .mesosversion PRE-CREATION 
>   3rdparty/python/BUILD 122f71db0bc6f37c19ae0f3cb2fcade8321404e6 
>   build-support/jenkins/build.sh f6f4940a1450cd0d8d8497e651d0e6c7377dfc3f 
>   build-support/jenkins/build_executor.sh PRE-CREATION 
>   build-support/jenkins/build_mesos.sh PRE-CREATION 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/test/python/apache/aurora/executor/test_thermos_executor.py 
> 0b143d0a2cbb25c1d35f79963478800a6b6713ec 
> 
> Diff: https://reviews.apache.org/r/21055/diff/
> 
> 
> Testing
> -------
> 
> ./build-support/jenkins/build_mesos.sh && ./pants 
> src/test/python/apache/aurora/executor:executor-large -vxs (what the new 
> build_executor.sh script does)
> 
> 
> Thanks,
> 
> Dan Norris
> 
>

Reply via email to