mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Guangya Liu <gyliu...@gmail.com>
Subject Re: Review Request 44934: Updated mesos-execute to add support for Appc.
Date Thu, 17 Mar 2016 15:37:35 GMT


> On 三月 17, 2016, 7:48 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 241
> > <https://reviews.apache.org/r/44934/diff/2/?file=1302214#file1302214line241>
> >
> >     The default value of `containerizer` is `mesos`, do we need to check this?
> >     
> >     What about make the logic as this? This will involve less code change and the
logic may be more clear?
> >     
> >     // Do not touch old dockerImage logic.
> >     if (dockerImage.isSome()) {
> >       old logic here;
> >     }
> >     
> >     if (appcImage.isSome() && containerizer == "mesos") {
> >       ContainerInfo containerInfo;
> >       containerInfo.set_type(ContainerInfo::MESOS);
> >       ContainerInfo::MesosInfo mesosInfo;
> >     
> >       Image::Appc appc;
> >     
> >       appc.set_name(appcImage.get());
> >     
> >       // TODO(jojy): Labels are hard coded right now.
> >       // Consider adding label flags for customization.
> >       Label arch;
> >       arch.set_key("arch");
> >       arch.set_value("amd64");
> >     
> >       Label os;
> >       os.set_key("os");
> >       os.set_value("linux");
> >     
> >       Labels labels;
> >       labels.add_labels()->CopyFrom(os);
> >       labels.add_labels()->CopyFrom(arch);
> >     
> >       appc.mutable_labels()->CopyFrom(labels);
> >     
> >       Image mesosImage;
> >       
> >       mesosImage.set_type(Image::APPC);
> >       mesosImage.mutable_appc()->CopyFrom(appc);
> >       
> >       mesosInfo.mutable_image()->CopyFrom(mesosImage);
> >       
> >       containerInfo.mutable_mesos()->CopyFrom(mesosInfo);
> >     
> >       task.mutable_container()->CopyFrom(containerInfo);
> >     }
> 
> Jojy Varghese wrote:
>     Then we will have to repeat the `if (containerizer == "mesos")` for both dockerImage
case and appcImage case. By seperating the logic by cotainerizer, we now can add more image
types into the "mesos" containerizer block (say OCI image).

Yes, that's also my concern when I open this issue, I think it is ok to keep your current
logic.

The logic of checking `containerizer.empty()` should be removed?


- Guangya


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


On 三月 17, 2016, 6:54 a.m., Jojy Varghese wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> -----------------------------------------------------------
> 
> (Updated 三月 17, 2016, 6:54 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Updated mesos-execute to add support for Appc.
> 
> 
> Diffs
> -----
> 
>   src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e 
> 
> Diff: https://reviews.apache.org/r/44934/diff/
> 
> 
> Testing
> -------
> 
> Tested with various Appc images.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>


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