Re: Review Request 44622: Introduced a protobuf message "NetworkResult".

[Qian Zhang]

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

(Updated March 22, 2016, 2:40 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


Bugs: MESOS-4759
https://issues.apache.org/jira/browse/MESOS-4759


Repository: mesos


Description
---

Introduced a protobuf message "NetworkResult".


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/spec.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/spec.cpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 

Diff: https://reviews.apache.org/r/44622/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44622: Introduced a protobuf message "NetworkResult".

[Qian Zhang]

> On March 21, 2016, 10:21 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/spec.proto, line 17
> > 
> >
> > Can we put this under cni::spec namespace instead?
> 
> Qian Zhang wrote:
> Jie, did you mean putting it under mesos.internal.slave.cni.spec or under 
> cni.spec? If what you mean is the latter, then I think we may also move this 
> `spec.proto` file to another place, e.g., under `include/mesos/cni`, just 
> like what we did for the spec.proto of Docker and Appc. Agree? :-)
> 
> Jie Yu wrote:
> sorry, i meant the former (under meaos.internal.slave)

Got it, let me update the patch accordingly.


- Qian


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


On March 17, 2016, 6:06 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44622/
> ---
> 
> (Updated March 17, 2016, 6:06 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkResult".
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44622/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44622: Introduced a protobuf message "NetworkResult".

[Jie Yu]

> On March 21, 2016, 2:21 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/spec.proto, line 17
> > 
> >
> > Can we put this under cni::spec namespace instead?
> 
> Qian Zhang wrote:
> Jie, did you mean putting it under mesos.internal.slave.cni.spec or under 
> cni.spec? If what you mean is the latter, then I think we may also move this 
> `spec.proto` file to another place, e.g., under `include/mesos/cni`, just 
> like what we did for the spec.proto of Docker and Appc. Agree? :-)

sorry, i meant the former (under meaos.internal.slave)


- Jie


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


On March 17, 2016, 10:06 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44622/
> ---
> 
> (Updated March 17, 2016, 10:06 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkResult".
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44622/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44622: Introduced a protobuf message "NetworkResult".

[Qian Zhang]

> On March 21, 2016, 10:21 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/spec.proto, line 17
> > 
> >
> > Can we put this under cni::spec namespace instead?

Jie, did you mean putting it under mesos.internal.slave.cni.spec or under 
cni.spec? If what you mean is the latter, then I think we may also move this 
`spec.proto` file to another place, e.g., under `include/mesos/cni`, just like 
what we did for the spec.proto of Docker and Appc. Agree? :-)


- Qian


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


On March 17, 2016, 6:06 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44622/
> ---
> 
> (Updated March 17, 2016, 6:06 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkResult".
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44622/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44622: Introduced a protobuf message "NetworkResult".

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/network/cni/spec.proto (line 17)


Can we put this under cni::spec namespace instead?


- Jie Yu


On March 17, 2016, 10:06 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44622/
> ---
> 
> (Updated March 17, 2016, 10:06 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkResult".
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44622/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44622: Introduced a protobuf message "NetworkResult".

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/network/cni/spec.proto (lines 55 - 59)


Let's move this to the top level:
https://github.com/appc/cni/blob/master/pkg/types/types.go#L94

Also, rename it to IPConfig like above.


- Jie Yu


On March 16, 2016, 7:58 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44622/
> ---
> 
> (Updated March 16, 2016, 7:58 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkResult".
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44622/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44622: Introduced a protobuf message "NetworkResult".

[Qian Zhang]

> On March 12, 2016, 2:51 a.m., Avinash sridharan wrote:
> > Can we introduce the protobuf before the `prepare` method patch?

Sure.


> On March 12, 2016, 2:51 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/spec.proto, line 77
> > 
> >
> > Can we insert an optional `name` field here to keep track of the 
> > `Network` this result is for? This should not break the JSON schema.

I introduced a new struct `NetworkResultInfo` which has a `name` field, please 
see https://reviews.apache.org/r/44514/ for details.


- Qian


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


On March 16, 2016, 3:58 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44622/
> ---
> 
> (Updated March 16, 2016, 3:58 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkResult".
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44622/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44622: Introduced a protobuf message "NetworkResult".

[Qian Zhang]

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

(Updated March 17, 2016, 6:06 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


Bugs: MESOS-4759
https://issues.apache.org/jira/browse/MESOS-4759


Repository: mesos


Description
---

Introduced a protobuf message "NetworkResult".


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 

Diff: https://reviews.apache.org/r/44622/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44622: Introduced a protobuf message "NetworkResult".

[Qian Zhang]

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

(Updated March 16, 2016, 3:58 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


Bugs: MESOS-4759
https://issues.apache.org/jira/browse/MESOS-4759


Repository: mesos


Description
---

Introduced a protobuf message "NetworkResult".


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 

Diff: https://reviews.apache.org/r/44622/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44622: Introduced a protobuf message "NetworkResult".

[Qian Zhang]

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

(Updated March 16, 2016, 3:48 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


Bugs: MESOS-4759
https://issues.apache.org/jira/browse/MESOS-4759


Repository: mesos


Description
---

Introduced a protobuf message "NetworkResult".


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 

Diff: https://reviews.apache.org/r/44622/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44622: Introduced a protobuf message "NetworkResult".

[Avinash sridharan]

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



Can we introduce the protobuf before the `prepare` method patch?


src/slave/containerizer/mesos/isolators/network/spec.proto (line 77)


Can we insert an optional `name` field here to keep track of the `Network` 
this result is for? This should not break the JSON schema.


- Avinash sridharan


On March 10, 2016, 3:29 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44622/
> ---
> 
> (Updated March 10, 2016, 3:29 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkResult".
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44622/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>