mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vinod Kone <vinodk...@gmail.com>
Subject Re: Review Request 47363: Introduced a driver to v1 executor shim/adapter.
Date Sun, 15 May 2016 00:55:21 GMT

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




include/mesos/v1/executor.hpp (line 38)
<https://reviews.apache.org/r/47363/#comment197577>

    We don't name our interfaces as *Interface :) Is there an alternative name we could use?

    
    Also, I'm curious to know why you used inheritance instead of composition. For example,
the Mesos class itself could take a parameter which tells it use V1 or V0 API.



include/mesos/v1/executor.hpp (line 74)
<https://reviews.apache.org/r/47363/#comment197578>

    use the override keyword here?



src/executor/v0_v1executor.hpp (lines 45 - 71)
<https://reviews.apache.org/r/47363/#comment197579>

    do you want to use the override keywords here as well?



src/executor/v0_v1executor.hpp (line 75)
<https://reviews.apache.org/r/47363/#comment197580>

    Any reason why the driver is not a part of the V0ToV1AdapterProcess ?



src/executor/v0_v1executor.cpp (lines 66 - 67)
<https://reviews.apache.org/r/47363/#comment197573>

    why did you need to make a copy?



src/executor/v0_v1executor.cpp (lines 106 - 107)
<https://reviews.apache.org/r/47363/#comment197582>

    hmm. this seems like a bug. we should fix the driver!



src/executor/v0_v1executor.cpp (line 173)
<https://reviews.apache.org/r/47363/#comment197574>

    why did you pull it out instead of doing it inside the case statement?



src/executor/v0_v1executor.cpp (line 234)
<https://reviews.apache.org/r/47363/#comment197581>

    s/pendingEvents/pending/


- Vinod Kone


On May 14, 2016, 8:39 a.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47363/
> -----------------------------------------------------------
> 
> (Updated May 14, 2016, 8:39 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5302
>     https://issues.apache.org/jira/browse/MESOS-5302
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This change adds a driver to v1 executor shim/adapter. This can
> be used by the command executor/docker executor to toggle
> between using the new/old API instead of duplicating the
> code like we did with `http_command_executor.cpp`.
> 
> 
> Diffs
> -----
> 
>   include/mesos/v1/executor.hpp adca287be3bb88c8b3298705740cb6bcbb3a09f0 
>   src/Makefile.am ce5245883f3d2661812272702c0d2060513b6d88 
>   src/executor/v0_v1executor.hpp PRE-CREATION 
>   src/executor/v0_v1executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47363/diff/
> 
> 
> Testing
> -------
> 
> make check (Later in the chain, we make the command executor use this interface)
> So the existing tests should be able to test this out.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>


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