Re: Review Request 51785: [HBASE-16608] Merge for the segments in the compaction pipeline and simplifying the user interface for in-memory flush

[ramkrishna vasudevan]

> On Oct. 16, 2016, 5:13 p.m., Anoop Sam John wrote:
> > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java,
> >  line 179
> > 
> >
> > Able to get what u r trying to do here.  Seems very complex now.
> > So can we take a diff path?
> > We have to merge N segments into one here means we have to merge N 
> > MSLABs referred by these segments.  What we do as per this patch is make a 
> > new HeapMSLAB object and add all pooled chunk from other MSLABs int the Q 
> > in new one.  And other stuff of refer next etc.
> > 
> > Can we make a new MSLAB impl? This will act as a collection of N 
> > HeapMSLABs. Now we have to impl 4 methods
> > incScannerCount and decScannerCount based on an openScannerCount in the 
> > new class.
> > close() method like in HeapMSLAB and has to call close() on each of the 
> > N MSLABs it wrap
> > This is an ImmutableSegment so no one is going to call copyCellInto(). 
> > Make this impl in this new class throw IllegalStateException or so.  Will 
> > that look good?
> 
> Michael Stack wrote:
> No copying? That sounds good.
> 
> Anoop Sam John wrote:
> So this fix and so this comment is applicable to merge op alone..  There 
> is still an open point which Me/Ram are not agreeing around this.. That is 
> whether to do this in mem merge.  This is required to limit the #segments it 
> pipeline and to get bigger sized segment at tail of the pipeline. As we do 
> flush of the tail of the segment.  But our point is let us flush all segments 
> (I mean flush to disk)..  In case of in mem compaction (This action is by def 
> OFF and as per data usage one can enable) it is worth to flush only tail.  In 
> this normal use case let us flush all segments and so there is only flatten 
> of segments comes in and so no bug as this. There is absolutely no gain we 
> get because of delaying of flush of the other segments in the pipeline ( I 
> mean in merge kind of usages)..   So I suggest let us discuss on that before 
> going to work on this. Else it wl b waste of time.
> 
> Anastasia Braginsky wrote:
> I am referencing in this answer the Anoop's suggestion to create a new 
> MSLAB implementation that act as a collection of N old HeapMSLABs. Please pay 
> attention that old scans (opened on N old HeapMSLABs) are not aware of 
> existing of the new MSLAB. In addition the close() of the new MSLAB doesn't 
> automatically means old MSLABs need to be closed, as there might be still 
> ongoing scans. Thus I do not think this idea works (at least not as it is 
> presented now).
> 
> Anastasia Braginsky wrote:
> Anoop, I do not understand about what exactly you disagree. Are you 
> saying that merging of the segments is waste of time? Do you prefer to do a 
> merge upon the flush to disk? Aa we have seen on stress benchmarks this 
> behavior delays flushes and blocks writes which is undesirable. Please 
> explain yourself clearer or we can talk about if we have a meeting this 
> Wednesday.
> 
> Anoop Sam John wrote:
> Reply
> First abt the COllection of N HeapMSLAB wont work.
> You say problem with old scans which were working on old HeapMSLABs and 
> they dont know abt the new MSLAB.  There wont be an issue.  We will really 
> close an MSLAB (I mean return chunks to pool) when there is a close call 
> happened on it AND the scanner count is zero.  So in the case u mentioned, as 
> there are old scan, the scanner count on these MSLABs are >0. Remember we are 
> not going to call close() on these.  We just have a new Collection based 
> MSLAB impl which point to N old MSLABs.  So the old scanners still use the 
> old MSLABs and once done they decr the counter.
> Now on the new one there might be new scan reqs.  So we have scanner 
> count in new COllection based MSLAB impl.  When we are at a point to close 
> this new MSLAB, we will call close on it. It will call close() on all old 
> MSLABs. (If its scanner count is zero or else the close on old MSLABs will be 
> delayed till a decr op makes the count to 0)  Now again within the old MSLAB 
> there will be normal close flow.. Iff its scanner count is 0, it will really 
> get closed.  So in a rare case where the new MSLAB is made and it is at a 
> point of getting closed and still a very old scanner still uses old MSLAB, 
> still no issue.. The one old MSLAB will get really closed only when the long 
> pending scan ends.
> 
> Second abt disagreement of merge
> Ya. I feel that there should NOT be an in memory merge op at all.  We 
> need to have the compaction op any way.  Only in case of compaction op mode, 
> the on disk flush will flush only tail of the pipeline.  In other case (def 
> case) there will be in memory flushes (flatten op) and we keep adding 
> segments to pipeline and here when a flush to disk req comes, we will flush 
> all segments not just tail of 

Review Request 52975: Factored out the create port range logic to test utils.

[Guangya Liu]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Factored out the create port range logic to test utils.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
2e979d784b8e6cdacebac78a67498b5f4d023540 
  src/tests/sorter_tests.cpp 1f17c011898836ea9159661dde7d544cb0d8ea83 
  src/tests/utils.hpp 140ebaaae43b03568ec49891635f0660cdfb4c85 
  src/tests/utils.cpp fc004a9c567898ffbc38a42cc2340dd57347a829 

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


Testing
---

make
make check

```
./bin/mesos-tests.sh  --benchmark  
--gtest_filter="*HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/0"
./bin/mesos-tests.sh  --benchmark 
--gtest_filter="*Sorter_BENCHMARK_Test.FullSort/*"
```


Thanks,

Guangya Liu

Re: Review Request 51785: [HBASE-16608] Merge for the segments in the compaction pipeline and simplifying the user interface for in-memory flush

[Anoop Sam John]

> On Oct. 16, 2016, 10:43 p.m., Anoop Sam John wrote:
> > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java,
> >  line 179
> > 
> >
> > Able to get what u r trying to do here.  Seems very complex now.
> > So can we take a diff path?
> > We have to merge N segments into one here means we have to merge N 
> > MSLABs referred by these segments.  What we do as per this patch is make a 
> > new HeapMSLAB object and add all pooled chunk from other MSLABs int the Q 
> > in new one.  And other stuff of refer next etc.
> > 
> > Can we make a new MSLAB impl? This will act as a collection of N 
> > HeapMSLABs. Now we have to impl 4 methods
> > incScannerCount and decScannerCount based on an openScannerCount in the 
> > new class.
> > close() method like in HeapMSLAB and has to call close() on each of the 
> > N MSLABs it wrap
> > This is an ImmutableSegment so no one is going to call copyCellInto(). 
> > Make this impl in this new class throw IllegalStateException or so.  Will 
> > that look good?
> 
> Michael Stack wrote:
> No copying? That sounds good.
> 
> Anoop Sam John wrote:
> So this fix and so this comment is applicable to merge op alone..  There 
> is still an open point which Me/Ram are not agreeing around this.. That is 
> whether to do this in mem merge.  This is required to limit the #segments it 
> pipeline and to get bigger sized segment at tail of the pipeline. As we do 
> flush of the tail of the segment.  But our point is let us flush all segments 
> (I mean flush to disk)..  In case of in mem compaction (This action is by def 
> OFF and as per data usage one can enable) it is worth to flush only tail.  In 
> this normal use case let us flush all segments and so there is only flatten 
> of segments comes in and so no bug as this. There is absolutely no gain we 
> get because of delaying of flush of the other segments in the pipeline ( I 
> mean in merge kind of usages)..   So I suggest let us discuss on that before 
> going to work on this. Else it wl b waste of time.
> 
> Anastasia Braginsky wrote:
> I am referencing in this answer the Anoop's suggestion to create a new 
> MSLAB implementation that act as a collection of N old HeapMSLABs. Please pay 
> attention that old scans (opened on N old HeapMSLABs) are not aware of 
> existing of the new MSLAB. In addition the close() of the new MSLAB doesn't 
> automatically means old MSLABs need to be closed, as there might be still 
> ongoing scans. Thus I do not think this idea works (at least not as it is 
> presented now).
> 
> Anastasia Braginsky wrote:
> Anoop, I do not understand about what exactly you disagree. Are you 
> saying that merging of the segments is waste of time? Do you prefer to do a 
> merge upon the flush to disk? Aa we have seen on stress benchmarks this 
> behavior delays flushes and blocks writes which is undesirable. Please 
> explain yourself clearer or we can talk about if we have a meeting this 
> Wednesday.

Reply
First abt the COllection of N HeapMSLAB wont work.
You say problem with old scans which were working on old HeapMSLABs and they 
dont know abt the new MSLAB.  There wont be an issue.  We will really close an 
MSLAB (I mean return chunks to pool) when there is a close call happened on it 
AND the scanner count is zero.  So in the case u mentioned, as there are old 
scan, the scanner count on these MSLABs are >0. Remember we are not going to 
call close() on these.  We just have a new Collection based MSLAB impl which 
point to N old MSLABs.  So the old scanners still use the old MSLABs and once 
done they decr the counter.
Now on the new one there might be new scan reqs.  So we have scanner count in 
new COllection based MSLAB impl.  When we are at a point to close this new 
MSLAB, we will call close on it. It will call close() on all old MSLABs. (If 
its scanner count is zero or else the close on old MSLABs will be delayed till 
a decr op makes the count to 0)  Now again within the old MSLAB there will be 
normal close flow.. Iff its scanner count is 0, it will really get closed.  So 
in a rare case where the new MSLAB is made and it is at a point of getting 
closed and still a very old scanner still uses old MSLAB, still no issue.. The 
one old MSLAB will get really closed only when the long pending scan ends.

Second abt disagreement of merge
Ya. I feel that there should NOT be an in memory merge op at all.  We need to 
have the compaction op any way.  Only in case of compaction op mode, the on 
disk flush will flush only tail of the pipeline.  In other case (def case) 
there will be in memory flushes (flatten op) and we keep adding segments to 
pipeline and here when a flush to disk req comes, we will flush all segments 
not just tail of pipeline..  We try to do this merge only to make the #segments 
in pipeline less and to have a bigger sized tail for 

Re: Review Request 52783: Added documentation for mesos-containerizer Linux capabilities support.

[Avinash sridharan]

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




docs/configuration.md (line 1003)


s/the agent/that the agent/



docs/configuration.md (line 1005)


is enabled, (linux/capabilities is not present in the
arguments to --isolation) and this flags is ignored.



docs/linux_capabilities.md (line 3)


s/described/describes

to add support for `Linux Capabilities`



docs/linux_capabilities.md (line 9)


s/tasks/Mesos tasks



docs/linux_capabilities.md (line 15)


two lines?



docs/linux_capabilities.md (line 22)


s/capabilities/capability?



docs/linux_capabilities.md (line 34)


s/-allowed_capabilities/--allowed_capabilities

Might be worthwhile to elaborate as to why an agent should ever be started 
with an empty `--allowed_capabilities` flag?


- Avinash sridharan


On Oct. 17, 2016, 1:16 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52783/
> ---
> 
> (Updated Oct. 17, 2016, 1:16 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6376
> https://issues.apache.org/jira/browse/MESOS-6376
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for mesos-containerizer Linux capabilities support.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md c83a58eb6884c8d8c37880a745e04cf0b789ebdc 
>   docs/linux_capabilities.md PRE-CREATION 
>   docs/mesos-containerizer.md 76544625838845fe2817347fea483a60ab250f99 
> 
> Diff: https://reviews.apache.org/r/52783/diff/
> 
> 
> Testing
> ---
> 
> Checked with local renderer.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51052: Make mesos-docker-execute understand cgroups_enable_cfs.

[Jie Yu]

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




src/docker/docker.hpp (line 51)


Having this as a global knob sounds really weird to me. I know you want to 
work around the Docker::run GMock issue. But this is too weird to me. That 
means we cannot have per docker container cfs control (i.e., some container 
uses cfs while some doesn't)



src/docker/docker.cpp (lines 499 - 500)


Maybe as a work around, you can combine these two:
```
struct Stdio {
  Subprocess::IO out;
  Subprocess::IO err;
};

Future Docker::run(
  ...
  const Stdio& stdio) const
```


- Jie Yu


On Oct. 12, 2016, 5:02 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51052/
> ---
> 
> (Updated Oct. 12, 2016, 5:02 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-6134
> https://issues.apache.org/jira/browse/MESOS-6134
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixes cpu quota for command executor (which runs outside
> of the docker container) by ensuing --cpu-quota flag to docker
> run.
> 
> Note: we have to add the boolean variable to `Docker` class
> because `Docker::run()` has reached the maximum argument length
> GMOCK can support.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp 50fda393a42afefc70790a26b44911e4cf17185e 
>   src/docker/executor.hpp a49ec1b4045116741af6af08578791fb0440ad8f 
>   src/docker/executor.cpp ab3f0473fdc9105d1c425f0dbe7b81c566d541e8 
>   src/slave/containerizer/docker.cpp d71386089bf7677872bcb1bd36e07da9263dcf0d 
>   src/tests/mock_docker.cpp 6a0e613bde6889864a37ffd7ec0b454e5fe4df1c 
> 
> Diff: https://reviews.apache.org/r/51052/diff/
> 
> 
> Testing
> ---
> 
> I am now able to make docker containers launched through mesos-execute have a 
> cpu quota.
> 
> Also making sure `make check` still works on mac os for the linux only flag.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52967: Improved documentation for shared persistent volumes.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52965, 52966, 52967]

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 Oct. 18, 2016, 12:50 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52967/
> ---
> 
> (Updated Oct. 18, 2016, 12:50 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved documentation for shared persistent volumes.
> 
> 
> Diffs
> -
> 
>   docs/home.md 1c6b191bd194a9100ce1ad4bf5aff62a20ed8f41 
>   docs/persistent-volume.md 5dfbbf1fb08cdf47f97ea6fb286e21ab26235d62 
>   docs/shared-resources.md 29e43389c530f816118a32dcd7c2f1b82b0af431 
> 
> Diff: https://reviews.apache.org/r/52967/diff/
> 
> 
> Testing
> ---
> 
> Previewed with site-docker.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 52696: Harden stout

[James Peach]

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




3rdparty/stout/Makefile.am (line 27)


Where does ``VARIANTS`` come from?


- James Peach


On Oct. 14, 2016, 3:20 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52696/
> ---
> 
> (Updated Oct. 14, 2016, 3:20 p.m.)
> 
> 
> Review request for mesos, Michael Park and Neil Conway.
> 
> 
> Bugs: MESOS-6229
> https://issues.apache.org/jira/browse/MESOS-6229
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use a default set of flags to provide additional security and hardening to 
> stout. Additionally, check and catch more warnings/errors.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am fda069d 
> 
> Diff: https://reviews.apache.org/r/52696/diff/
> 
> 
> Testing
> ---
> 
> Compared the benchmarks with and without the flags being used. Also did a 
> comparsion with the flags being used with and without optimizations and 
> without the flags being used with and without optimizations. Overall the 
> performance hit was very small with a 3-8% overhead (optimizations brings 
> this down slightly). Most benchmarks were about 5% (or less) slower.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52647: Fix new sign comparison errors in libprocess produced by hardened flags

[James Peach]

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




3rdparty/libprocess/src/encoder.hpp (line 291)


It's not obvious to me that this is the right change since the surrounding 
code tries to deal with ``off_t``. Can you explain this some more?



3rdparty/libprocess/src/tests/io_tests.cpp (line 284)


Can you just make ``length`` type ``ssize_t``?


- James Peach


On Oct. 14, 2016, 3:14 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52647/
> ---
> 
> (Updated Oct. 14, 2016, 3:14 p.m.)
> 
> 
> Review request for mesos, Michael Park and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors in libprocess 
> that need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp c79296b 
>   3rdparty/libprocess/src/encoder.hpp 005d1cc 
>   3rdparty/libprocess/src/process.cpp 18a8e20 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
>   3rdparty/libprocess/src/tests/http_tests.cpp 8d6c8c4 
>   3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp c8350cf 
> 
> Diff: https://reviews.apache.org/r/52647/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran `make && make check && make bench`.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52625: Replaced POSIX `int` with `FileDesc` abstraction in `libprocess` folder.

[Daniel Pravat]

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

(Updated Oct. 18, 2016, 2:47 a.m.)


Review request for mesos, Alex Naparu, Alex Clemmer, Joseph Wu, and Michael 
Park.


Repository: mesos


Description
---

On POSIX this should have no effect.


Diffs (updated)
-

  3rdparty/libprocess/include/process/io.hpp 
eec5efd7e6b71a783f2bb40826054d0488cee71f 
  3rdparty/libprocess/include/process/network.hpp 
52110667185370a4c92e2fa524819ab1f34bdec9 
  3rdparty/libprocess/include/process/socket.hpp 
67551a904ebc4c2f97d65ad7ab5d4ab8c07f16db 
  3rdparty/libprocess/include/process/subprocess_base.hpp 
1d02454d5541f96cb4928bf027fcae3764989d67 
  3rdparty/libprocess/include/process/windows/subprocess.hpp 
f452f6743d01f0b99010fa5e5bcbaae1358c8241 
  3rdparty/libprocess/src/io.cpp d930ceebc90468e082b984e41385549f906dc6ae 
  3rdparty/libprocess/src/poll_socket.hpp 
d04f3f2d1bcf70464ac659b29f96574bbd233414 
  3rdparty/libprocess/src/poll_socket.cpp 
d9ab3fbbf385c39c0e43ee92ff06609ae8d47e2d 
  3rdparty/libprocess/src/process.cpp 18a8e206f6f297157d246a94f374311be67cd782 
  3rdparty/libprocess/src/socket.cpp 6089248639793603226210421a2c2193d14ea049 
  3rdparty/libprocess/src/subprocess_windows.cpp 
20cad52d4a4d7fc51487e150a849972eb19ed08e 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 
630a7147c327de41f3166f5802f7ef6e12532aa3 
  3rdparty/stout/include/stout/os/open.hpp 
2a357926860b1523c51f12c7edee2babe6afbfa3 

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


Testing
---


Thanks,

Daniel Pravat

Re: Review Request 52624: Replaced POSIX `int` with `FileDesc` abstraction in `src` folder.

[Daniel Pravat]

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

(Updated Oct. 18, 2016, 2:46 a.m.)


Review request for mesos, Alex Naparu, Alex Clemmer, Joseph Wu, and Michael 
Park.


Repository: mesos


Description
---

On POSIX this should have no effect.


Diffs (updated)
-

  cmake/CompilationConfigure.cmake 11a8507eb773391073a7b945e2aac503262f86b7 
  src/files/files.cpp 9c634ac928887b3f1a111f67ebb3fc5229c6fa16 
  src/health-check/health_checker.cpp 96ae1a733ff3d211b84d0893b4603873af1c89f0 
  src/slave/containerizer/fetcher.cpp 11104d66e6dd05d8eb1d37a2e3250aca19278110 
  src/slave/containerizer/mesos/containerizer.cpp 
eac70d955e08142a2d054039d610a3d516b1b57e 
  src/slave/containerizer/mesos/launch.hpp 
f8bac0650965a49562b9910bf6140ded8dbb69ac 
  src/slave/containerizer/mesos/launch.cpp 
8a30ff8bd6f9263d68a4344b79f2374a2ae53c04 
  src/slave/state.cpp a94bb8d7029295abef70d6595ebc732ac1ab87a8 
  src/slave/status_update_manager.cpp 056a684b52756d5c6309e7e2167a1532c4e60957 

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


Testing
---


Thanks,

Daniel Pravat

Review Request 52972: Replaced POSIX `int` with `FileDesc` abstraction in `stout` folder.

[Daniel Pravat]

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

Review request for mesos.


Repository: mesos


Description
---

Replaced POSIX `int` with `FileDesc` abstraction in `stout` folder.


Diffs
-

  3rdparty/stout/include/stout/net.hpp 083b8d1959dcd305d0aa8892082b39cb03786ebf 
  3rdparty/stout/include/stout/os/mktemp.hpp 
2dfd35605eb4202a5475fe0e0d2f1fd27690a2de 
  3rdparty/stout/include/stout/os/open.hpp 
2a357926860b1523c51f12c7edee2babe6afbfa3 
  3rdparty/stout/include/stout/os/read.hpp 
d0b657db5b7dc0c1e11edc13fd6830a9f6273867 
  3rdparty/stout/include/stout/os/socket.hpp 
e9d9306e63aff2be083b4254fbf6f31c37edc424 
  3rdparty/stout/include/stout/os/touch.hpp 
84d49bb8adc2612e380f037fd42c47166d55f593 
  3rdparty/stout/include/stout/os/windows/close.hpp 
9f1566bff19ee872418bce8a43a119c4f0a70a7c 
  3rdparty/stout/include/stout/os/windows/fcntl.hpp 
2bc794a405e59d80c1e8308c0049d2306347adfa 
  3rdparty/stout/include/stout/os/windows/read.hpp 
09190f97f33db5dfa023f937f8f2a4a62ed1ec15 
  3rdparty/stout/include/stout/os/windows/shell.hpp 
17e3d564564abebf1d558b7a7a277aef3c87e5ae 
  3rdparty/stout/include/stout/os/windows/socket.hpp 
1f6551bc30cf31842513df0fed43ee134c620ebd 
  3rdparty/stout/include/stout/os/windows/write.hpp 
23488708ae366b8571bb8b4805f67d2054223fff 
  3rdparty/stout/include/stout/os/write.hpp 
24a69d8f60efd3c2888d464d75164c758b3701a2 
  3rdparty/stout/include/stout/posix/os.hpp 
c37e64db662ba3cee83d2f55de0f9d71ad72c038 
  3rdparty/stout/include/stout/protobuf.hpp 
80cb20f40a7ddd4309d27973eef9fca9e4052b64 
  3rdparty/stout/include/stout/windows/os.hpp 
7ca0b5dc9793369ea142684e3614e8f33cac64b6 
  3rdparty/stout/tests/os/filesystem_tests.cpp 
e3894ef6f21c15845085400a0d3426520411788e 

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


Testing
---


Thanks,

Daniel Pravat

Re: Review Request 52544: Introduced `FileDesc` class.

[Daniel Pravat]

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

(Updated Oct. 18, 2016, 2:45 a.m.)


Review request for mesos, Alex Naparu, Alex Clemmer, Joseph Wu, and Michael 
Park.


Repository: mesos


Description (updated)
---

In POSIX the `socket`,`pipe` and the `filedescriptor` are
represented by an int type. In Windows a socket is kept in a
`SOCKET` type (64 bit wide), a pipe in a `HANDLE` (64 bit wide) and
a file descriptor in an int. Ths class unifies all Windows types.
In POSIX this class defaults to int (can be easily extended).


Diffs (updated)
-

  3rdparty/stout/include/stout/os.hpp 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
  3rdparty/stout/include/stout/os/filedescriptor.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/windows/filedescriptor.hpp PRE-CREATION 

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


Testing
---


Thanks,

Daniel Pravat

Re: Review Request 50841: Added GPU scheduling logic to docker containerizer process.

[Guangya Liu]

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




src/slave/containerizer/docker.hpp (lines 258 - 274)


Move this to `#ifdef __linux__`

```
#ifdef __linux__
// Allocate GPU resources for a specified container.
process::Future allocateNvidiaGpus(
  const size_t count,
  const ContainerID& containerId);

process::Future _allocateNvidiaGpus(
  const std::set& allocated,
  const ContainerID& containerId);

// Deallocate GPU resources for a specified container.
process::Future deallocateNvidiaGpus(
  const ContainerID& containerId);

process::Future _deallocateNvidiaGpus(
  const ContainerID& containerId);
#endif
```



src/slave/containerizer/docker.hpp (lines 503 - 504)


```
#ifdef __linux__
// GPU resources allocated to the container.
std::set gpus;
#endif
```



src/slave/containerizer/docker.cpp (lines 670 - 688)


You will not need `#ifdef __linux__` for those helper functions as we 
already put those helper functions surrounded by `#ifdef __liux__`.

Ditto for others.


- Guangya Liu


On 十月 14, 2016, 9:56 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50841/
> ---
> 
> (Updated 十月 14, 2016, 9:56 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and 
> Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added control logic to allocate/deallocate GPUs to GPU-related task
> when the task is started/terminated.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 8da63101f951892e673612134770fc155d86112d 
>   src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
> 
> Diff: https://reviews.apache.org/r/50841/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 50123: Added GPU scheduler for docker containerizer process.

[Guangya Liu]

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




src/slave/containerizer/docker.hpp (line 76)


Had some discussion offline with Kevin, it is better to use 
`NvidiaComponents` as a parameter here as it can also enable us to handle 
`NvidiaVolume` easily in future, this can also enable us use less `#ifdef 
__linux__` in docker.cpp.


- Guangya Liu


On 十月 14, 2016, 9:55 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50123/
> ---
> 
> (Updated 十月 14, 2016, 9:55 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This added 'NvidiaGpuAllocator' to docker containerizer process so that
> docker containerizer process is ready to use it to allocate GPUs to task
> with 'gpus' resource.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 8da63101f951892e673612134770fc155d86112d 
>   src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
>   src/slave/containerizer/mesos/isolators/gpu/allocator.hpp 
> b2eabfebef99ccebef427d144bb816adc0175ecf 
>   src/slave/containerizer/mesos/isolators/gpu/components.hpp 
> 82fca5de07c28720f102a02a77563e325e98e066 
>   src/tests/mock_docker.hpp 1bf09c8dba020b421526b650523c879fb87380f8 
>   src/tests/mock_docker.cpp 6a0e613bde6889864a37ffd7ec0b454e5fe4df1c 
> 
> Diff: https://reviews.apache.org/r/50123/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 52741: Added capabilities support to mesos-execute.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52780, 52741]

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 Oct. 12, 2016, 2:31 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52741/
> ---
> 
> (Updated Oct. 12, 2016, 2:31 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5303
> https://issues.apache.org/jira/browse/MESOS-5303
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces a flags `capabilities` to mesos-execute which
> can be used to specify the capabilities a task should be able to
> aquire.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp fcf627b0dcbedd01700cc8c9acadf7ba0dae4faa 
> 
> Diff: https://reviews.apache.org/r/52741/diff/
> 
> 
> Testing
> ---
> 
> * `make check`, including `ROOT` tests,
> * tested as root with a `ping` task with an agent where both the task and the 
> agent have different or no capabilities configured.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 52970: Fixed a comment and some style issues in `network/cni` isolator.

[Avinash sridharan]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Fixed a comment and some style issues in `network/cni` isolator.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ae78c65d205380bae8ae39135710d5e78fdfd475 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 52311: Pass the user value from executor of switch_user flag is set.

[Joseph Wu]

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



The `MasterTest.MasterFailoverLongLivedExecutor` failing indicates a regression 
with this patch.  Since we can't change the `getExecutorInfo` method without 
breaking something else, we'll have to resort to making a change to the 
`ContainerLogger`'s interface.  It will need an explicit `const Option& 
user` argument.

In the Mesos containerizer, that user can be derived like:
```
Option user;
if (container->config.has_user()) {
  user = container->config.user();
}
```

In the Docker containerizer, it is simply:
```
container->user
```

- Joseph Wu


On Oct. 13, 2016, 11:52 a.m., Sivaram Kannan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52311/
> ---
> 
> (Updated Oct. 13, 2016, 11:52 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-5856
> https://issues.apache.org/jira/browse/MESOS-5856
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pass the user value from executor of switch_user flag is set.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 119fb36c27b25739f2a86a55d48e964ca4a84ff7 
> 
> Diff: https://reviews.apache.org/r/52311/diff/
> 
> 
> Testing
> ---
> 
> 1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
> the logs are getting rotated. 
> 2. Run the mesos-logrotate-logger as root user and verify whether the logs 
> are getting rotated.
> 
> 
> Thanks,
> 
> Sivaram Kannan
> 
>

Re: Review Request 52693: Changed master to send TASK_UNKNOWN during reconciliation.

[Neil Conway]

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

(Updated Oct. 18, 2016, 1:14 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebase, new dep.


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


Repository: mesos


Description
---

Previously, the master would send TASK_LOST in response to explicit
reconciliation requests for (a) unknown tasks at registered slaves and
(b) tasks at unknown slaves (neither registered nor unreachable). The
master will now send TASK_UNKNOWN for these situations if the framework
has the PARTITION_AWARE capability.


Diffs (updated)
-

  src/master/master.cpp 3c6b18ead44cd5f2978093f5415e974cfcbfa714 
  src/tests/partition_tests.cpp 12fe8593ff17c35d540f944c428cf7f33b7dcbb3 
  src/tests/reconciliation_tests.cpp 1412090299df388456f04ed58a1d384ce3ff550a 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 52659: Changed master to send TASK_DROPPED for task launch errors.

[Neil Conway]

> On Oct. 18, 2016, 12:35 a.m., Vinod Kone wrote:
> > src/tests/master_authorization_tests.cpp, line 783
> > 
> >
> > This test looks identical to SlaveRemoved test. Please kill this in a 
> > different review.

Done, https://reviews.apache.org/r/52969/


- Neil


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


On Oct. 18, 2016, 1:05 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52659/
> ---
> 
> (Updated Oct. 18, 2016, 1:05 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6329
> https://issues.apache.org/jira/browse/MESOS-6329
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a task launch fails due to a transient error (e.g., insufficient
> available resources at an agent), the master sends a TASK_LOST update to
> the framework. For PARTITION_AWARE frameworks, we now send TASK_DROPPED
> instead.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 3c6b18ead44cd5f2978093f5415e974cfcbfa714 
>   src/tests/master_authorization_tests.cpp 
> a4623d15c246651fd1038fdedf16321b1d5f273f 
>   src/tests/master_tests.cpp 88cf1e612ad8186ef2cea161b3a52b0df9517305 
> 
> Diff: https://reviews.apache.org/r/52659/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 52969: Removed redundant test.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

`MasterAuthorizationTest.SlaveDisconnectedLost` was identical to
`MasterAuthorizationTest.SlaveRemovedLost`.


Diffs
-

  src/tests/master_authorization_tests.cpp 
a4623d15c246651fd1038fdedf16321b1d5f273f 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 52659: Changed master to send TASK_DROPPED for task launch errors.

[Neil Conway]

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

(Updated Oct. 18, 2016, 1:05 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Tweak comment per review.


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


Repository: mesos


Description
---

When a task launch fails due to a transient error (e.g., insufficient
available resources at an agent), the master sends a TASK_LOST update to
the framework. For PARTITION_AWARE frameworks, we now send TASK_DROPPED
instead.


Diffs (updated)
-

  src/master/master.cpp 3c6b18ead44cd5f2978093f5415e974cfcbfa714 
  src/tests/master_authorization_tests.cpp 
a4623d15c246651fd1038fdedf16321b1d5f273f 
  src/tests/master_tests.cpp 88cf1e612ad8186ef2cea161b3a52b0df9517305 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 52953: Added container 'exec' command to the CLI.

[Joseph Wu]

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




src/cli_new/lib/mesos/plugins/container/main.py (line 30)


Hit the same linter-using-old-virtualenv error when applying this patch as 
described in:
https://reviews.apache.org/r/51008/#comment12


- Joseph Wu


On Oct. 17, 2016, 1:12 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52953/
> ---
> 
> (Updated Oct. 17, 2016, 1:12 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-5676
> https://issues.apache.org/jira/browse/MESOS-5676
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added container 'exec' command to the CLI.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/mesos/plugins/container/main.py PRE-CREATION 
>   src/cli_new/lib/mesos/plugins/container/tests.py PRE-CREATION 
>   src/cli_new/lib/mesos/tests/base.py PRE-CREATION 
>   src/cli_new/lib/mesos/util.py 87d2a65e78f04209566c1434b489b941d570ee01 
>   src/cli_new/pip-requirements.txt 9cc8d096ac353f4ed2d4318f2d0e439a6edf 
> 
> Diff: https://reviews.apache.org/r/52953/diff/
> 
> 
> Testing
> ---
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ sudo bin/mesos-cli-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 51008: Added infrastructure for unit tests in the new python-based CLI.

[Joseph Wu]

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




src/cli_new/bin/tests.py (lines 23 - 25)


This actually failed to lint after applying:
```
Checking 1 Python file
* Module tests
E: 23, 0: Unable to import 'colour_runner.runner' (import-error)
E: 25, 0: Unable to import 'termcolor' (import-error)
Total errors found: 2
```

Reason being that I had a pre-existing virtualenv, which did not have these 
modules installed.

Doing `rm -rf src/cli_new/.virtualenv/` allows the linter to pass.


- Joseph Wu


On Oct. 17, 2016, 1:11 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51008/
> ---
> 
> (Updated Oct. 17, 2016, 1:11 p.m.)
> 
> 
> Review request for mesos, Haris Choudhary and Joseph Wu.
> 
> 
> Bugs: MESOS-6032
> https://issues.apache.org/jira/browse/MESOS-6032
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added infrastructure for unit tests in the new python-based CLI.
> 
> 
> Diffs
> -
> 
>   src/cli_new/bin/mesos-cli-tests PRE-CREATION 
>   src/cli_new/bin/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 9cc8d096ac353f4ed2d4318f2d0e439a6edf 
> 
> Diff: https://reviews.apache.org/r/51008/diff/
> 
> 
> Testing
> ---
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52943: Updated CLI pylint configuration to disable no-self-use warnings.

[Joseph Wu]

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


Ship it!




I see this warning here: 
https://reviews.apache.org/r/52950/
Which seems valid in that context.

Another warning here:
https://reviews.apache.org/r/52952/
That one seems ok to disable the warning.

Lots of warnings in:
https://reviews.apache.org/r/52953/

Considering the potential (unnecessary) code churn from switching back and 
forth between `@classmethod` and not-`@classmethod`, disabling this one seems 
ok.

- Joseph Wu


On Oct. 17, 2016, 1:11 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52943/
> ---
> 
> (Updated Oct. 17, 2016, 1:11 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-5676
> https://issues.apache.org/jira/browse/MESOS-5676
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated CLI pylint configuration to disable no-self-use warnings.
> 
> 
> Diffs
> -
> 
>   src/cli_new/pylint.config c398220db063d249e6c62cf7e8cd6757e7860630 
> 
> Diff: https://reviews.apache.org/r/52943/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52944: Updated CLI pylint configuration to disable 'fixme' warnings.

[Joseph Wu]

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


Ship it!




Considering how we love TODOs on our codebase, this one is perfectly reasonable 
to disable :)

- Joseph Wu


On Oct. 17, 2016, 1:11 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52944/
> ---
> 
> (Updated Oct. 17, 2016, 1:11 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-5676
> https://issues.apache.org/jira/browse/MESOS-5676
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated CLI pylint configuration to disable 'fixme' warnings.
> 
> 
> Diffs
> -
> 
>   src/cli_new/pylint.config c398220db063d249e6c62cf7e8cd6757e7860630 
> 
> Diff: https://reviews.apache.org/r/52944/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52658: Changed scheduler driver to send TASK_DROPPED.

[Neil Conway]

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

(Updated Oct. 18, 2016, 1:01 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Tweak comment per review.


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


Repository: mesos


Description
---

If a scheduler tries to launch a task when the scheduler driver is not
connected to the master, the scheduler driver creates a faux TASK_LOST
status update to indicate that the task launch has not succeeded. If the
framework is PARTITION_AWARE, the scheduler driver will now send
TASK_DROPPED instead.


Diffs (updated)
-

  src/sched/sched.cpp 9d1b5ce2e1a179b2e6ea212d99d8d7fe72a0793a 
  src/tests/fault_tolerance_tests.cpp 5a9944cf459ab688907d95bbda09f464b37efd1e 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 52941: Updated CLI pylint configuration to allow 0 public methods.

[Joseph Wu]

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




src/cli_new/pylint.config (line 15)


Is this used anywhere?  I did not encounter a linting error for this in 
your chain.


- Joseph Wu


On Oct. 17, 2016, 1:11 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52941/
> ---
> 
> (Updated Oct. 17, 2016, 1:11 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-5676
> https://issues.apache.org/jira/browse/MESOS-5676
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated CLI pylint configuration to allow 0 public methods.
> 
> 
> Diffs
> -
> 
>   src/cli_new/pylint.config c398220db063d249e6c62cf7e8cd6757e7860630 
> 
> Diff: https://reviews.apache.org/r/52941/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52940: Updated CLI pylint configuration to allow up to 30 local variables.

[Joseph Wu]

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




src/cli_new/pylint.config (line 14)


Looks like `__enter_namespaces` in https://reviews.apache.org/r/52953/ is 
over the default limit of 15 by one variable...

I'd say that function needs some helpers, rather than silencing the warning.


- Joseph Wu


On Oct. 17, 2016, 1:11 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52940/
> ---
> 
> (Updated Oct. 17, 2016, 1:11 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-5676
> https://issues.apache.org/jira/browse/MESOS-5676
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated CLI pylint configuration to allow up to 30 local variables.
> 
> 
> Diffs
> -
> 
>   src/cli_new/pylint.config c398220db063d249e6c62cf7e8cd6757e7860630 
> 
> Diff: https://reviews.apache.org/r/52940/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52968: Added `mesos-cni-port-mapper` to the CHANGELOG.

[Avinash sridharan]

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

(Updated Oct. 18, 2016, 1 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Added a JIRA.


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


Repository: mesos


Description
---

Added `mesos-cni-port-mapper` to the CHANGELOG.


Diffs
-

  CHANGELOG 004568f9ab6284a004a090944b2c6f15d6018c85 

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


Testing
---


Thanks,

Avinash sridharan

Re: Review Request 52939: Updated CLI pylint configuration to allow 2 character variable names.

[Joseph Wu]

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




src/cli_new/pylint.config (line 13)


I see a single 2 character variable name `io` here: 
https://reviews.apache.org/r/52953/diff/1#0 -- Line 306
I think a longer variable name is reasonable there.

`ip` can be added to the `good-names`.


- Joseph Wu


On Oct. 17, 2016, 1:11 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52939/
> ---
> 
> (Updated Oct. 17, 2016, 1:11 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-5676
> https://issues.apache.org/jira/browse/MESOS-5676
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated CLI pylint configuration to allow 2 character variable names.
> 
> 
> Diffs
> -
> 
>   src/cli_new/pylint.config c398220db063d249e6c62cf7e8cd6757e7860630 
> 
> Diff: https://reviews.apache.org/r/52939/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52968: Added `mesos-cni-port-mapper` to the CHANGELOG.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Oct. 18, 2016, 12:57 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52968/
> ---
> 
> (Updated Oct. 18, 2016, 12:57 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `mesos-cni-port-mapper` to the CHANGELOG.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 004568f9ab6284a004a090944b2c6f15d6018c85 
> 
> Diff: https://reviews.apache.org/r/52968/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 52771: Captured the `stderr` during execution of CNI plugin.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 1229)


Why this change?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 1486 - 1487)


Out of date?


- Jie Yu


On Oct. 18, 2016, 12:02 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52771/
> ---
> 
> (Updated Oct. 18, 2016, 12:02 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6282
> https://issues.apache.org/jira/browse/MESOS-6282
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Till now we were capturing only the `stdout` of the CNI plugin. However,
> during errors it makes sense to add the output from `stderr` for the
> CNI plugin as well, since as per the CNI spec the CNI plugin is
> supposed to output all unstructured output (debugs for example) to
> `stderr`. This greatly helps in debugging the exact reason for CNI
> plugin failures.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 70f30831819d7a0e6233fcb13a703dc6981324b6 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 7c35c3056326a8a135e4ad944ac741cda96cab99 
> 
> Diff: https://reviews.apache.org/r/52771/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Review Request 52968: Added `mesos-cni-port-mapper` to the CHANGELOG.

[Avinash sridharan]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Added `mesos-cni-port-mapper` to the CHANGELOG.


Diffs
-

  CHANGELOG 004568f9ab6284a004a090944b2c6f15d6018c85 

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


Testing
---


Thanks,

Avinash sridharan

Review Request 52967: Improved documentation for shared persistent volumes.

[Neil Conway]

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

Review request for mesos and Jiang Yan Xu.


Repository: mesos


Description
---

Improved documentation for shared persistent volumes.


Diffs
-

  docs/home.md 1c6b191bd194a9100ce1ad4bf5aff62a20ed8f41 
  docs/persistent-volume.md 5dfbbf1fb08cdf47f97ea6fb286e21ab26235d62 
  docs/shared-resources.md 29e43389c530f816118a32dcd7c2f1b82b0af431 

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


Testing
---

Previewed with site-docker.


Thanks,

Neil Conway

Review Request 52966: Fixed typo in comment.

[Neil Conway]

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

Review request for mesos and Jiang Yan Xu.


Repository: mesos


Description
---

Fixed typo in comment.


Diffs
-

  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
f17ed4437a5e1366f85803ce7e29bee24162504c 

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


Testing
---


Thanks,

Neil Conway

Review Request 52965: Added missing titles to documentation pages.

[Neil Conway]

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

Review request for mesos and Jiang Yan Xu.


Repository: mesos


Description
---

Added missing titles to documentation pages.


Diffs
-

  docs/shared-resources.md 29e43389c530f816118a32dcd7c2f1b82b0af431 
  docs/weights.md c088e585b040ee8b31248650dd478430d2ac4137 

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


Testing
---

Previewed with site-docker.


Thanks,

Neil Conway

Re: Review Request 52659: Changed master to send TASK_DROPPED for task launch errors.

[Vinod Kone]

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


Fix it, then Ship it!





src/tests/master_authorization_tests.cpp (line 743)


TASK_DROPPED



src/tests/master_authorization_tests.cpp (line 781)


This test looks identical to SlaveRemoved test. Please kill this in a 
different review.


- Vinod Kone


On Oct. 7, 2016, 11:52 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52659/
> ---
> 
> (Updated Oct. 7, 2016, 11:52 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6329
> https://issues.apache.org/jira/browse/MESOS-6329
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a task launch fails due to a transient error (e.g., insufficient
> available resources at an agent), the master sends a TASK_LOST update to
> the framework. For PARTITION_AWARE frameworks, we now send TASK_DROPPED
> instead.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 8837f3d55be99626f79ed7f88eee021f115a2889 
>   src/tests/master_authorization_tests.cpp 
> a4623d15c246651fd1038fdedf16321b1d5f273f 
>   src/tests/master_tests.cpp 88cf1e612ad8186ef2cea161b3a52b0df9517305 
> 
> Diff: https://reviews.apache.org/r/52659/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 51617: Added the logic for installing and removing DNAT rules.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (line 482)


No tailing period.



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (line 493)


do period at the end of a log line.


- Jie Yu


On Oct. 17, 2016, 10:56 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51617/
> ---
> 
> (Updated Oct. 17, 2016, 10:56 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6023
> https://issues.apache.org/jira/browse/MESOS-6023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the logic for installing and removing DNAT rules.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
>  7fad707a240234e35828917aea1bc79f42fe130e 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  2ff8b0e76a11b6f6c98b839d3ac91a81e41285f5 
> 
> Diff: https://reviews.apache.org/r/51617/diff/
> 
> 
> Testing
> ---
> 
> Ran the CNI plugin against a network namespace with the following JSON input:
> ```
> {
> "name": "mynet",
> "type": "port-mapper",
> "chain": "MESOS-TEST",
> "excludeDevices": ["mesos-cni0"],
> "delegate": {
>   "type" : "bridge",
>   "bridge": "cni0",
>   "isGateway": true,
>   "ipMasq": true,
>   "ipam": {
>   "type": "host-local",
>   "subnet": "192.168.37.0/24",
>   "routes": [
> { "dst": "0.0.0.0/0" }
>   ]
>   }
> },
> "args" : {
>   "org.apache.mesos" : {
> "network_info" : {
>   "port_mappings": {
> "host_port" : 8080,
> "container_port" : 9000
>   }
> }
>   }
> }
> }
> ```
> 
> Used the ADD command to test that the CNI plugin correctly invokes the 
> delegate plugin (a CNI bridge plugin in this case) and also inserts the 
> correct iptable entries for the given port mapping. After running this 
> plugin, this was the output of the `iptables -t nat -S MESOS-TEST` command:
> ```
> sudo iptables -t nat -S MESOS-TEST
> -N MESOS-TEST
> -A MESOS-TEST ! -i mesos-cni0 -p tcp -m tcp --dport 8080 -j DNAT 
> --to-destination 192.168.37.21:9000
> ```
> 
> Ran a python HTTP server in this network namespace and verified that DNAT 
> works from outside the box. Was able to connect to port 9000 of this server, 
> by connecting to port 8080 on the host.
> 
> Used the DEL command to test the CNI plugin correctly deletes the DNAT rule 
> and chain, if there are no DNAT rules exist in the chain. After running the 
> DEL command (by injecting `NetworkInfo` into the above JSON schema) verified 
> the chain and the DNAT rule is deleted from iptables.
> 
> 
> Apart from these tests ran a single node cluster and did an end-to-end test 
> with a modified `mesos-execute` binary that can setup port-mapping.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 52083: Changed reconciliation for unregistering, reregistering agents.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Oct. 13, 2016, 2:10 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52083/
> ---
> 
> (Updated Oct. 13, 2016, 2:10 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6206
> https://issues.apache.org/jira/browse/MESOS-6206
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, explicit reconciliation for an agent that was in the process
> of reregistering or unregistering returned no results. This degree of
> cleverness seems unwarranted: if the agent hasn't completed the
> reregistration or unregistration process, it seems simpler for the
> master to return the previous state of the agent. This is what the
> framework would observe if their reconcile request lost the race with
> the reregister/unregister operation, anyway.
> 
> Note that since reregistering agents are no longer considered to be "in
> transition", we need to slightly adjust the rules for how we update the
> `slaves.recovered` collection in the master: an agent remains in the
> "recovered" collection until it has been marked reachable in the
> registry (rather than removing it from "recovered" as soon as the
> reregistration process beings). This is more consistent with how we
> manage the other collections in the master anyway: an agent appears in
> the `recovered` list until the registry operation that reregisters it
> has been successfully applied.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 43518b9bf1bfaa54e26acc7f2e70c4161c667a84 
>   src/master/master.cpp 7ef898781ce5c2349ffeaa3ce43e68dede19c852 
>   src/tests/master_tests.cpp 88cf1e612ad8186ef2cea161b3a52b0df9517305 
>   src/tests/reconciliation_tests.cpp 1412090299df388456f04ed58a1d384ce3ff550a 
> 
> Diff: https://reviews.apache.org/r/52083/diff/
> 
> 
> Testing
> ---
> 
> `make check` on OSX, Linux.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 52771: Captured the `stderr` during execution of CNI plugin.

[Avinash sridharan]

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

(Updated Oct. 18, 2016, 12:02 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description
---

Till now we were capturing only the `stdout` of the CNI plugin. However,
during errors it makes sense to add the output from `stderr` for the
CNI plugin as well, since as per the CNI spec the CNI plugin is
supposed to output all unstructured output (debugs for example) to
`stderr`. This greatly helps in debugging the exact reason for CNI
plugin failures.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
70f30831819d7a0e6233fcb13a703dc6981324b6 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
7c35c3056326a8a135e4ad944ac741cda96cab99 

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


Testing
---

make and make check.


Thanks,

Avinash sridharan

Re: Review Request 52809: User Namespaces Initial Implementation.

[Srinivas Brahmaroutu]

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

(Updated Oct. 17, 2016, 11:59 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Summary (updated)
-

User Namespaces Initial Implementation.


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


Repository: mesos


Description (updated)
---

Work in progress : implementing User namespaces.
Phase 1: Create isolator and enable isolator to when Agent is
  run with "userns=true". If this flags is not set the original
  functionality will run the task as user who started the task.
  With the flag set to true, the task will be run inside the user
  namespace as a root inside the container and task is run as the
  user who started the task when seen from outside of the container.
  Approriate uid and gid maps are created.
Phase 2: Provide mount point support for containers running in
  user namespace. This will allow to properly mount and access
  the filesystems with proper permission.


Diffs (updated)
-

  src/Makefile.am 3bcc0f2dfc2c4f71841bd6d161f39e0e919fc0d7 
  src/slave/containerizer/mesos/containerizer.cpp 
eac70d955e08142a2d054039d610a3d516b1b57e 
  src/slave/containerizer/mesos/isolators/user/user.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/user/user.cpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/user/usermaps.hpp PRE-CREATION 
  src/slave/containerizer/mesos/launch.cpp 
8a30ff8bd6f9263d68a4344b79f2374a2ae53c04 
  src/slave/flags.hpp 3c292bac9394347318865f49782907def6541742 
  src/slave/flags.cpp 87d9e4632321134192bb0a67f1b91db7d89f539b 

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


Testing
---

Work in progress implementing User namespaces.
Phase 1: Create isolator and enable isolator to when Agent is run with 
"userns=true". If this flags is not set the original functionality will run the 
task as user who started the task. With User namespace the task will be run 
inside the user namespace with as a root with the user who started the task is 
mapped to outside of the container. Approriate uid and gid maps are created.
Phase 2: Provide mount point support for containers running in user namespace.


Thanks,

Srinivas Brahmaroutu

Re: Review Request 52810: Added tests to test usernamespaces.

[Srinivas Brahmaroutu]

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

(Updated Oct. 17, 2016, 11:58 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Repository: mesos


Description (updated)
---

Basic testing to run the task as Root and Non-Root
when user namespace is enabled.


Diffs (updated)
-

  src/tests/containerizer/isolator_tests.cpp 
8fefeef8c83ed2ab01f56a1ec572d3acb307143c 

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


Testing
---


Thanks,

Srinivas Brahmaroutu

Re: Review Request 52543: Added configure/make options to build the new CLI and run unit tests.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52939, 52940, 52941, 52942, 52943, 52944, 52945, 52946, 
52947, 51008, 52948, 52950, 52951, 52952, 52953, 52543]

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 Oct. 17, 2016, 8:26 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52543/
> ---
> 
> (Updated Oct. 17, 2016, 8:26 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-6008 and MESOS-6032
> https://issues.apache.org/jira/browse/MESOS-6008
> https://issues.apache.org/jira/browse/MESOS-6032
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added configure/make options to build the new CLI and run unit tests.
> 
> 
> Diffs
> -
> 
>   configure.ac 015255ec1876a51b9eb2cf488b375af90f73c722 
>   docs/getting-started.md de3fba4a821ca7fa9b9e2fd692b33b40edd35b68 
>   src/Makefile.am 3bcc0f2dfc2c4f71841bd6d161f39e0e919fc0d7 
> 
> Diff: https://reviews.apache.org/r/52543/diff/
> 
> 
> Testing
> ---
> 
> ../configure --enable-new-cli
> make -C src mesos
> src/mesos
> 
> GTEST_FILTER="" make -j check
> sudo GTEST_FILTER="" make -j check
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52771: Captured the `stderr` during execution of CNI plugin.

[Avinash sridharan]

> On Oct. 17, 2016, 11:30 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 1507-1508
> > 
> >
> > Looks like CNI spec does not specify if stdout will be empty or not. 
> > Let's just print all of them like you did in the attach code.

Agreed.
Actually in case of an error, the CNI error is going to be thrown in the 
stdout, so should capturing stdout as well.


- Avinash


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


On Oct. 12, 2016, 6:27 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52771/
> ---
> 
> (Updated Oct. 12, 2016, 6:27 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6282
> https://issues.apache.org/jira/browse/MESOS-6282
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Till now we were capturing only the `stdout` of the CNI plugin. However,
> during errors it makes sense to add the output from `stderr` for the
> CNI plugin as well, since as per the CNI spec the CNI plugin is
> supposed to output all unstructured output (debugs for example) to
> `stderr`. This greatly helps in debugging the exact reason for CNI
> plugin failures.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 70f30831819d7a0e6233fcb13a703dc6981324b6 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 1b22b28825e8160f659c3cbac37cc576f01666d5 
> 
> Diff: https://reviews.apache.org/r/52771/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 52771: Captured the `stderr` during execution of CNI plugin.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 1231)


Just to be consistent with `output` above, let's use `error` here.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 1507 - 1508)


Looks like CNI spec does not specify if stdout will be empty or not. Let's 
just print all of them like you did in the attach code.


- Jie Yu


On Oct. 12, 2016, 6:27 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52771/
> ---
> 
> (Updated Oct. 12, 2016, 6:27 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6282
> https://issues.apache.org/jira/browse/MESOS-6282
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Till now we were capturing only the `stdout` of the CNI plugin. However,
> during errors it makes sense to add the output from `stderr` for the
> CNI plugin as well, since as per the CNI spec the CNI plugin is
> supposed to output all unstructured output (debugs for example) to
> `stderr`. This greatly helps in debugging the exact reason for CNI
> plugin failures.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 70f30831819d7a0e6233fcb13a703dc6981324b6 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 1b22b28825e8160f659c3cbac37cc576f01666d5 
> 
> Diff: https://reviews.apache.org/r/52771/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 52658: Changed scheduler driver to send TASK_DROPPED.

[Vinod Kone]

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


Fix it, then Ship it!





src/sched/sched.cpp (line 1305)


back ticks around `launchTasks`


- Vinod Kone


On Oct. 7, 2016, 11:51 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52658/
> ---
> 
> (Updated Oct. 7, 2016, 11:51 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6331
> https://issues.apache.org/jira/browse/MESOS-6331
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a scheduler tries to launch a task when the scheduler driver is not
> connected to the master, the scheduler driver creates a faux TASK_LOST
> status update to indicate that the task launch has not succeeded. If the
> framework is PARTITION_AWARE, the scheduler driver will now send
> TASK_DROPPED instead.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp 9d1b5ce2e1a179b2e6ea212d99d8d7fe72a0793a 
>   src/tests/fault_tolerance_tests.cpp 
> 5a9944cf459ab688907d95bbda09f464b37efd1e 
> 
> Diff: https://reviews.apache.org/r/52658/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 52092: Avoided to concat cgroup internally in subsystems.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 572)


I would use containerId here.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 573)


Ditto.


- Jie Yu


On Oct. 11, 2016, 3:53 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52092/
> ---
> 
> (Updated Oct. 11, 2016, 3:53 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6218
> https://issues.apache.org/jira/browse/MESOS-6218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To avoid using `path::join(flags.cgroups_root, containerId.value())` to
> concat cgroup internally in subsystems, pass cgroup to the subsystem
> interfaces directly.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 9359ceff3f1f6204822b511b46d62d2fceec25e4 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 
> 6594110bd45a71c6d41a24b3f5b73c4a4e3a7335 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 
> 05cb8b8efadd2355789d08ce5c9da9d3de0fc72a 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 
> c1f1004d7578f59dfa9e28323e7a55df41c4cb6b 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
> 5912c6504a375178b892bf8099d5f75fa9e91c05 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp 
> 10fbbc9cb39a3b41d64cc090b0e890a2416b2306 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 
> a35ecb073acefb903d7e8c4737d6cd048a2b494d 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 
> b5d5ed233c0c8ddc339d2550c0f57a02dd3f14c3 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 
> 0afc24867eb0b1949e95b3f5a8914be013dbf531 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 
> a1c87ce2ace33f1a779e843578f55d7502562c8d 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 
> 5b3ce15c209f6ce0e430386d97bc6768fca805c8 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp 
> 16208655e16b81f1e4bbf97bf5e32e75f4c3f3a9 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp 
> 93539f1fc8265f66b62294c22f4eaba704b8b876 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 
> 5a0adfdb8705ae6b20c61a827380142e418c0ae4 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.cpp 
> cc39f287a64a4125260597e74784dc0847953376 
> 
> Diff: https://reviews.apache.org/r/52092/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52657: Clarified a comment that occurs in several tests.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Oct. 7, 2016, 11:50 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52657/
> ---
> 
> (Updated Oct. 7, 2016, 11:50 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Clarified a comment that occurs in several tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_authorization_tests.cpp 
> a4623d15c246651fd1038fdedf16321b1d5f273f 
>   src/tests/master_validation_tests.cpp 
> 99e350e0587e73e9ee25ef20dd369cd146bd446a 
> 
> Diff: https://reviews.apache.org/r/52657/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 52656: Cleaned up a test case.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Oct. 7, 2016, 11:50 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52656/
> ---
> 
> (Updated Oct. 7, 2016, 11:50 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleaned up a test case.
> 
> 
> Diffs
> -
> 
>   src/tests/fault_tolerance_tests.cpp 
> 5a9944cf459ab688907d95bbda09f464b37efd1e 
> 
> Diff: https://reviews.apache.org/r/52656/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 51617: Added the logic for installing and removing DNAT rules.

[Avinash sridharan]

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

(Updated Oct. 17, 2016, 10:56 p.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Fixed some comments.


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


Repository: mesos


Description
---

Added the logic for installing and removing DNAT rules.


Diffs (updated)
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
 7fad707a240234e35828917aea1bc79f42fe130e 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 2ff8b0e76a11b6f6c98b839d3ac91a81e41285f5 

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


Testing
---

Ran the CNI plugin against a network namespace with the following JSON input:
```
{
"name": "mynet",
"type": "port-mapper",
"chain": "MESOS-TEST",
"excludeDevices": ["mesos-cni0"],
"delegate": {
  "type" : "bridge",
  "bridge": "cni0",
  "isGateway": true,
  "ipMasq": true,
  "ipam": {
  "type": "host-local",
  "subnet": "192.168.37.0/24",
  "routes": [
{ "dst": "0.0.0.0/0" }
  ]
  }
},
"args" : {
  "org.apache.mesos" : {
"network_info" : {
  "port_mappings": {
"host_port" : 8080,
"container_port" : 9000
  }
}
  }
}
}
```

Used the ADD command to test that the CNI plugin correctly invokes the delegate 
plugin (a CNI bridge plugin in this case) and also inserts the correct iptable 
entries for the given port mapping. After running this plugin, this was the 
output of the `iptables -t nat -S MESOS-TEST` command:
```
sudo iptables -t nat -S MESOS-TEST
-N MESOS-TEST
-A MESOS-TEST ! -i mesos-cni0 -p tcp -m tcp --dport 8080 -j DNAT 
--to-destination 192.168.37.21:9000
```

Ran a python HTTP server in this network namespace and verified that DNAT works 
from outside the box. Was able to connect to port 9000 of this server, by 
connecting to port 8080 on the host.

Used the DEL command to test the CNI plugin correctly deletes the DNAT rule and 
chain, if there are no DNAT rules exist in the chain. After running the DEL 
command (by injecting `NetworkInfo` into the above JSON schema) verified the 
chain and the DNAT rule is deleted from iptables.


Apart from these tests ran a single node cluster and did an end-to-end test 
with a modified `mesos-execute` binary that can setup port-mapping.


Thanks,

Avinash sridharan

Re: Review Request 40266: Libprocess Reinit: Cleanup SocketManager alongside ProcessManager.

[Vinod Kone]

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


Ship it!





3rdparty/libprocess/src/process.cpp (line 2858)


s/Attempted/Ignoring the attempt/ ?


- Vinod Kone


On Oct. 12, 2016, 8:48 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40266/
> ---
> 
> (Updated Oct. 12, 2016, 8:48 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Artem Harutyunyan, Joris Van Remoortere, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-3910
> https://issues.apache.org/jira/browse/MESOS-3910
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `SocketManager` and `ProcessManager` are highly inter-dependent, 
> which requires some untangling in `process::finalize`.
> 
> * Logic originally found in `~ProcessManager` has been split into 
>   `ProcessManager::finalize` due to what happens during cleanup.
> * The future from `__s__->accept()` must be explicitly discarded as 
>   libevent does not detect a locally closed socket.
> * Terminating `HttpProxy`s must close the associated socket.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> f1d746c52cfe659f5cd7da4b7a6424ff585619a3 
> 
> Diff: https://reviews.apache.org/r/40266/diff/
> 
> 
> Testing
> ---
> 
> `make check` (libev)
> `make check` (--enable-libevent --enable-ssl)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 51617: Added the logic for installing and removing DNAT rules.

[Avinash sridharan]

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

(Updated Oct. 17, 2016, 10:27 p.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Rebased and fixed some comments.


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


Repository: mesos


Description
---

Added the logic for installing and removing DNAT rules.


Diffs (updated)
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
 7fad707a240234e35828917aea1bc79f42fe130e 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 2ff8b0e76a11b6f6c98b839d3ac91a81e41285f5 

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


Testing
---

Ran the CNI plugin against a network namespace with the following JSON input:
```
{
"name": "mynet",
"type": "port-mapper",
"chain": "MESOS-TEST",
"excludeDevices": ["mesos-cni0"],
"delegate": {
  "type" : "bridge",
  "bridge": "cni0",
  "isGateway": true,
  "ipMasq": true,
  "ipam": {
  "type": "host-local",
  "subnet": "192.168.37.0/24",
  "routes": [
{ "dst": "0.0.0.0/0" }
  ]
  }
},
"args" : {
  "org.apache.mesos" : {
"network_info" : {
  "port_mappings": {
"host_port" : 8080,
"container_port" : 9000
  }
}
  }
}
}
```

Used the ADD command to test that the CNI plugin correctly invokes the delegate 
plugin (a CNI bridge plugin in this case) and also inserts the correct iptable 
entries for the given port mapping. After running this plugin, this was the 
output of the `iptables -t nat -S MESOS-TEST` command:
```
sudo iptables -t nat -S MESOS-TEST
-N MESOS-TEST
-A MESOS-TEST ! -i mesos-cni0 -p tcp -m tcp --dport 8080 -j DNAT 
--to-destination 192.168.37.21:9000
```

Ran a python HTTP server in this network namespace and verified that DNAT works 
from outside the box. Was able to connect to port 9000 of this server, by 
connecting to port 8080 on the host.

Used the DEL command to test the CNI plugin correctly deletes the DNAT rule and 
chain, if there are no DNAT rules exist in the chain. After running the DEL 
command (by injecting `NetworkInfo` into the above JSON schema) verified the 
chain and the DNAT rule is deleted from iptables.


Apart from these tests ran a single node cluster and did an end-to-end test 
with a modified `mesos-execute` binary that can setup port-mapping.


Thanks,

Avinash sridharan

Re: Review Request 52810: Added tests to test usernamespaces.

[Srinivas Brahmaroutu]

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

(Updated Oct. 17, 2016, 10:27 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Repository: mesos


Description
---

Added tests to test usernamespaces.


Diffs (updated)
-

  src/tests/containerizer/isolator_tests.cpp 
8fefeef8c83ed2ab01f56a1ec572d3acb307143c 

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


Testing
---


Thanks,

Srinivas Brahmaroutu

Re: Review Request 52810: Added tests to test usernamespaces.

[Srinivas Brahmaroutu]

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

(Updated Oct. 17, 2016, 10:22 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Repository: mesos


Description (updated)
---

Added tests to test usernamespaces.


Diffs (updated)
-

  src/tests/containerizer/isolator_tests.cpp 
8fefeef8c83ed2ab01f56a1ec572d3acb307143c 

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


Testing
---


Thanks,

Srinivas Brahmaroutu

Re: Review Request 52783: Added documentation for mesos-containerizer Linux capabilities support.

[Jie Yu]

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


Fix it, then Ship it!




Thanks!!!


docs/linux_capabilities.md (line 11)


s/had/hand/?



docs/linux_capabilities.md (line 44)


I'd also discuss the case where the task specifies some capabilities that 
are not in 'allowed_capabilities', what will happen.


- Jie Yu


On Oct. 17, 2016, 1:16 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52783/
> ---
> 
> (Updated Oct. 17, 2016, 1:16 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6376
> https://issues.apache.org/jira/browse/MESOS-6376
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for mesos-containerizer Linux capabilities support.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md c83a58eb6884c8d8c37880a745e04cf0b789ebdc 
>   docs/linux_capabilities.md PRE-CREATION 
>   docs/mesos-containerizer.md 76544625838845fe2817347fea483a60ab250f99 
> 
> Diff: https://reviews.apache.org/r/52783/diff/
> 
> 
> Testing
> ---
> 
> Checked with local renderer.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 52780: Added input and output functions for v1::CapabilityInfo.

[Jie Yu]

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




src/v1/mesos.cpp (lines 517 - 533)


Can you follow the same impl in v0?


- Jie Yu


On Oct. 12, 2016, 12:49 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52780/
> ---
> 
> (Updated Oct. 12, 2016, 12:49 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added input and output functions for v1::CapabilityInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/mesos.hpp f17145857355d7b85d70b6452bf1f89bf54edbac 
>   src/common/parse.hpp 62a333cc1f287439c761aa6f0c7f0bf7ade4a818 
>   src/v1/mesos.cpp 0f88abaf49117e844eeb3654edabeeba3862a0eb 
> 
> Diff: https://reviews.apache.org/r/52780/diff/
> 
> 
> Testing
> ---
> 
> * confirmed identically configured unversioned and `v1` `CapabilityInfo`s 
> produce identical output,
> * used and tested implicitly as part of https://reviews.apache.org/r/52741/
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51617: Added the logic for installing and removing DNAT rules.

[Avinash sridharan]

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

(Updated Oct. 17, 2016, 10:11 p.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description
---

Added the logic for installing and removing DNAT rules.


Diffs (updated)
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
 7fad707a240234e35828917aea1bc79f42fe130e 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 2ff8b0e76a11b6f6c98b839d3ac91a81e41285f5 

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


Testing
---

Ran the CNI plugin against a network namespace with the following JSON input:
```
{
"name": "mynet",
"type": "port-mapper",
"chain": "MESOS-TEST",
"excludeDevices": ["mesos-cni0"],
"delegate": {
  "type" : "bridge",
  "bridge": "cni0",
  "isGateway": true,
  "ipMasq": true,
  "ipam": {
  "type": "host-local",
  "subnet": "192.168.37.0/24",
  "routes": [
{ "dst": "0.0.0.0/0" }
  ]
  }
},
"args" : {
  "org.apache.mesos" : {
"network_info" : {
  "port_mappings": {
"host_port" : 8080,
"container_port" : 9000
  }
}
  }
}
}
```

Used the ADD command to test that the CNI plugin correctly invokes the delegate 
plugin (a CNI bridge plugin in this case) and also inserts the correct iptable 
entries for the given port mapping. After running this plugin, this was the 
output of the `iptables -t nat -S MESOS-TEST` command:
```
sudo iptables -t nat -S MESOS-TEST
-N MESOS-TEST
-A MESOS-TEST ! -i mesos-cni0 -p tcp -m tcp --dport 8080 -j DNAT 
--to-destination 192.168.37.21:9000
```

Ran a python HTTP server in this network namespace and verified that DNAT works 
from outside the box. Was able to connect to port 9000 of this server, by 
connecting to port 8080 on the host.

Used the DEL command to test the CNI plugin correctly deletes the DNAT rule and 
chain, if there are no DNAT rules exist in the chain. After running the DEL 
command (by injecting `NetworkInfo` into the above JSON schema) verified the 
chain and the DNAT rule is deleted from iptables.


Apart from these tests ran a single node cluster and did an end-to-end test 
with a modified `mesos-execute` binary that can setup port-mapping.


Thanks,

Avinash sridharan

Re: Review Request 52741: Added capabilities support to mesos-execute.

[Jie Yu]

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


Fix it, then Ship it!





src/cli/execute.cpp (line 690)


Why the extra `()`?


- Jie Yu


On Oct. 12, 2016, 2:31 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52741/
> ---
> 
> (Updated Oct. 12, 2016, 2:31 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5303
> https://issues.apache.org/jira/browse/MESOS-5303
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces a flags `capabilities` to mesos-execute which
> can be used to specify the capabilities a task should be able to
> aquire.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp fcf627b0dcbedd01700cc8c9acadf7ba0dae4faa 
> 
> Diff: https://reviews.apache.org/r/52741/diff/
> 
> 
> Testing
> ---
> 
> * `make check`, including `ROOT` tests,
> * tested as root with a `ping` task with an agent where both the task and the 
> agent have different or no capabilities configured.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 52682: Cleaned up a few style issues in the capabilities isolator tests.

[Jie Yu]

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


Fix it, then Ship it!





src/tests/containerizer/linux_capabilities_isolator_tests.cpp (line 99)


const UsaImage&



src/tests/containerizer/linux_capabilities_isolator_tests.cpp (line 100)


const Result&


- Jie Yu


On Oct. 10, 2016, 11:23 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52682/
> ---
> 
> (Updated Oct. 10, 2016, 11:23 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We remove an unused using declaration, and replace several bool
> parameters with more readable enum values.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/linux_capabilities_isolator_tests.cpp 
> f040c209b4b4c87cef00b0569b7da7581f4ccf03 
> 
> Diff: https://reviews.apache.org/r/52682/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51785: [HBASE-16608] Merge for the segments in the compaction pipeline and simplifying the user interface for in-memory flush

[Anastasia Braginsky]

> On Oct. 15, 2016, 3:18 a.m., Michael Stack wrote:
> > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactor.java,
> >  line 229
> > 
> >
> > This is too many threads. An executor of a fixed amount of threads 
> > instead?

We have executor from RegionServices (it is invoked in CompactingMemStore). The 
total number of threads is bounded.


- Anastasia


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


On Oct. 14, 2016, 7:19 a.m., Anastasia Braginsky wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51785/
> ---
> 
> (Updated Oct. 14, 2016, 7:19 a.m.)
> 
> 
> Review request for hbase.
> 
> 
> Repository: hbase-git
> 
> 
> Description
> ---
> 
> This is a step toward final compacting memstore that allowes two modes of 
> work: index-compaction and data-compaction. 
> 
> The index-compaction means that when the new segment is pushed into the 
> pipeline, it is flattened and probably merged with old segments in the 
> pipeline. The new merge "feature" induces no data-copy-compaction and no 
> speculative SQM scan. 
> The compacting memstore of the data-compaction type means the usage of the 
> data-copy-compaction.
> 
> 
> Diffs
> -
> 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactingMemStore.java
>  177f222 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactionPipeline.java
>  6a13f43 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java
>  378601d 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ImmutableSegment.java
>  12b7916 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactor.java
>  714ffe3 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorIterator.java
>  9798ec2 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreMergerSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SegmentFactory.java
>  510ebbd 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/VersionedSegmentsList.java
>  2e8bead 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingMemStore.java
>  211a6d8 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingToCellArrayMapMemStore.java
>  f89a040 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStoreLAB.java
>  1ea5112 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestPerColumnFamilyFlush.java
>  6bfaa59 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestWalAndCompactingMemStoreFlush.java
>  74826b0 
> 
> Diff: https://reviews.apache.org/r/51785/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anastasia Braginsky
> 
>

Re: Review Request 51785: [HBASE-16608] Merge for the segments in the compaction pipeline and simplifying the user interface for in-memory flush

[Anastasia Braginsky]

> On Oct. 15, 2016, 3:18 a.m., Michael Stack wrote:
> > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactor.java,
> >  line 186
> > 
> >
> > s/isInterrupted/interrupted/... its a variable? not a method?

isInterrupted is a variable. Now wrapped it with a method.


- Anastasia


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


On Oct. 14, 2016, 7:19 a.m., Anastasia Braginsky wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51785/
> ---
> 
> (Updated Oct. 14, 2016, 7:19 a.m.)
> 
> 
> Review request for hbase.
> 
> 
> Repository: hbase-git
> 
> 
> Description
> ---
> 
> This is a step toward final compacting memstore that allowes two modes of 
> work: index-compaction and data-compaction. 
> 
> The index-compaction means that when the new segment is pushed into the 
> pipeline, it is flattened and probably merged with old segments in the 
> pipeline. The new merge "feature" induces no data-copy-compaction and no 
> speculative SQM scan. 
> The compacting memstore of the data-compaction type means the usage of the 
> data-copy-compaction.
> 
> 
> Diffs
> -
> 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactingMemStore.java
>  177f222 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactionPipeline.java
>  6a13f43 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java
>  378601d 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ImmutableSegment.java
>  12b7916 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactor.java
>  714ffe3 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorIterator.java
>  9798ec2 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreMergerSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SegmentFactory.java
>  510ebbd 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/VersionedSegmentsList.java
>  2e8bead 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingMemStore.java
>  211a6d8 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingToCellArrayMapMemStore.java
>  f89a040 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStoreLAB.java
>  1ea5112 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestPerColumnFamilyFlush.java
>  6bfaa59 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestWalAndCompactingMemStoreFlush.java
>  74826b0 
> 
> Diff: https://reviews.apache.org/r/51785/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anastasia Braginsky
> 
>

Re: Review Request 51785: [HBASE-16608] Merge for the segments in the compaction pipeline and simplifying the user interface for in-memory flush

[Anastasia Braginsky]

> On Oct. 15, 2016, 3:18 a.m., Michael Stack wrote:
> > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ImmutableSegment.java,
> >  line 88
> > 
> >
> > Why we have a MemStoreSegmentsIterator and not just an Iterator? Does 
> > it do something special?

This iterator is getting list of Segments and iterates over the Cells in those 
segments in two different ways. But you are right if we use Segment and not 
MemStoreSegment, I'll rename this to SegmentsIterator.


> On Oct. 15, 2016, 3:18 a.m., Michael Stack wrote:
> > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactor.java,
> >  line 111
> > 
> >
> > Higher in the patch you have initiateType ... looks like it should be 
> > initiateAction?

OK. Changed the initiateType() of the CompactingMemStore to be initiateAction()


- Anastasia


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


On Oct. 14, 2016, 7:19 a.m., Anastasia Braginsky wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51785/
> ---
> 
> (Updated Oct. 14, 2016, 7:19 a.m.)
> 
> 
> Review request for hbase.
> 
> 
> Repository: hbase-git
> 
> 
> Description
> ---
> 
> This is a step toward final compacting memstore that allowes two modes of 
> work: index-compaction and data-compaction. 
> 
> The index-compaction means that when the new segment is pushed into the 
> pipeline, it is flattened and probably merged with old segments in the 
> pipeline. The new merge "feature" induces no data-copy-compaction and no 
> speculative SQM scan. 
> The compacting memstore of the data-compaction type means the usage of the 
> data-copy-compaction.
> 
> 
> Diffs
> -
> 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactingMemStore.java
>  177f222 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactionPipeline.java
>  6a13f43 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java
>  378601d 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ImmutableSegment.java
>  12b7916 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactor.java
>  714ffe3 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorIterator.java
>  9798ec2 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreMergerSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SegmentFactory.java
>  510ebbd 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/VersionedSegmentsList.java
>  2e8bead 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingMemStore.java
>  211a6d8 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingToCellArrayMapMemStore.java
>  f89a040 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStoreLAB.java
>  1ea5112 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestPerColumnFamilyFlush.java
>  6bfaa59 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestWalAndCompactingMemStoreFlush.java
>  74826b0 
> 
> Diff: https://reviews.apache.org/r/51785/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anastasia Braginsky
> 
>

Re: Review Request 51785: [HBASE-16608] Merge for the segments in the compaction pipeline and simplifying the user interface for in-memory flush

[Anastasia Braginsky]

> On Oct. 15, 2016, 3:18 a.m., Michael Stack wrote:
> > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java,
> >  line 120
> > 
> >
> > Has to be public?

This method is used by SegmentFactory to build the new ImmutableSegment, which 
is a result of merging all segments in the compacting pipeline.


- Anastasia


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


On Oct. 14, 2016, 7:19 a.m., Anastasia Braginsky wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51785/
> ---
> 
> (Updated Oct. 14, 2016, 7:19 a.m.)
> 
> 
> Review request for hbase.
> 
> 
> Repository: hbase-git
> 
> 
> Description
> ---
> 
> This is a step toward final compacting memstore that allowes two modes of 
> work: index-compaction and data-compaction. 
> 
> The index-compaction means that when the new segment is pushed into the 
> pipeline, it is flattened and probably merged with old segments in the 
> pipeline. The new merge "feature" induces no data-copy-compaction and no 
> speculative SQM scan. 
> The compacting memstore of the data-compaction type means the usage of the 
> data-copy-compaction.
> 
> 
> Diffs
> -
> 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactingMemStore.java
>  177f222 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactionPipeline.java
>  6a13f43 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java
>  378601d 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ImmutableSegment.java
>  12b7916 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactor.java
>  714ffe3 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorIterator.java
>  9798ec2 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreMergerSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SegmentFactory.java
>  510ebbd 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/VersionedSegmentsList.java
>  2e8bead 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingMemStore.java
>  211a6d8 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingToCellArrayMapMemStore.java
>  f89a040 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStoreLAB.java
>  1ea5112 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestPerColumnFamilyFlush.java
>  6bfaa59 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestWalAndCompactingMemStoreFlush.java
>  74826b0 
> 
> Diff: https://reviews.apache.org/r/51785/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anastasia Braginsky
> 
>

Re: Review Request 51785: [HBASE-16608] Merge for the segments in the compaction pipeline and simplifying the user interface for in-memory flush

[Anastasia Braginsky]

> On Oct. 15, 2016, 3:18 a.m., Michael Stack wrote:
> > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java,
> >  line 91
> > 
> >
> > Does this need to be atomic reference or volatile or something? Single 
> > thread access or mutiple threads in here?

This field (nextMSLAB) is read and written only under lock for the entire MSLAB 
object (under synchronized). So this is OK for multi-thread-ed access.


- Anastasia


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


On Oct. 14, 2016, 7:19 a.m., Anastasia Braginsky wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51785/
> ---
> 
> (Updated Oct. 14, 2016, 7:19 a.m.)
> 
> 
> Review request for hbase.
> 
> 
> Repository: hbase-git
> 
> 
> Description
> ---
> 
> This is a step toward final compacting memstore that allowes two modes of 
> work: index-compaction and data-compaction. 
> 
> The index-compaction means that when the new segment is pushed into the 
> pipeline, it is flattened and probably merged with old segments in the 
> pipeline. The new merge "feature" induces no data-copy-compaction and no 
> speculative SQM scan. 
> The compacting memstore of the data-compaction type means the usage of the 
> data-copy-compaction.
> 
> 
> Diffs
> -
> 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactingMemStore.java
>  177f222 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactionPipeline.java
>  6a13f43 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java
>  378601d 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ImmutableSegment.java
>  12b7916 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactor.java
>  714ffe3 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorIterator.java
>  9798ec2 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreMergerSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SegmentFactory.java
>  510ebbd 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/VersionedSegmentsList.java
>  2e8bead 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingMemStore.java
>  211a6d8 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingToCellArrayMapMemStore.java
>  f89a040 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStoreLAB.java
>  1ea5112 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestPerColumnFamilyFlush.java
>  6bfaa59 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestWalAndCompactingMemStoreFlush.java
>  74826b0 
> 
> Diff: https://reviews.apache.org/r/51785/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anastasia Braginsky
> 
>

Re: Review Request 51785: [HBASE-16608] Merge for the segments in the compaction pipeline and simplifying the user interface for in-memory flush

[Anastasia Braginsky]

> On Oct. 15, 2016, 3:18 a.m., Michael Stack wrote:
> > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactionPipeline.java,
> >  line 93
> > 
> >
> > Why not do this always? Why an option?

This is exactly the merge case. As you can see when merging the segments in the 
pipeline we do not copy the data. Meaning we create a new segment with new 
MSLAB, but this new MSLAB is referencing the chunks also referenced by the 
MSLABs of the old segments. We can not close any MSLAB in this chain of merges 
as they all are referencing the same chunks with data. The last MSLAB will be 
closed when the flush to disk will come.


> On Oct. 15, 2016, 3:18 a.m., Michael Stack wrote:
> > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactionPipeline.java,
> >  line 213
> > 
> >
> > Yeah, why this optional at all? Why not close all the time.

Explained in the comment above.


- Anastasia


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


On Oct. 14, 2016, 7:19 a.m., Anastasia Braginsky wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51785/
> ---
> 
> (Updated Oct. 14, 2016, 7:19 a.m.)
> 
> 
> Review request for hbase.
> 
> 
> Repository: hbase-git
> 
> 
> Description
> ---
> 
> This is a step toward final compacting memstore that allowes two modes of 
> work: index-compaction and data-compaction. 
> 
> The index-compaction means that when the new segment is pushed into the 
> pipeline, it is flattened and probably merged with old segments in the 
> pipeline. The new merge "feature" induces no data-copy-compaction and no 
> speculative SQM scan. 
> The compacting memstore of the data-compaction type means the usage of the 
> data-copy-compaction.
> 
> 
> Diffs
> -
> 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactingMemStore.java
>  177f222 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactionPipeline.java
>  6a13f43 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java
>  378601d 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ImmutableSegment.java
>  12b7916 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactor.java
>  714ffe3 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorIterator.java
>  9798ec2 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreMergerSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SegmentFactory.java
>  510ebbd 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/VersionedSegmentsList.java
>  2e8bead 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingMemStore.java
>  211a6d8 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingToCellArrayMapMemStore.java
>  f89a040 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStoreLAB.java
>  1ea5112 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestPerColumnFamilyFlush.java
>  6bfaa59 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestWalAndCompactingMemStoreFlush.java
>  74826b0 
> 
> Diff: https://reviews.apache.org/r/51785/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anastasia Braginsky
> 
>

Re: Review Request 51785: [HBASE-16608] Merge for the segments in the compaction pipeline and simplifying the user interface for in-memory flush

[Anastasia Braginsky]

> On Oct. 15, 2016, 3:18 a.m., Michael Stack wrote:
> > Patch looks fine to me. What was the fix? A few comments in the below. How 
> > to test? Thanks Anastasia.

The recent bug was in merging MSLABs. We didn't reference the open scan 
counters from the old MSLABs to the new merged MSLAB. No we take care for those 
counter correctly. We have added many tests to the regression while writing the 
code. Also we run multiple YCSB benchmarks and PE tool (which means stress 
tests). For further testing I can suggest add some more tests to the regression 
and to run more PE tool.


- Anastasia


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


On Oct. 14, 2016, 7:19 a.m., Anastasia Braginsky wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51785/
> ---
> 
> (Updated Oct. 14, 2016, 7:19 a.m.)
> 
> 
> Review request for hbase.
> 
> 
> Repository: hbase-git
> 
> 
> Description
> ---
> 
> This is a step toward final compacting memstore that allowes two modes of 
> work: index-compaction and data-compaction. 
> 
> The index-compaction means that when the new segment is pushed into the 
> pipeline, it is flattened and probably merged with old segments in the 
> pipeline. The new merge "feature" induces no data-copy-compaction and no 
> speculative SQM scan. 
> The compacting memstore of the data-compaction type means the usage of the 
> data-copy-compaction.
> 
> 
> Diffs
> -
> 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactingMemStore.java
>  177f222 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactionPipeline.java
>  6a13f43 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java
>  378601d 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ImmutableSegment.java
>  12b7916 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactor.java
>  714ffe3 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorIterator.java
>  9798ec2 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreMergerSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SegmentFactory.java
>  510ebbd 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/VersionedSegmentsList.java
>  2e8bead 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingMemStore.java
>  211a6d8 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingToCellArrayMapMemStore.java
>  f89a040 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStoreLAB.java
>  1ea5112 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestPerColumnFamilyFlush.java
>  6bfaa59 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestWalAndCompactingMemStoreFlush.java
>  74826b0 
> 
> Diff: https://reviews.apache.org/r/51785/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anastasia Braginsky
> 
>

Re: Review Request 52934: Explain read-only mode of persistent volumes in shared-resources.md.

[Jiang Yan Xu]

> On Oct. 17, 2016, 11:22 a.m., Neil Conway wrote:
> > The mode of a volume doesn't necessarily have anything to do with whether 
> > the volume is shared -- perhaps it would be better to document this in the 
> > persistent-volume doc page, and link to it from here?

It makes sense to have a section in persistent-volumes.md to describe the 
general case for launching tasks with the volumes (e.g., you can actually 
change `container_path` when using it)

I am thinking of doing some improvements on persistent volumes (MESOS-6374) 
that may change that section (if we write one) and figured we can update it 
afterwards.

Here in this doc I mostly wanted to talk about from the user's perspective why 
and when you would want to use shared read-only persistent volumes (which 
doesn't make sense to regular persistent volumes) so I think this paragraph is 
going to stay even when the general persistent-volumes.md has a section for 
"using persisent volumes in LAUNCH". 

I can see that we remove the example and link to persistent-volumes.md later 
though but perhaps let's add it here for now and remove it later when that's 
done?


> On Oct. 17, 2016, 11:22 a.m., Neil Conway wrote:
> > docs/shared-resources.md, line 168
> > 
> >
> > I think we should discuss what happens if you try to write to a 
> > read-only volume.

SG.


- Jiang Yan


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


On Oct. 17, 2016, 11:15 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52934/
> ---
> 
> (Updated Oct. 17, 2016, 11:15 a.m.)
> 
> 
> Review request for mesos, (Disabled_DoNotUse) Anindya Sinha and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Explain read-only mode of persistent volumes in shared-resources.md.
> 
> 
> Diffs
> -
> 
>   docs/shared-resources.md 29e43389c530f816118a32dcd7c2f1b82b0af431 
> 
> Diff: https://reviews.apache.org/r/52934/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 51785: [HBASE-16608] Merge for the segments in the compaction pipeline and simplifying the user interface for in-memory flush

[Anastasia Braginsky]

> On Oct. 15, 2016, 3:18 a.m., Michael Stack wrote:
> > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactingMemStore.java,
> >  line 398
> > 
> >
> > What is an initiateType? I like initiateAction better.

Pay attention that Action is for MemStoreCompactor, there is no action of the 
CompactingMemStore (at least now). Currently there is a type for 
CompactingMemStore saying whether it is data-compaction or index-compaction. 
From this the MemStoreCompactor's action is derived. Whould you suggest to 
address CompactingMemStore's type fpr the testing case as an 
CompactingMemStore's action and rename this method?


- Anastasia


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


On Oct. 14, 2016, 7:19 a.m., Anastasia Braginsky wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51785/
> ---
> 
> (Updated Oct. 14, 2016, 7:19 a.m.)
> 
> 
> Review request for hbase.
> 
> 
> Repository: hbase-git
> 
> 
> Description
> ---
> 
> This is a step toward final compacting memstore that allowes two modes of 
> work: index-compaction and data-compaction. 
> 
> The index-compaction means that when the new segment is pushed into the 
> pipeline, it is flattened and probably merged with old segments in the 
> pipeline. The new merge "feature" induces no data-copy-compaction and no 
> speculative SQM scan. 
> The compacting memstore of the data-compaction type means the usage of the 
> data-copy-compaction.
> 
> 
> Diffs
> -
> 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactingMemStore.java
>  177f222 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactionPipeline.java
>  6a13f43 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java
>  378601d 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ImmutableSegment.java
>  12b7916 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactor.java
>  714ffe3 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorIterator.java
>  9798ec2 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreMergerSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SegmentFactory.java
>  510ebbd 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/VersionedSegmentsList.java
>  2e8bead 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingMemStore.java
>  211a6d8 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingToCellArrayMapMemStore.java
>  f89a040 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStoreLAB.java
>  1ea5112 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestPerColumnFamilyFlush.java
>  6bfaa59 
>   
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestWalAndCompactingMemStoreFlush.java
>  74826b0 
> 
> Diff: https://reviews.apache.org/r/51785/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anastasia Braginsky
> 
>

Re: Review Request 52543: Added configure/make options to build the new CLI and run unit tests.

[Kevin Klues]

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

(Updated Oct. 17, 2016, 8:26 p.m.)


Review request for mesos and Joseph Wu.


Changes
---

Rebased and updated to call new `mesos-cli-tests` wrapper.


Bugs: MESOS-6008 and MESOS-6032
https://issues.apache.org/jira/browse/MESOS-6008
https://issues.apache.org/jira/browse/MESOS-6032


Repository: mesos


Description
---

Added configure/make options to build the new CLI and run unit tests.


Diffs (updated)
-

  configure.ac 015255ec1876a51b9eb2cf488b375af90f73c722 
  docs/getting-started.md de3fba4a821ca7fa9b9e2fd692b33b40edd35b68 
  src/Makefile.am 3bcc0f2dfc2c4f71841bd6d161f39e0e919fc0d7 

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


Testing (updated)
---

../configure --enable-new-cli
make -C src mesos
src/mesos

GTEST_FILTER="" make -j check
sudo GTEST_FILTER="" make -j check


Thanks,

Kevin Klues

Re: Review Request 51785: [HBASE-16608] Merge for the segments in the compaction pipeline and simplifying the user interface for in-memory flush

[Anastasia Braginsky]

> On Oct. 16, 2016, 5:13 p.m., Anoop Sam John wrote:
> > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java,
> >  line 179
> > 
> >
> > Able to get what u r trying to do here.  Seems very complex now.
> > So can we take a diff path?
> > We have to merge N segments into one here means we have to merge N 
> > MSLABs referred by these segments.  What we do as per this patch is make a 
> > new HeapMSLAB object and add all pooled chunk from other MSLABs int the Q 
> > in new one.  And other stuff of refer next etc.
> > 
> > Can we make a new MSLAB impl? This will act as a collection of N 
> > HeapMSLABs. Now we have to impl 4 methods
> > incScannerCount and decScannerCount based on an openScannerCount in the 
> > new class.
> > close() method like in HeapMSLAB and has to call close() on each of the 
> > N MSLABs it wrap
> > This is an ImmutableSegment so no one is going to call copyCellInto(). 
> > Make this impl in this new class throw IllegalStateException or so.  Will 
> > that look good?
> 
> Michael Stack wrote:
> No copying? That sounds good.
> 
> Anoop Sam John wrote:
> So this fix and so this comment is applicable to merge op alone..  There 
> is still an open point which Me/Ram are not agreeing around this.. That is 
> whether to do this in mem merge.  This is required to limit the #segments it 
> pipeline and to get bigger sized segment at tail of the pipeline. As we do 
> flush of the tail of the segment.  But our point is let us flush all segments 
> (I mean flush to disk)..  In case of in mem compaction (This action is by def 
> OFF and as per data usage one can enable) it is worth to flush only tail.  In 
> this normal use case let us flush all segments and so there is only flatten 
> of segments comes in and so no bug as this. There is absolutely no gain we 
> get because of delaying of flush of the other segments in the pipeline ( I 
> mean in merge kind of usages)..   So I suggest let us discuss on that before 
> going to work on this. Else it wl b waste of time.
> 
> Anastasia Braginsky wrote:
> I am referencing in this answer the Anoop's suggestion to create a new 
> MSLAB implementation that act as a collection of N old HeapMSLABs. Please pay 
> attention that old scans (opened on N old HeapMSLABs) are not aware of 
> existing of the new MSLAB. In addition the close() of the new MSLAB doesn't 
> automatically means old MSLABs need to be closed, as there might be still 
> ongoing scans. Thus I do not think this idea works (at least not as it is 
> presented now).

Anoop, I do not understand about what exactly you disagree. Are you saying that 
merging of the segments is waste of time? Do you prefer to do a merge upon the 
flush to disk? Aa we have seen on stress benchmarks this behavior delays 
flushes and blocks writes which is undesirable. Please explain yourself clearer 
or we can talk about if we have a meeting this Wednesday.


- Anastasia


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


On Oct. 14, 2016, 7:19 a.m., Anastasia Braginsky wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51785/
> ---
> 
> (Updated Oct. 14, 2016, 7:19 a.m.)
> 
> 
> Review request for hbase.
> 
> 
> Repository: hbase-git
> 
> 
> Description
> ---
> 
> This is a step toward final compacting memstore that allowes two modes of 
> work: index-compaction and data-compaction. 
> 
> The index-compaction means that when the new segment is pushed into the 
> pipeline, it is flattened and probably merged with old segments in the 
> pipeline. The new merge "feature" induces no data-copy-compaction and no 
> speculative SQM scan. 
> The compacting memstore of the data-compaction type means the usage of the 
> data-copy-compaction.
> 
> 
> Diffs
> -
> 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactingMemStore.java
>  177f222 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactionPipeline.java
>  6a13f43 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java
>  378601d 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ImmutableSegment.java
>  12b7916 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactor.java
>  714ffe3 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorIterator.java
>  9798ec2 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorSegmentsIterator.java
>  PRE-CREATION 
>  

Review Request 52952: Added a 'container' plugin to the CLI with a 'list()' command.

[Kevin Klues]

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

Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Added a 'container' plugin to the CLI with a 'list()' command.


Diffs
-

  src/cli_new/bin/config.py 274f8c63b0c642637f17aa2e3c8c4a8a5a059e37 
  src/cli_new/bin/tests.py PRE-CREATION 
  src/cli_new/lib/mesos/plugins/container/__init__.py PRE-CREATION 
  src/cli_new/lib/mesos/plugins/container/main.py PRE-CREATION 
  src/cli_new/lib/mesos/plugins/container/tests.py PRE-CREATION 

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


Testing
---

$ cd src/cli_new
$ ./bootstrap
$ source activate

(mesos-cli) $ mesos-cli-tests


Thanks,

Kevin Klues

Review Request 52950: Extended the basic CLI unit test infrastructure.

[Kevin Klues]

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

Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

This infrastrcture includes the ability to bring up a test cluster to
run the CLI against. Future unit tests will use this infrastructure to
test their commands against a running mesos cluster.


Diffs
-

  src/cli_new/bin/tests.py PRE-CREATION 
  src/cli_new/lib/mesos/http.py PRE-CREATION 
  src/cli_new/lib/mesos/tests/__init__.py PRE-CREATION 
  src/cli_new/lib/mesos/tests/base.py PRE-CREATION 
  src/cli_new/lib/mesos/tests/tests.py PRE-CREATION 

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


Testing
---

$ cd src/cli_new
$ ./bootstrap
$ source activate

(mesos-cli) $ mesos-cli-tests


Thanks,

Kevin Klues

Re: Review Request 51008: Added infrastructure for unit tests in the new python-based CLI.

[Kevin Klues]

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

(Updated Oct. 17, 2016, 8:11 p.m.)


Review request for mesos, Haris Choudhary and Joseph Wu.


Changes
---

Rebased and renamed the `mesos-tests` to `mesos-cli-tests`


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


Repository: mesos


Description
---

Added infrastructure for unit tests in the new python-based CLI.


Diffs (updated)
-

  src/cli_new/bin/mesos-cli-tests PRE-CREATION 
  src/cli_new/bin/tests.py PRE-CREATION 
  src/cli_new/pip-requirements.txt 9cc8d096ac353f4ed2d4318f2d0e439a6edf 

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


Testing (updated)
---

$ cd src/cli_new
$ ./bootstrap
$ source activate

(mesos-cli) $ mesos-cli-tests


Thanks,

Kevin Klues

Review Request 52948: Added a Table abstraction and some utility functions to the new CLI.

[Kevin Klues]

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

Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

These will be used by future plugins.


Diffs
-

  src/cli_new/lib/mesos/http.py PRE-CREATION 
  src/cli_new/lib/mesos/util.py 87d2a65e78f04209566c1434b489b941d570ee01 

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


Testing
---


Thanks,

Kevin Klues

Review Request 52953: Added container 'exec' command to the CLI.

[Kevin Klues]

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

Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Added container 'exec' command to the CLI.


Diffs
-

  src/cli_new/lib/mesos/plugins/container/main.py PRE-CREATION 
  src/cli_new/lib/mesos/plugins/container/tests.py PRE-CREATION 
  src/cli_new/lib/mesos/tests/base.py PRE-CREATION 
  src/cli_new/lib/mesos/util.py 87d2a65e78f04209566c1434b489b941d570ee01 
  src/cli_new/pip-requirements.txt 9cc8d096ac353f4ed2d4318f2d0e439a6edf 

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


Testing
---

$ cd src/cli_new
$ ./bootstrap
$ source activate

(mesos-cli) $ sudo bin/mesos-cli-tests


Thanks,

Kevin Klues

Review Request 52951: Introduced the notion of an AGENT_IP in the new CLI.

[Kevin Klues]

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

Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Introduced the notion of an AGENT_IP in the new CLI.


Diffs
-

  src/cli_new/bin/config.py 274f8c63b0c642637f17aa2e3c8c4a8a5a059e37 
  src/cli_new/lib/mesos/plugins/base.py 
61b15428b029cec9ec7c9d89ab949959c3dc5b88 

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


Testing
---


Thanks,

Kevin Klues

Review Request 52947: Updated parsing for CLI config to be more dynamic.

[Kevin Klues]

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

Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

This includes the ability to inject the values from these config
parameters into plugin help strings.


Diffs
-

  src/cli_new/bin/config.py 274f8c63b0c642637f17aa2e3c8c4a8a5a059e37 
  src/cli_new/lib/mesos/plugins/base.py 
61b15428b029cec9ec7c9d89ab949959c3dc5b88 

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


Testing
---


Thanks,

Kevin Klues

Review Request 52946: Fixed some bugs in the CLI help formatting.

[Kevin Klues]

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

Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Fixed some bugs in the CLI help formatting.


Diffs
-

  src/cli_new/lib/mesos/plugins/base.py 
61b15428b029cec9ec7c9d89ab949959c3dc5b88 
  src/cli_new/lib/mesos/util.py 87d2a65e78f04209566c1434b489b941d570ee01 

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


Testing
---


Thanks,

Kevin Klues

Review Request 52941: Updated CLI pylint configuration to allow 0 public methods.

[Kevin Klues]

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

Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Updated CLI pylint configuration to allow 0 public methods.


Diffs
-

  src/cli_new/pylint.config c398220db063d249e6c62cf7e8cd6757e7860630 

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


Testing
---


Thanks,

Kevin Klues

Review Request 52942: Updated CLI pylint configuration to ignore the 'netifaces' module.

[Kevin Klues]

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

Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Updated CLI pylint configuration to ignore the 'netifaces' module.


Diffs
-

  src/cli_new/pylint.config c398220db063d249e6c62cf7e8cd6757e7860630 

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


Testing
---


Thanks,

Kevin Klues

Review Request 52945: Added the ability for a CLI plugin command to have an alias.

[Kevin Klues]

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

Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Added the ability for a CLI plugin command to have an alias.


Diffs
-

  src/cli_new/lib/mesos/plugins/base.py 
61b15428b029cec9ec7c9d89ab949959c3dc5b88 

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


Testing
---


Thanks,

Kevin Klues

Review Request 52939: Updated CLI pylint configuration to allow 2 character variable names.

[Kevin Klues]

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

Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Updated CLI pylint configuration to allow 2 character variable names.


Diffs
-

  src/cli_new/pylint.config c398220db063d249e6c62cf7e8cd6757e7860630 

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


Testing
---


Thanks,

Kevin Klues

Review Request 52940: Updated CLI pylint configuration to allow up to 30 local variables.

[Kevin Klues]

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

Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Updated CLI pylint configuration to allow up to 30 local variables.


Diffs
-

  src/cli_new/pylint.config c398220db063d249e6c62cf7e8cd6757e7860630 

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


Testing
---


Thanks,

Kevin Klues

Review Request 52944: Updated CLI pylint configuration to disable 'fixme' warnings.

[Kevin Klues]

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

Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Updated CLI pylint configuration to disable 'fixme' warnings.


Diffs
-

  src/cli_new/pylint.config c398220db063d249e6c62cf7e8cd6757e7860630 

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


Testing
---


Thanks,

Kevin Klues

Review Request 52943: Updated CLI pylint configuration to disable no-self-use warnings.

[Kevin Klues]

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

Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Updated CLI pylint configuration to disable no-self-use warnings.


Diffs
-

  src/cli_new/pylint.config c398220db063d249e6c62cf7e8cd6757e7860630 

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


Testing
---


Thanks,

Kevin Klues

Re: Review Request 52879: Updated the way in which the default and command executors load options.

[Jie Yu]

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




src/launcher/default_executor.cpp (lines 1015 - 1042)


Thanks for doing that! This definitely simplies the parsing of those env 
variables.

I think there are two types of flags (or ENV) that'll be passed to default 
executor (and this also applies to the old command executor):

1) executor environment variables that are *COMMON* to all executors, 
including custom executors. For instance, MESOS_EXECUTOR_ID, 
MESOS_FRAMEWORK_ID, MESOS_SANDBOX.

2) flags or env variables that only apply to default executor (or old 
command executor). For instance, `launcher_dir`, `MESOS_HTTP_COMMAND_EXECUTOR`. 
In retrospect, we probably should do the following for command executor:
```
COMMAND_EXECUTOR_HTTP_API
COMMAND_EXECUTOR_LAUNCHER_DIR
```

and in command executor, we probably need two sets of flags:
```
MesosFlags mesosFlags; // MESOS_*
CommandExecutorFlags commandExecutorFlags; // COMMAND_EXECUTOR_*
```

cc @vinodkone @anand



src/launcher/default_executor.cpp (line 1028)


What's this? Is this MESOS_SANDBOX?



src/launcher/default_executor.cpp (line 1038)


s/string/ExecutorID/

you may need to add a parse function for ExecutorID



src/launcher/default_executor.cpp (line 1039)


Ditto.



src/launcher/default_executor.cpp (lines 1056 - 1059)


I thought we should start to use `LIBPROCESS_SSL_xxx`



src/launcher/executor.cpp (lines 883 - 885)


Ditto. Let's separate the two sets of flags.


- Jie Yu


On Oct. 14, 2016, 5:17 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52879/
> ---
> 
> (Updated Oct. 14, 2016, 5:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, Jie Yu, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the command and the default executor so that they use only
> `stout::flags` to load config options.
> 
> They used to use a mix of `stout::flags` and `os::getenv`.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp af4a97f7de5f2157aa65fdab742455b0683c40a4 
>   src/launcher/executor.cpp 3e95d6029bea0ce6e0dfb39c24b795fe98d90d13 
> 
> Diff: https://reviews.apache.org/r/52879/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 51785: [HBASE-16608] Merge for the segments in the compaction pipeline and simplifying the user interface for in-memory flush

[Anastasia Braginsky]

> On Oct. 16, 2016, 5:13 p.m., Anoop Sam John wrote:
> > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java,
> >  line 179
> > 
> >
> > Able to get what u r trying to do here.  Seems very complex now.
> > So can we take a diff path?
> > We have to merge N segments into one here means we have to merge N 
> > MSLABs referred by these segments.  What we do as per this patch is make a 
> > new HeapMSLAB object and add all pooled chunk from other MSLABs int the Q 
> > in new one.  And other stuff of refer next etc.
> > 
> > Can we make a new MSLAB impl? This will act as a collection of N 
> > HeapMSLABs. Now we have to impl 4 methods
> > incScannerCount and decScannerCount based on an openScannerCount in the 
> > new class.
> > close() method like in HeapMSLAB and has to call close() on each of the 
> > N MSLABs it wrap
> > This is an ImmutableSegment so no one is going to call copyCellInto(). 
> > Make this impl in this new class throw IllegalStateException or so.  Will 
> > that look good?
> 
> Michael Stack wrote:
> No copying? That sounds good.
> 
> Anoop Sam John wrote:
> So this fix and so this comment is applicable to merge op alone..  There 
> is still an open point which Me/Ram are not agreeing around this.. That is 
> whether to do this in mem merge.  This is required to limit the #segments it 
> pipeline and to get bigger sized segment at tail of the pipeline. As we do 
> flush of the tail of the segment.  But our point is let us flush all segments 
> (I mean flush to disk)..  In case of in mem compaction (This action is by def 
> OFF and as per data usage one can enable) it is worth to flush only tail.  In 
> this normal use case let us flush all segments and so there is only flatten 
> of segments comes in and so no bug as this. There is absolutely no gain we 
> get because of delaying of flush of the other segments in the pipeline ( I 
> mean in merge kind of usages)..   So I suggest let us discuss on that before 
> going to work on this. Else it wl b waste of time.

I am referencing in this answer the Anoop's suggestion to create a new MSLAB 
implementation that act as a collection of N old HeapMSLABs. Please pay 
attention that old scans (opened on N old HeapMSLABs) are not aware of existing 
of the new MSLAB. In addition the close() of the new MSLAB doesn't 
automatically means old MSLABs need to be closed, as there might be still 
ongoing scans. Thus I do not think this idea works (at least not as it is 
presented now).


- Anastasia


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


On Oct. 14, 2016, 7:19 a.m., Anastasia Braginsky wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51785/
> ---
> 
> (Updated Oct. 14, 2016, 7:19 a.m.)
> 
> 
> Review request for hbase.
> 
> 
> Repository: hbase-git
> 
> 
> Description
> ---
> 
> This is a step toward final compacting memstore that allowes two modes of 
> work: index-compaction and data-compaction. 
> 
> The index-compaction means that when the new segment is pushed into the 
> pipeline, it is flattened and probably merged with old segments in the 
> pipeline. The new merge "feature" induces no data-copy-compaction and no 
> speculative SQM scan. 
> The compacting memstore of the data-compaction type means the usage of the 
> data-copy-compaction.
> 
> 
> Diffs
> -
> 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactingMemStore.java
>  177f222 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactionPipeline.java
>  6a13f43 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java
>  378601d 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ImmutableSegment.java
>  12b7916 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactor.java
>  714ffe3 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorIterator.java
>  9798ec2 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreMergerSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreSegmentsIterator.java
>  PRE-CREATION 
>   
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SegmentFactory.java
>  510ebbd 
>   
> 

Re: Review Request 52934: Explain read-only mode of persistent volumes in shared-resources.md.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52934]

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 Oct. 17, 2016, 6:15 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52934/
> ---
> 
> (Updated Oct. 17, 2016, 6:15 p.m.)
> 
> 
> Review request for mesos, (Disabled_DoNotUse) Anindya Sinha and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Explain read-only mode of persistent volumes in shared-resources.md.
> 
> 
> Diffs
> -
> 
>   docs/shared-resources.md 29e43389c530f816118a32dcd7c2f1b82b0af431 
> 
> Diff: https://reviews.apache.org/r/52934/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 52877: Fixed wrong float serialization in JSON in locales different from C.

[Joris Van Remoortere]

> On Oct. 14, 2016, 11:23 p.m., Joris Van Remoortere wrote:
> > I have a feeling that if you kept around a stringstream with the local set 
> > your benchmarks would look rather different.
> > I also suggest using callgrind to get the instruction count / # of library 
> > calls made.
> 
> Alexander Rojas wrote:
> That occurred to me, but then some issues appeared with the idea. The 
> version in `json.hpp` uses a free function, the way to keep the 
> `stringstream` is by making it global (either having a global variable or 
> marking it `static`), however that makes the function non thread safe and a 
> mutex will be needed. It probably will still perform better in a single 
> thread situation though.
> 
> The `jsonify.hpp` version has more or less the same concerns. I though 
> moving the `stringstream` to the constructor of the `Number` object, which 
> would fix the thread safety issue, but at the end one is just moving the 
> object construction penalty from serialization to construction time. But 
> given the usage of `jsonify`, it wouldn't have any performance issue.
> 
> Do you have any other idea on how to keep the `stringstream` alive?

Thread Local?


- Joris


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


On Oct. 14, 2016, 12:50 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52877/
> ---
> 
> (Updated Oct. 14, 2016, 12:50 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, and Michael Park.
> 
> 
> Bugs: MESOS-6349
> https://issues.apache.org/jira/browse/MESOS-6349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Resolves an isse with the JSON serialization functions where floats
> would use the process active locale as a decimal separator instead
> of the JSON conformant period (`.`).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/json.hpp 
> 9734561549eea867d57f14a0ab71febeb9dddc06 
>   3rdparty/stout/include/stout/jsonify.hpp 
> 8220d001001e8b9f247c05be58534f1174611aad 
> 
> Diff: https://reviews.apache.org/r/52877/diff/
> 
> 
> Testing
> ---
> 
> Three types of tests were used for this issue. The first just run our current
> test suite (`make check`). The second verifies that the proposed solutions
> are able to generate a locale independent output and the third consist of a
> benchmark of all the solutions.
> 
> The verification that the proposed solutions produce locale independent JSON,
> the following program was used (with manual verification of the generated
> output):
> 
> ```c++
> #include 
> 
> #include 
> #include 
> #include 
> 
> int main(int argc, char **argv) {
>   // Set locale to German.
>   std::setlocale(LC_ALL, "de_DE.UTF-8");
>   std::cout.imbue(std::locale("de_DE.UTF-8"));
> 
>   JSON::Object object;
>   object.values["float"] = 1234567890.12345;
> 
>   std::cout << stringify(object) << '\n';
>   return 0;
> }
> 
> ```
> 
> In order to test the different options the following benchmarking program was
> written using [Google benchmark](https://github.com/google/benchmark):
> 
> ```c++
> #include 
> #include 
> 
> #include 
> #include 
> #include 
> #include 
> 
> static void BM_Jsonify(benchmark::State& state) {
>   while (state.KeepRunning()) {
> state.PauseTiming();
> JSON::Object object;
> object.values["float00"] = 1234567890.12345;
> object.values["float01"] = 123456789.012345;
> object.values["float02"] = 12345678.9012345;
> object.values["float03"] = 1234567.89012345;
> object.values["float04"] = 123456.789012345;
> object.values["float05"] = 12345.6789012345;
> object.values["float06"] = 1234.56789012345;
> object.values["float07"] = 123.456789012345;
> object.values["float08"] = 12.3456789012345;
> object.values["float09"] = 1.23456789012345;
> object.values["float10"] = 1234567890.12345;
> object.values["float11"] = 123456789.012345;
> object.values["float12"] = 12345678.9012345;
> object.values["float13"] = 1234567.89012345;
> object.values["float14"] = 123456.789012345;
> object.values["float15"] = 12345.6789012345;
> object.values["float16"] = 1234.56789012345;
> object.values["float17"] = 123.456789012345;
> object.values["float18"] = 12.3456789012345;
> object.values["float19"] = 1.23456789012345;
> object.values["float20"] = 1234567890.12345;
> object.values["float21"] = 123456789.012345;
> object.values["float22"] = 12345678.9012345;
> object.values["float23"] = 1234567.89012345;
> object.values["float24"] = 123456.789012345;
> object.values["float25"] = 12345.6789012345;
> object.values["float26"] = 

Re: Review Request 52880: Added "launcher_dir" to the default executor flags.

[Jie Yu]

> On Oct. 14, 2016, 8:25 p.m., Jiang Yan Xu wrote:
> > This is conversation carried over from /r/52556/
> > 
> > Now that we're in the business of (re)naming things for consistency, 
> > understandability and unambiguity, how about we just follow some widely 
> > used conventions:
> > https://www.gnu.org/prep/standards/html_node/Directory-Variables.html
> > 
> > The motivation in the renaming in /r/52556/ seems to be because the number 
> > of binaries in the directory has grown and not all related to the launcher 
> > anymore. `--libexec_dir` or `MESOS_LIBEXEC_DIR` feels pretty clear for what 
> > it is.
> > 
> > BTW, when I first see the flag `--runtime_dir` on the agent, I thought it 
> > was for the "runtime binaries" as well. Would `--runstate_dir` or 
> > `MESOS_RUNSTATE_DIR` (from the linked doc) be better names? 
> > 
> > ^^ @Jie
> 
> Gastón Kleiman wrote:
> The goal of this patch is to resolve MESOS-6288 (The default executor 
> should maintain launcher_dir) and therefore to unblock MESOS-6119 (TCP health 
> checks are not portable).
> 
> I agree that `launcher_dir` is not a good name for the flag and volunteer 
> for doing patch with a sweeping rename ss part of MESOS-6341 (Improve 
> environment variable setting for executors, tasks and nested containers).
> 
> haosdent huang wrote:
> +1 for `MESOS_LIBEXEC_DIR` and agree with we should rename it in separate 
> patches. Because we use `launcher_dir` in different files, should not mess up 
> the rename part with this.
> 
> ```
> $ grep -r 'launcher_dir' src |wc -l
> 61
> ```

Yeah, launcher_dir is unfortunate. Given that agent already has this flag, it's 
not easy to retire. We can probably introduce flag alias. Let's create a ticket 
and track this deprecation.


- Jie


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


On Oct. 14, 2016, 5:18 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52880/
> ---
> 
> (Updated Oct. 14, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6288
> https://issues.apache.org/jira/browse/MESOS-6288
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added "launcher_dir" to the default executor flags.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp af4a97f7de5f2157aa65fdab742455b0683c40a4 
>   src/slave/slave.cpp 6bd9b49c3bbdb973a0d03552ae8fe55b33371083 
> 
> Diff: https://reviews.apache.org/r/52880/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 51617: Added the logic for installing and removing DNAT rules.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
 (line 125)


Can this be `const net::IP& ip`?



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
 (line 144)


Can you make this non optional?



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


This sounds not necessary if we pass in cniContainerID as a required field.



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (line 309)


`s/_command/script/`



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (line 314)


you probably want to print the command executed. Probably use `set -x` 
here? Can we dump all the output of this script to stderr (redirect stdout to 
stderr).



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


Avoid DNAT 127.0.0.0/8 traffic here?



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (line 366)


`s/_command/script/`



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (line 367)


Let's make this a script as well and add `set -x`



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (line 398)


`s/_networkInfo/delegateResult/`



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (line 402)


Can you also print ADD in error message



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (line 406)


quota before and after 'delegatePlugin'



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (line 408)


stringify is not needed.

Remove space before `:`



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


Delegate CNI plugin 'xxx' did not return an IPv4 address: ...



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (line 417)


This should be delegate failure?



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (line 429)


This should be delegate failure?



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (line 437)


kill this line.



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (line 466)


`s/_networkInfo/delegateResult/`



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (line 469)


Can you also print "DEL" in the error message?



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


I'd prefer the following flow:
```
if (cniCommand == spec::CNI_CMD_ADD) {
  ...
} else if (cniCommand == spec::CNI_CMD_DEL) {
  ...
} else {
  return Error("");
}
```


- Jie Yu


On Oct. 17, 2016, 7:12 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51617/
> ---
> 
> (Updated Oct. 17, 2016, 7:12 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6023
> https://issues.apache.org/jira/browse/MESOS-6023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the logic for installing and removing DNAT rules.
> 
> 
> Diffs
> -
> 
>   
> 

Re: Review Request 51617: Added the logic for installing and removing DNAT rules.

[Avinash sridharan]

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

(Updated Oct. 17, 2016, 7:12 p.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Added an exception to iptables jump rule in OUTPUT chain. Made 
`CNI_CONTAINERID` a required property of the port-mapper plugin.


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


Repository: mesos


Description
---

Added the logic for installing and removing DNAT rules.


Diffs (updated)
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
 7fad707a240234e35828917aea1bc79f42fe130e 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 2ff8b0e76a11b6f6c98b839d3ac91a81e41285f5 

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


Testing
---

Ran the CNI plugin against a network namespace with the following JSON input:
```
{
"name": "mynet",
"type": "port-mapper",
"chain": "MESOS-TEST",
"excludeDevices": ["mesos-cni0"],
"delegate": {
  "type" : "bridge",
  "bridge": "cni0",
  "isGateway": true,
  "ipMasq": true,
  "ipam": {
  "type": "host-local",
  "subnet": "192.168.37.0/24",
  "routes": [
{ "dst": "0.0.0.0/0" }
  ]
  }
},
"args" : {
  "org.apache.mesos" : {
"network_info" : {
  "port_mappings": {
"host_port" : 8080,
"container_port" : 9000
  }
}
  }
}
}
```

Used the ADD command to test that the CNI plugin correctly invokes the delegate 
plugin (a CNI bridge plugin in this case) and also inserts the correct iptable 
entries for the given port mapping. After running this plugin, this was the 
output of the `iptables -t nat -S MESOS-TEST` command:
```
sudo iptables -t nat -S MESOS-TEST
-N MESOS-TEST
-A MESOS-TEST ! -i mesos-cni0 -p tcp -m tcp --dport 8080 -j DNAT 
--to-destination 192.168.37.21:9000
```

Ran a python HTTP server in this network namespace and verified that DNAT works 
from outside the box. Was able to connect to port 9000 of this server, by 
connecting to port 8080 on the host.

Used the DEL command to test the CNI plugin correctly deletes the DNAT rule and 
chain, if there are no DNAT rules exist in the chain. After running the DEL 
command (by injecting `NetworkInfo` into the above JSON schema) verified the 
chain and the DNAT rule is deleted from iptables.


Apart from these tests ran a single node cluster and did an end-to-end test 
with a modified `mesos-execute` binary that can setup port-mapping.


Thanks,

Avinash sridharan

Re: Review Request 50857: Modified a scheduler test to run with SSL enabled.

[Greg Mann]

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

(Updated Oct. 17, 2016, 6:58 p.m.)


Review request for mesos, Joseph Wu and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

This patch modifies the test `SchedulerTest.Teardown` to
be parametrized by both `ContentType` and SSL configuration,
and renames it to `SchedulerSSLTest.RunTaskAndTeardown`.
This allows the test to verify the scheduler's behavior with
SSL both enabled and disabled.


Diffs (updated)
-

  src/tests/scheduler_tests.cpp b0ea0bbcce9d847285fda40f778caaf721804457 

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


Testing
---

The test was run in repetition as follows:

`GTEST_REPEAT=-1 GTEST_BREAK_ON_FAILURE=1 
GTEST_FILTER="ContentTypeAndSSLConfig/SchedulerSSLTest.RunTaskAndTeardown*" 
bin/mesos-tests.sh`


Thanks,

Greg Mann

Re: Review Request 50737: Parametrized libprocess HTTPTests by SSL configuration.

[Greg Mann]

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

(Updated Oct. 17, 2016, 6:57 p.m.)


Review request for mesos, Anand Mazumdar, Joseph Wu, and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

This patch parametrizes the HTTP tests in libprocess so
that they run with SSL both enabled and disabled when
the library was compiled with SSL support.


Diffs (updated)
-

  3rdparty/libprocess/src/tests/http_tests.cpp 
533104c93dd1eaf67bf3752163d2e0cad090078d 

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


Testing
---

Tested on OSX with `GTEST_REPEAT=100 GTEST_BREAK_ON_FAILURE=1 
GTEST_FILTER="*HTTPTest*" 3rdparty/libprocess/libprocess-tests`


Thanks,

Greg Mann

Re: Review Request 51065: Changed hostname used for SSL cert creation in tests.

[Greg Mann]

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

(Updated Oct. 17, 2016, 6:54 p.m.)


Review request for mesos, Joseph Wu and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

This changes the SSL helpers in libprocess to generate
certs using a hostname determined using the same address
that is advertised by libprocess. This assures that
validation of the certificates will succeed. The test
`SSLTest.BasicSameProcess` is also updated to accommodate
this change.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/gtest.hpp 
21a0fc45b55a368a21b3e616c751ab43eebd4902 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
55c8c309571b1892b0acc4d766eda9bb98085a6f 

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


Testing
---

`make check` on Ubuntu 14.04


Thanks,

Greg Mann

Re: Review Request 52934: Explain read-only mode of persistent volumes in shared-resources.md.

[Neil Conway]

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



The mode of a volume doesn't necessarily have anything to do with whether the 
volume is shared -- perhaps it would be better to document this in the 
persistent-volume doc page, and link to it from here?


docs/shared-resources.md (line 168)


I think we should discuss what happens if you try to write to a read-only 
volume.


- Neil Conway


On Oct. 17, 2016, 6:15 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52934/
> ---
> 
> (Updated Oct. 17, 2016, 6:15 p.m.)
> 
> 
> Review request for mesos, (Disabled_DoNotUse) Anindya Sinha and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Explain read-only mode of persistent volumes in shared-resources.md.
> 
> 
> Diffs
> -
> 
>   docs/shared-resources.md 29e43389c530f816118a32dcd7c2f1b82b0af431 
> 
> Diff: https://reviews.apache.org/r/52934/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Review Request 52934: Explain read-only mode of persistent volumes in shared-resources.md.

[Jiang Yan Xu]

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

Review request for mesos, (Disabled_DoNotUse) Anindya Sinha and Neil Conway.


Repository: mesos


Description
---

Explain read-only mode of persistent volumes in shared-resources.md.


Diffs
-

  docs/shared-resources.md 29e43389c530f816118a32dcd7c2f1b82b0af431 

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


Testing
---

N/A


Thanks,

Jiang Yan Xu

Re: Review Request 52879: Updated the way in which the default and command executors load options.

[haosdent huang]

> On Oct. 17, 2016, 5:52 p.m., haosdent huang wrote:
> > src/launcher/default_executor.cpp, line 1034
> > 
> >
> > Let's use `agent_pid` here? Since we rename slave to agent.
> 
> haosdent huang wrote:
> hmm, I saw we still use `slave_pid` in the default-executor. Would be 
> better to keep consistency with there. Let me drop this.

Just realize we use `MESOS_SLAVE_PID` for quite a long time before staring 
rename `slave` to `agent`, sorry for create incorrect issue before.


- haosdent


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


On Oct. 14, 2016, 5:17 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52879/
> ---
> 
> (Updated Oct. 14, 2016, 5:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, Jie Yu, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the command and the default executor so that they use only
> `stout::flags` to load config options.
> 
> They used to use a mix of `stout::flags` and `os::getenv`.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp af4a97f7de5f2157aa65fdab742455b0683c40a4 
>   src/launcher/executor.cpp 3e95d6029bea0ce6e0dfb39c24b795fe98d90d13 
> 
> Diff: https://reviews.apache.org/r/52879/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52879: Updated the way in which the default and command executors load options.

[haosdent huang]

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



Do you forget to update the `MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD` in 
`docker-executor`?

- haosdent huang


On Oct. 14, 2016, 5:17 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52879/
> ---
> 
> (Updated Oct. 14, 2016, 5:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, Jie Yu, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the command and the default executor so that they use only
> `stout::flags` to load config options.
> 
> They used to use a mix of `stout::flags` and `os::getenv`.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp af4a97f7de5f2157aa65fdab742455b0683c40a4 
>   src/launcher/executor.cpp 3e95d6029bea0ce6e0dfb39c24b795fe98d90d13 
> 
> Diff: https://reviews.apache.org/r/52879/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52878: Removed outdated TODO in stout::flags.

[haosdent huang]

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


Ship it!




Ship It!

- haosdent huang


On Oct. 14, 2016, 1:35 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52878/
> ---
> 
> (Updated Oct. 14, 2016, 1:35 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Jie Yu, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed outdated TODO in stout::flags.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/flags.hpp 
> 21257b7ee6ed7a9a48a0ca1ad4edd8890ca2fa8f 
> 
> Diff: https://reviews.apache.org/r/52878/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>