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