Re: Review Request 51737: Added `PluginError` to simplify error reporting for CNI plugins.

[Jie Yu]

> On Oct. 12, 2016, 4:49 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/spec.hpp, line 61
> > 
> >
> > 2 lines apart

I'll fix for you


- Jie


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


On Sept. 28, 2016, 7:29 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51737/
> ---
> 
> (Updated Sept. 28, 2016, 7:29 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jie Yu, switched to 'mcypark', and 
> Qian Zhang.
> 
> 
> Bugs: MESOS-6023
> https://issues.apache.org/jira/browse/MESOS-6023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `PluginError` to simplify error reporting for CNI plugins.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
>  11c03fdf8a85414d20704299103e22be7d844f80 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
>  85547533b0b13011615b512ec8c71b7545f33324 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  0ecf64f2de5fc27f208e9dd0e3608b9a6750e9a6 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
> 2d0187beaf0d20e87d5a9ff80f5672ce7b8cc53b 
> 
> Diff: https://reviews.apache.org/r/51737/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51737: Added `PluginError` to simplify error reporting for CNI plugins.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/network/cni/spec.hpp (line 61)


2 lines apart


- Jie Yu


On Sept. 28, 2016, 7:29 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51737/
> ---
> 
> (Updated Sept. 28, 2016, 7:29 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jie Yu, switched to 'mcypark', and 
> Qian Zhang.
> 
> 
> Bugs: MESOS-6023
> https://issues.apache.org/jira/browse/MESOS-6023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `PluginError` to simplify error reporting for CNI plugins.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
>  11c03fdf8a85414d20704299103e22be7d844f80 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
>  85547533b0b13011615b512ec8c71b7545f33324 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  0ecf64f2de5fc27f208e9dd0e3608b9a6750e9a6 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
> 2d0187beaf0d20e87d5a9ff80f5672ce7b8cc53b 
> 
> Diff: https://reviews.apache.org/r/51737/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51737: Added `PluginError` to simplify error reporting for CNI plugins.

[Avinash sridharan]

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

(Updated Sept. 28, 2016, 7:29 p.m.)


Review request for mesos, haosdent huang, Jie Yu, switched to 'mcypark', and 
Qian Zhang.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added `PluginError` to simplify error reporting for CNI plugins.


Diffs (updated)
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
 11c03fdf8a85414d20704299103e22be7d844f80 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
 85547533b0b13011615b512ec8c71b7545f33324 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 0ecf64f2de5fc27f208e9dd0e3608b9a6750e9a6 
  src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
2d0187beaf0d20e87d5a9ff80f5672ce7b8cc53b 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 51737: Added `PluginError` to simplify error reporting for CNI plugins.

[haosdent huang]

> On Sept. 13, 2016, 2:59 p.m., haosdent huang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/spec.hpp, lines 55-59
> > 
> >
> > s/::Error/Error/g
> 
> Avinash sridharan wrote:
> We need the `::Error` to disambiguate the global definition of `Error` 
> from `spec::Error`.

Got it, thx for explanation!


- haosdent


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


On Sept. 12, 2016, 7:27 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51737/
> ---
> 
> (Updated Sept. 12, 2016, 7:27 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jie Yu, switched to 'mcypark', and 
> Qian Zhang.
> 
> 
> Bugs: MESOS-6023
> https://issues.apache.org/jira/browse/MESOS-6023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `PluginError` to simplify error reporting for CNI plugins.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
>  11c03fdf8a85414d20704299103e22be7d844f80 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
>  85547533b0b13011615b512ec8c71b7545f33324 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  0ecf64f2de5fc27f208e9dd0e3608b9a6750e9a6 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
> 2d0187beaf0d20e87d5a9ff80f5672ce7b8cc53b 
> 
> Diff: https://reviews.apache.org/r/51737/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51737: Added `PluginError` to simplify error reporting for CNI plugins.

[Avinash sridharan]

> On Sept. 13, 2016, 2:59 p.m., haosdent huang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/spec.hpp, lines 55-59
> > 
> >
> > s/::Error/Error/g

We need the `::Error` to disambiguate the global definition of `Error` from 
`spec::Error`.


- Avinash


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


On Sept. 12, 2016, 7:27 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51737/
> ---
> 
> (Updated Sept. 12, 2016, 7:27 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jie Yu, switched to 'mcypark', and 
> Qian Zhang.
> 
> 
> Bugs: MESOS-6023
> https://issues.apache.org/jira/browse/MESOS-6023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `PluginError` to simplify error reporting for CNI plugins.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
>  11c03fdf8a85414d20704299103e22be7d844f80 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
>  85547533b0b13011615b512ec8c71b7545f33324 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  0ecf64f2de5fc27f208e9dd0e3608b9a6750e9a6 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
> 2d0187beaf0d20e87d5a9ff80f5672ce7b8cc53b 
> 
> Diff: https://reviews.apache.org/r/51737/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51737: Added `PluginError` to simplify error reporting for CNI plugins.

[haosdent huang]

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


Fix it, then Ship it!




Ship It!


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


s/::Error/Error/g


- haosdent huang


On Sept. 12, 2016, 7:27 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51737/
> ---
> 
> (Updated Sept. 12, 2016, 7:27 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jie Yu, switched to 'mcypark', and 
> Qian Zhang.
> 
> 
> Bugs: MESOS-6023
> https://issues.apache.org/jira/browse/MESOS-6023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `PluginError` to simplify error reporting for CNI plugins.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
>  11c03fdf8a85414d20704299103e22be7d844f80 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
>  85547533b0b13011615b512ec8c71b7545f33324 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  0ecf64f2de5fc27f208e9dd0e3608b9a6750e9a6 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
> 2d0187beaf0d20e87d5a9ff80f5672ce7b8cc53b 
> 
> Diff: https://reviews.apache.org/r/51737/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51737: Added `PluginError` to simplify error reporting for CNI plugins.

[Avinash sridharan]

> On Sept. 10, 2016, 1:15 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/spec.hpp, lines 63-66
> > 
> >
> > Why we need this?

If I don't have an `ostream` operator for `PluginError` the clang compiler 
fails:
../../src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp:66:10:
 error: invalid operands to binary expression ('ostream' (aka 
'basic_ostream') and 'const typename std::conditional::type' (aka 'const 
mesos::internal::slave::cni::spec::PluginError'))
cout << portMapper.error() << endl;

I think its because of this :
https://github.com/apache/mesos/blob/bf7162205b53114eb7367fa322951d573cbb716d/3rdparty/stout/include/stout/try.hpp#L87

Earlier `.error()` used to return a `std::string` but now will return an object 
of type `E` specified in the template for `Try`.


- Avinash


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


On Sept. 12, 2016, 7:27 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51737/
> ---
> 
> (Updated Sept. 12, 2016, 7:27 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jie Yu, switched to 'mcypark', and 
> Qian Zhang.
> 
> 
> Bugs: MESOS-6023
> https://issues.apache.org/jira/browse/MESOS-6023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `PluginError` to simplify error reporting for CNI plugins.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
>  11c03fdf8a85414d20704299103e22be7d844f80 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
>  85547533b0b13011615b512ec8c71b7545f33324 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  0ecf64f2de5fc27f208e9dd0e3608b9a6750e9a6 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
> 2d0187beaf0d20e87d5a9ff80f5672ce7b8cc53b 
> 
> Diff: https://reviews.apache.org/r/51737/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51737: Added `PluginError` to simplify error reporting for CNI plugins.

[Avinash sridharan]

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

(Updated Sept. 12, 2016, 7:27 p.m.)


Review request for mesos, haosdent huang, Jie Yu, switched to 'mcypark', and 
Qian Zhang.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description
---

Added `PluginError` to simplify error reporting for CNI plugins.


Diffs (updated)
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
 11c03fdf8a85414d20704299103e22be7d844f80 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
 85547533b0b13011615b512ec8c71b7545f33324 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 0ecf64f2de5fc27f208e9dd0e3608b9a6750e9a6 
  src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
2d0187beaf0d20e87d5a9ff80f5672ce7b8cc53b 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 51737: Added `PluginError` to simplify error reporting for CNI plugins.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (lines 41 - 42)


swap these two and add a blank line in between



src/slave/containerizer/mesos/isolators/network/cni/spec.hpp (line 59)


No need for the tailing `;'?



src/slave/containerizer/mesos/isolators/network/cni/spec.hpp (lines 63 - 66)


Why we need this?


- Jie Yu


On Sept. 9, 2016, 7:09 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51737/
> ---
> 
> (Updated Sept. 9, 2016, 7:09 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jie Yu, switched to 'mcypark', and 
> Qian Zhang.
> 
> 
> Bugs: MESOS-6023
> https://issues.apache.org/jira/browse/MESOS-6023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `PluginError` to simplify error reporting for CNI plugins.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
>  11c03fdf8a85414d20704299103e22be7d844f80 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
>  85547533b0b13011615b512ec8c71b7545f33324 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  0ecf64f2de5fc27f208e9dd0e3608b9a6750e9a6 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
> 2d0187beaf0d20e87d5a9ff80f5672ce7b8cc53b 
> 
> Diff: https://reviews.apache.org/r/51737/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51737: Added `PluginError` to simplify error reporting for CNI plugins.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51736, 51737]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Sept. 9, 2016, 7:09 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51737/
> ---
> 
> (Updated Sept. 9, 2016, 7:09 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jie Yu, switched to 'mcypark', and 
> Qian Zhang.
> 
> 
> Bugs: MESOS-6023
> https://issues.apache.org/jira/browse/MESOS-6023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `PluginError` to simplify error reporting for CNI plugins.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
>  11c03fdf8a85414d20704299103e22be7d844f80 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
>  85547533b0b13011615b512ec8c71b7545f33324 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  0ecf64f2de5fc27f208e9dd0e3608b9a6750e9a6 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
> 2d0187beaf0d20e87d5a9ff80f5672ce7b8cc53b 
> 
> Diff: https://reviews.apache.org/r/51737/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>