> On May 15, 2016, 12:55 a.m., Vinod Kone wrote:
> > include/mesos/v1/executor.hpp, line 38
> > <https://reviews.apache.org/r/47363/diff/2/?file=1383572#file1383572line38>
> >
> >     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.

+1, modified it to `MesosBase` as per our offline discussion. Our style guide 
was mum on the naming scheme. Hence, I picked up the recommendation for naming 
interfaces from the google style guide.


> On May 15, 2016, 12:55 a.m., Vinod Kone wrote:
> > src/executor/v0_v1executor.hpp, line 75
> > <https://reviews.apache.org/r/47363/diff/2/?file=1383574#file1383574line75>
> >
> >     Any reason why the driver is not a part of the V0ToV1AdapterProcess ?

As per our offline discussion: The `driver` takes in an argument to `this` i.e. 
the class that implements the executor interface. Hence, I kept it as part of 
the adapter interface since it implements the executor interface and not the 
process.


> On May 15, 2016, 12:55 a.m., Vinod Kone wrote:
> > src/executor/v0_v1executor.cpp, lines 66-67
> > <https://reviews.apache.org/r/47363/diff/2/?file=1383575#file1383575line66>
> >
> >     why did you need to make a copy?

These are needed for creating the `Event::Subscribed` object upon receiving 
`reregistered` callback. I added an explicit comment now.


> On May 15, 2016, 12:55 a.m., Vinod Kone wrote:
> > src/executor/v0_v1executor.cpp, lines 106-107
> > <https://reviews.apache.org/r/47363/diff/2/?file=1383575#file1383575line106>
> >
> >     hmm. this seems like a bug. we should fix the driver!

Would file a JIRA issue.


- Anand


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


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
> 
>

Reply via email to