Re: Review Request 55065: Added support for s390x architecture

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [55065]

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 March 30, 2017, 11:29 a.m., Ayanampudi Varsha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55065/
> ---
> 
> (Updated March 30, 2017, 11:29 a.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Vinod Kone.
> 
> 
> Bugs: MESOS-6742
> https://issues.apache.org/jira/browse/MESOS-6742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for s390x architecture
> 
> 
> Diffs
> -
> 
>   src/linux/ldcache.cpp 6260496672cd0226a24ea1b34003a48af9678a63 
> 
> 
> Diff: https://reviews.apache.org/r/55065/diff/2/
> 
> 
> Testing
> ---
> 
> make check 
> make check GTEST_FILTER="LdcacheTest.Parse"
> 
> 
> Thanks,
> 
> Ayanampudi Varsha
> 
>

Re: Review Request 55065: Added support for s390x architecture

[Ayanampudi Varsha]

> On March 22, 2017, 2:45 p.m., Till Toenshoff wrote:
> > src/linux/ldcache.cpp
> > Line 120 (original), 120 (patched)
> > 
> >
> > Can you elaborate why this change is safe for all systems?
> 
> Ayanampudi Varsha wrote:
> We have tried this on Big endian system where 16 bit flag was causing 
> issue.
> Test case LdcacheTest.Parse failed at condition ASSERT_GT(cache->size(), 
> 1u); as size was returned as 0. This happens because the 16 bits after the 
> field "flags" is unused. 
> On x86_64, I have verified that this "cache->size()" is same with 16 bit 
> and 32 bit flag.
> Observed that the other fields in structure are all 32bit/64bit.
> An alternative would be to make changes only specific to s390x by putting 
> a condition. Please suggest
> 
> Till Toenshoff wrote:
> You may actually have found a bug in our ldcache parsing. After looking 
> into the glibc sources, I think this field should have been 32bit also for 
> x86_64 instead of 16bit - I suspect missing struct-packing for this bug to 
> not have surfaced earlier. For confirming this without mixing it with your 
> platform specific docker-repository change, I would like you to split this 
> patch into two.
> 
> So please add another patch that only fixes `flags` attribute size into 
> 32bit. Please make sure you add the following reviewers: tillt, klueska.
> 
> Remove that 32bit change from this patch and update it according to my 
> and Kevin's review comments.
> 
> Thanks a bunch for your work - this is highly appreciated.
> 
> Till Toenshoff wrote:
> Ow, for further reference of this `flags` change, here are the internal 
> glibc structures: 
> https://github.com/lattera/glibc/blob/master/sysdeps/generic/dl-cache.h#L80
> 
> Ayanampudi Varsha wrote:
> As I am planning to drop the docker_registry customization change, can I 
> resubmit removing docker_registry change in same patch and continue in the 
> same review board ticket. please suggest
> 
> Till Toenshoff wrote:
> Certainly Ayanampudi. Please do as you suggested...

I have updated the ticket to have only flags 32 bit change.


- Ayanampudi


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


On March 30, 2017, 11:29 a.m., Ayanampudi Varsha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55065/
> ---
> 
> (Updated March 30, 2017, 11:29 a.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Vinod Kone.
> 
> 
> Bugs: MESOS-6742
> https://issues.apache.org/jira/browse/MESOS-6742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for s390x architecture
> 
> 
> Diffs
> -
> 
>   src/linux/ldcache.cpp 6260496672cd0226a24ea1b34003a48af9678a63 
> 
> 
> Diff: https://reviews.apache.org/r/55065/diff/2/
> 
> 
> Testing
> ---
> 
> make check 
> make check GTEST_FILTER="LdcacheTest.Parse"
> 
> 
> Thanks,
> 
> Ayanampudi Varsha
> 
>

Re: Review Request 55065: Added support for s390x architecture

[Ayanampudi Varsha]

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

(Updated March 30, 2017, 11:29 a.m.)


Review request for mesos, Till Toenshoff and Vinod Kone.


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


Repository: mesos


Description (updated)
---

Added support for s390x architecture


Diffs (updated)
-

  src/linux/ldcache.cpp 6260496672cd0226a24ea1b34003a48af9678a63 


Diff: https://reviews.apache.org/r/55065/diff/2/

Changes: https://reviews.apache.org/r/55065/diff/1-2/


Testing
---

make check 
make check GTEST_FILTER="LdcacheTest.Parse"


Thanks,

Ayanampudi Varsha

Re: Review Request 55065: Added support for s390x architecture

[Till Toenshoff]

> On March 22, 2017, 2:45 p.m., Till Toenshoff wrote:
> > src/linux/ldcache.cpp
> > Line 120 (original), 120 (patched)
> > 
> >
> > Can you elaborate why this change is safe for all systems?
> 
> Ayanampudi Varsha wrote:
> We have tried this on Big endian system where 16 bit flag was causing 
> issue.
> Test case LdcacheTest.Parse failed at condition ASSERT_GT(cache->size(), 
> 1u); as size was returned as 0. This happens because the 16 bits after the 
> field "flags" is unused. 
> On x86_64, I have verified that this "cache->size()" is same with 16 bit 
> and 32 bit flag.
> Observed that the other fields in structure are all 32bit/64bit.
> An alternative would be to make changes only specific to s390x by putting 
> a condition. Please suggest
> 
> Till Toenshoff wrote:
> You may actually have found a bug in our ldcache parsing. After looking 
> into the glibc sources, I think this field should have been 32bit also for 
> x86_64 instead of 16bit - I suspect missing struct-packing for this bug to 
> not have surfaced earlier. For confirming this without mixing it with your 
> platform specific docker-repository change, I would like you to split this 
> patch into two.
> 
> So please add another patch that only fixes `flags` attribute size into 
> 32bit. Please make sure you add the following reviewers: tillt, klueska.
> 
> Remove that 32bit change from this patch and update it according to my 
> and Kevin's review comments.
> 
> Thanks a bunch for your work - this is highly appreciated.
> 
> Till Toenshoff wrote:
> Ow, for further reference of this `flags` change, here are the internal 
> glibc structures: 
> https://github.com/lattera/glibc/blob/master/sysdeps/generic/dl-cache.h#L80
> 
> Ayanampudi Varsha wrote:
> As I am planning to drop the docker_registry customization change, can I 
> resubmit removing docker_registry change in same patch and continue in the 
> same review board ticket. please suggest

Certainly Ayanampudi. Please do as you suggested...


- Till


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


On March 27, 2017, 9:25 a.m., Ayanampudi Varsha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55065/
> ---
> 
> (Updated March 27, 2017, 9:25 a.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Vinod Kone.
> 
> 
> Bugs: MESOS-6742
> https://issues.apache.org/jira/browse/MESOS-6742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for s390x architecture
> There are 2 issues addressed in this patch:
> 1. LdcacheTest.Parse test case fails on s390x machines.
> 2. From the value of flag docker_registry in slave/flags.cpp, amd64 images 
> get downloaded due to which test cases fail on s390x with "Exec format Error"
> 
> 
> Diffs
> -
> 
>   src/linux/ldcache.cpp 6260496672cd0226a24ea1b34003a48af9678a63 
>   src/slave/flags.cpp 1eccea920338032173be4df6c374ec50dbd2eaf3 
> 
> 
> Diff: https://reviews.apache.org/r/55065/diff/1/
> 
> 
> Testing
> ---
> 
> make check 
> make check GTEST_FILTER="LdcacheTest.Parse"
> 
> 
> Thanks,
> 
> Ayanampudi Varsha
> 
>

Re: Review Request 55065: Added support for s390x architecture

[Ayanampudi Varsha]

> On March 27, 2017, 7:01 p.m., Kevin Klues wrote:
> > src/slave/flags.cpp
> > Lines 152-158 (original), 152-169 (patched)
> > 
> >
> > We typically don't create duplicate entries like this when simply 
> > customizing the default value. Instead we use a lambda with the #ifdef's 
> > inside of it. For example,
> > 
> > https://github.com/apache/mesos/blob/master/src/slave/flags.cpp#L212
> 
> Till Toenshoff wrote:
> I am not sure we really need to come up with a lambda for such static 
> default - see 
> https://github.com/apache/mesos/blob/42f43ceb3b4b375357281030fb3816e548dac763/src/slave/flags.cpp#L670
> Let's not make this more complicated than needed.
> 
> Kevin Klues wrote:
> Sure that makes sense. No need for a lambda in this case, but the #ifdef 
> should only be around the new default value parameter, not a duplicate of the 
> entire flags field.

On a second thought, as user can anyways configure to local path if required, 
as mentioned in flag description.
I would like to drop these changes.


- Ayanampudi


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


On March 27, 2017, 9:25 a.m., Ayanampudi Varsha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55065/
> ---
> 
> (Updated March 27, 2017, 9:25 a.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Vinod Kone.
> 
> 
> Bugs: MESOS-6742
> https://issues.apache.org/jira/browse/MESOS-6742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for s390x architecture
> There are 2 issues addressed in this patch:
> 1. LdcacheTest.Parse test case fails on s390x machines.
> 2. From the value of flag docker_registry in slave/flags.cpp, amd64 images 
> get downloaded due to which test cases fail on s390x with "Exec format Error"
> 
> 
> Diffs
> -
> 
>   src/linux/ldcache.cpp 6260496672cd0226a24ea1b34003a48af9678a63 
>   src/slave/flags.cpp 1eccea920338032173be4df6c374ec50dbd2eaf3 
> 
> 
> Diff: https://reviews.apache.org/r/55065/diff/1/
> 
> 
> Testing
> ---
> 
> make check 
> make check GTEST_FILTER="LdcacheTest.Parse"
> 
> 
> Thanks,
> 
> Ayanampudi Varsha
> 
>

Re: Review Request 55065: Added support for s390x architecture

[Ayanampudi Varsha]

> On March 22, 2017, 2:45 p.m., Till Toenshoff wrote:
> > src/linux/ldcache.cpp
> > Line 120 (original), 120 (patched)
> > 
> >
> > Can you elaborate why this change is safe for all systems?
> 
> Ayanampudi Varsha wrote:
> We have tried this on Big endian system where 16 bit flag was causing 
> issue.
> Test case LdcacheTest.Parse failed at condition ASSERT_GT(cache->size(), 
> 1u); as size was returned as 0. This happens because the 16 bits after the 
> field "flags" is unused. 
> On x86_64, I have verified that this "cache->size()" is same with 16 bit 
> and 32 bit flag.
> Observed that the other fields in structure are all 32bit/64bit.
> An alternative would be to make changes only specific to s390x by putting 
> a condition. Please suggest
> 
> Till Toenshoff wrote:
> You may actually have found a bug in our ldcache parsing. After looking 
> into the glibc sources, I think this field should have been 32bit also for 
> x86_64 instead of 16bit - I suspect missing struct-packing for this bug to 
> not have surfaced earlier. For confirming this without mixing it with your 
> platform specific docker-repository change, I would like you to split this 
> patch into two.
> 
> So please add another patch that only fixes `flags` attribute size into 
> 32bit. Please make sure you add the following reviewers: tillt, klueska.
> 
> Remove that 32bit change from this patch and update it according to my 
> and Kevin's review comments.
> 
> Thanks a bunch for your work - this is highly appreciated.
> 
> Till Toenshoff wrote:
> Ow, for further reference of this `flags` change, here are the internal 
> glibc structures: 
> https://github.com/lattera/glibc/blob/master/sysdeps/generic/dl-cache.h#L80

As I am planning to drop the docker_registry customization change, can I 
resubmit removing docker_registry change in same patch and continue in the same 
review board ticket. please suggest


- Ayanampudi


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


On March 27, 2017, 9:25 a.m., Ayanampudi Varsha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55065/
> ---
> 
> (Updated March 27, 2017, 9:25 a.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Vinod Kone.
> 
> 
> Bugs: MESOS-6742
> https://issues.apache.org/jira/browse/MESOS-6742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for s390x architecture
> There are 2 issues addressed in this patch:
> 1. LdcacheTest.Parse test case fails on s390x machines.
> 2. From the value of flag docker_registry in slave/flags.cpp, amd64 images 
> get downloaded due to which test cases fail on s390x with "Exec format Error"
> 
> 
> Diffs
> -
> 
>   src/linux/ldcache.cpp 6260496672cd0226a24ea1b34003a48af9678a63 
>   src/slave/flags.cpp 1eccea920338032173be4df6c374ec50dbd2eaf3 
> 
> 
> Diff: https://reviews.apache.org/r/55065/diff/1/
> 
> 
> Testing
> ---
> 
> make check 
> make check GTEST_FILTER="LdcacheTest.Parse"
> 
> 
> Thanks,
> 
> Ayanampudi Varsha
> 
>

Re: Review Request 55065: Added support for s390x architecture

[Kevin Klues]

> On March 27, 2017, 7:01 p.m., Kevin Klues wrote:
> > src/slave/flags.cpp
> > Lines 152-158 (original), 152-169 (patched)
> > 
> >
> > We typically don't create duplicate entries like this when simply 
> > customizing the default value. Instead we use a lambda with the #ifdef's 
> > inside of it. For example,
> > 
> > https://github.com/apache/mesos/blob/master/src/slave/flags.cpp#L212
> 
> Till Toenshoff wrote:
> I am not sure we really need to come up with a lambda for such static 
> default - see 
> https://github.com/apache/mesos/blob/42f43ceb3b4b375357281030fb3816e548dac763/src/slave/flags.cpp#L670
> Let's not make this more complicated than needed.

Sure that makes sense. No need for a lambda in this case, but the #ifdef should 
only be around the new default value parameter, not a duplicate of the entire 
flags field.


- Kevin


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


On March 27, 2017, 9:25 a.m., Ayanampudi Varsha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55065/
> ---
> 
> (Updated March 27, 2017, 9:25 a.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Vinod Kone.
> 
> 
> Bugs: MESOS-6742
> https://issues.apache.org/jira/browse/MESOS-6742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for s390x architecture
> There are 2 issues addressed in this patch:
> 1. LdcacheTest.Parse test case fails on s390x machines.
> 2. From the value of flag docker_registry in slave/flags.cpp, amd64 images 
> get downloaded due to which test cases fail on s390x with "Exec format Error"
> 
> 
> Diffs
> -
> 
>   src/linux/ldcache.cpp 6260496672cd0226a24ea1b34003a48af9678a63 
>   src/slave/flags.cpp 1eccea920338032173be4df6c374ec50dbd2eaf3 
> 
> 
> Diff: https://reviews.apache.org/r/55065/diff/1/
> 
> 
> Testing
> ---
> 
> make check 
> make check GTEST_FILTER="LdcacheTest.Parse"
> 
> 
> Thanks,
> 
> Ayanampudi Varsha
> 
>

Re: Review Request 55065: Added support for s390x architecture

[Till Toenshoff]

> On March 27, 2017, 7:01 p.m., Kevin Klues wrote:
> > src/slave/flags.cpp
> > Lines 152-158 (original), 152-169 (patched)
> > 
> >
> > We typically don't create duplicate entries like this when simply 
> > customizing the default value. Instead we use a lambda with the #ifdef's 
> > inside of it. For example,
> > 
> > https://github.com/apache/mesos/blob/master/src/slave/flags.cpp#L212

I am not sure we really need to come up with a lambda for such static default - 
see 
https://github.com/apache/mesos/blob/42f43ceb3b4b375357281030fb3816e548dac763/src/slave/flags.cpp#L670
Let's not make this more complicated than needed.


- Till


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


On March 27, 2017, 9:25 a.m., Ayanampudi Varsha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55065/
> ---
> 
> (Updated March 27, 2017, 9:25 a.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Vinod Kone.
> 
> 
> Bugs: MESOS-6742
> https://issues.apache.org/jira/browse/MESOS-6742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for s390x architecture
> There are 2 issues addressed in this patch:
> 1. LdcacheTest.Parse test case fails on s390x machines.
> 2. From the value of flag docker_registry in slave/flags.cpp, amd64 images 
> get downloaded due to which test cases fail on s390x with "Exec format Error"
> 
> 
> Diffs
> -
> 
>   src/linux/ldcache.cpp 6260496672cd0226a24ea1b34003a48af9678a63 
>   src/slave/flags.cpp 1eccea920338032173be4df6c374ec50dbd2eaf3 
> 
> 
> Diff: https://reviews.apache.org/r/55065/diff/1/
> 
> 
> Testing
> ---
> 
> make check 
> make check GTEST_FILTER="LdcacheTest.Parse"
> 
> 
> Thanks,
> 
> Ayanampudi Varsha
> 
>

Re: Review Request 55065: Added support for s390x architecture

[Till Toenshoff]

> On March 22, 2017, 2:45 p.m., Till Toenshoff wrote:
> > src/linux/ldcache.cpp
> > Line 120 (original), 120 (patched)
> > 
> >
> > Can you elaborate why this change is safe for all systems?
> 
> Ayanampudi Varsha wrote:
> We have tried this on Big endian system where 16 bit flag was causing 
> issue.
> Test case LdcacheTest.Parse failed at condition ASSERT_GT(cache->size(), 
> 1u); as size was returned as 0. This happens because the 16 bits after the 
> field "flags" is unused. 
> On x86_64, I have verified that this "cache->size()" is same with 16 bit 
> and 32 bit flag.
> Observed that the other fields in structure are all 32bit/64bit.
> An alternative would be to make changes only specific to s390x by putting 
> a condition. Please suggest
> 
> Till Toenshoff wrote:
> You may actually have found a bug in our ldcache parsing. After looking 
> into the glibc sources, I think this field should have been 32bit also for 
> x86_64 instead of 16bit - I suspect missing struct-packing for this bug to 
> not have surfaced earlier. For confirming this without mixing it with your 
> platform specific docker-repository change, I would like you to split this 
> patch into two.
> 
> So please add another patch that only fixes `flags` attribute size into 
> 32bit. Please make sure you add the following reviewers: tillt, klueska.
> 
> Remove that 32bit change from this patch and update it according to my 
> and Kevin's review comments.
> 
> Thanks a bunch for your work - this is highly appreciated.

Ow, for further reference of this `flags` change, here are the internal glibc 
structures: 
https://github.com/lattera/glibc/blob/master/sysdeps/generic/dl-cache.h#L80


- Till


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


On March 27, 2017, 9:25 a.m., Ayanampudi Varsha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55065/
> ---
> 
> (Updated March 27, 2017, 9:25 a.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Vinod Kone.
> 
> 
> Bugs: MESOS-6742
> https://issues.apache.org/jira/browse/MESOS-6742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for s390x architecture
> There are 2 issues addressed in this patch:
> 1. LdcacheTest.Parse test case fails on s390x machines.
> 2. From the value of flag docker_registry in slave/flags.cpp, amd64 images 
> get downloaded due to which test cases fail on s390x with "Exec format Error"
> 
> 
> Diffs
> -
> 
>   src/linux/ldcache.cpp 6260496672cd0226a24ea1b34003a48af9678a63 
>   src/slave/flags.cpp 1eccea920338032173be4df6c374ec50dbd2eaf3 
> 
> 
> Diff: https://reviews.apache.org/r/55065/diff/1/
> 
> 
> Testing
> ---
> 
> make check 
> make check GTEST_FILTER="LdcacheTest.Parse"
> 
> 
> Thanks,
> 
> Ayanampudi Varsha
> 
>

Re: Review Request 55065: Added support for s390x architecture

[Till Toenshoff]

> On March 22, 2017, 2:45 p.m., Till Toenshoff wrote:
> > src/linux/ldcache.cpp
> > Line 120 (original), 120 (patched)
> > 
> >
> > Can you elaborate why this change is safe for all systems?
> 
> Ayanampudi Varsha wrote:
> We have tried this on Big endian system where 16 bit flag was causing 
> issue.
> Test case LdcacheTest.Parse failed at condition ASSERT_GT(cache->size(), 
> 1u); as size was returned as 0. This happens because the 16 bits after the 
> field "flags" is unused. 
> On x86_64, I have verified that this "cache->size()" is same with 16 bit 
> and 32 bit flag.
> Observed that the other fields in structure are all 32bit/64bit.
> An alternative would be to make changes only specific to s390x by putting 
> a condition. Please suggest

You may actually have found a bug in our ldcache parsing. After looking into 
the glibc sources, I think this field should have been 32bit also for x86_64 
instead of 16bit - I suspect missing struct-packing for this bug to not have 
surfaced earlier. For confirming this without mixing it with your platform 
specific docker-repository change, I would like you to split this patch into 
two.

So please add another patch that only fixes `flags` attribute size into 32bit. 
Please make sure you add the following reviewers: tillt, klueska.

Remove that 32bit change from this patch and update it according to my and 
Kevin's review comments.

Thanks a bunch for your work - this is highly appreciated.


- Till


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


On March 27, 2017, 9:25 a.m., Ayanampudi Varsha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55065/
> ---
> 
> (Updated March 27, 2017, 9:25 a.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Vinod Kone.
> 
> 
> Bugs: MESOS-6742
> https://issues.apache.org/jira/browse/MESOS-6742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for s390x architecture
> There are 2 issues addressed in this patch:
> 1. LdcacheTest.Parse test case fails on s390x machines.
> 2. From the value of flag docker_registry in slave/flags.cpp, amd64 images 
> get downloaded due to which test cases fail on s390x with "Exec format Error"
> 
> 
> Diffs
> -
> 
>   src/linux/ldcache.cpp 6260496672cd0226a24ea1b34003a48af9678a63 
>   src/slave/flags.cpp 1eccea920338032173be4df6c374ec50dbd2eaf3 
> 
> 
> Diff: https://reviews.apache.org/r/55065/diff/1/
> 
> 
> Testing
> ---
> 
> make check 
> make check GTEST_FILTER="LdcacheTest.Parse"
> 
> 
> Thanks,
> 
> Ayanampudi Varsha
> 
>

Re: Review Request 55065: Added support for s390x architecture

[Kevin Klues]

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




src/slave/flags.cpp
Lines 152-158 (original), 152-169 (patched)


We typically don't create duplicate entries like this when simply 
customizing the default value. Instead we use a lambda with the #ifdef's inside 
of it. For example,

https://github.com/apache/mesos/blob/master/src/slave/flags.cpp#L212


- Kevin Klues


On March 27, 2017, 9:25 a.m., Ayanampudi Varsha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55065/
> ---
> 
> (Updated March 27, 2017, 9:25 a.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Vinod Kone.
> 
> 
> Bugs: MESOS-6742
> https://issues.apache.org/jira/browse/MESOS-6742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for s390x architecture
> There are 2 issues addressed in this patch:
> 1. LdcacheTest.Parse test case fails on s390x machines.
> 2. From the value of flag docker_registry in slave/flags.cpp, amd64 images 
> get downloaded due to which test cases fail on s390x with "Exec format Error"
> 
> 
> Diffs
> -
> 
>   src/linux/ldcache.cpp 6260496672cd0226a24ea1b34003a48af9678a63 
>   src/slave/flags.cpp 1eccea920338032173be4df6c374ec50dbd2eaf3 
> 
> 
> Diff: https://reviews.apache.org/r/55065/diff/1/
> 
> 
> Testing
> ---
> 
> make check 
> make check GTEST_FILTER="LdcacheTest.Parse"
> 
> 
> Thanks,
> 
> Ayanampudi Varsha
> 
>

Re: Review Request 55065: Added support for s390x architecture

[Ayanampudi Varsha]

> On March 22, 2017, 2:45 p.m., Till Toenshoff wrote:
> > src/linux/ldcache.cpp
> > Line 120 (original), 120 (patched)
> > 
> >
> > Can you elaborate why this change is safe for all systems?

We have tried this on Big endian system where 16 bit flag was causing issue.
Test case LdcacheTest.Parse failed at condition ASSERT_GT(cache->size(), 1u); 
as size was returned as 0. This happens because the 16 bits after the field 
"flags" is unused. 
On x86_64, I have verified that this "cache->size()" is same with 16 bit and 32 
bit flag.
Observed that the other fields in structure are all 32bit/64bit.
An alternative would be to make changes only specific to s390x by putting a 
condition. Please suggest


- Ayanampudi


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


On March 27, 2017, 9:25 a.m., Ayanampudi Varsha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55065/
> ---
> 
> (Updated March 27, 2017, 9:25 a.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Vinod Kone.
> 
> 
> Bugs: MESOS-6742
> https://issues.apache.org/jira/browse/MESOS-6742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for s390x architecture
> There are 2 issues addressed in this patch:
> 1. LdcacheTest.Parse test case fails on s390x machines.
> 2. From the value of flag docker_registry in slave/flags.cpp, amd64 images 
> get downloaded due to which test cases fail on s390x with "Exec format Error"
> 
> 
> Diffs
> -
> 
>   src/linux/ldcache.cpp 6260496672cd0226a24ea1b34003a48af9678a63 
>   src/slave/flags.cpp 1eccea920338032173be4df6c374ec50dbd2eaf3 
> 
> 
> Diff: https://reviews.apache.org/r/55065/diff/1/
> 
> 
> Testing
> ---
> 
> make check 
> make check GTEST_FILTER="LdcacheTest.Parse"
> 
> 
> Thanks,
> 
> Ayanampudi Varsha
> 
>

Re: Review Request 55065: Added support for s390x architecture

[Ayanampudi Varsha]

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

(Updated March 27, 2017, 9:25 a.m.)


Review request for mesos, Till Toenshoff and Vinod Kone.


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


Repository: mesos


Description
---

Added support for s390x architecture
There are 2 issues addressed in this patch:
1. LdcacheTest.Parse test case fails on s390x machines.
2. From the value of flag docker_registry in slave/flags.cpp, amd64 images get 
downloaded due to which test cases fail on s390x with "Exec format Error"


Diffs
-

  src/linux/ldcache.cpp 6260496672cd0226a24ea1b34003a48af9678a63 
  src/slave/flags.cpp 1eccea920338032173be4df6c374ec50dbd2eaf3 


Diff: https://reviews.apache.org/r/55065/diff/1/


Testing (updated)
---

make check 
make check GTEST_FILTER="LdcacheTest.Parse"


Thanks,

Ayanampudi Varsha

Re: Review Request 55065: Added support for s390x architecture

[Till Toenshoff]

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



Please make sure you fill out the "testing done" section with all the steps you 
used for validating your changes?


src/linux/ldcache.cpp
Line 120 (original), 120 (patched)


Can you elaborate why this change is safe for all systems?



src/slave/flags.cpp
Lines 152 (patched)


We generally use `#ifdef` instead of `#if defined()`.

Seems we could avoid a bit of duplication by only guarding the default 
value by the `ifdef`.


- Till Toenshoff


On March 22, 2017, 2:33 p.m., Ayanampudi Varsha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55065/
> ---
> 
> (Updated March 22, 2017, 2:33 p.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Vinod Kone.
> 
> 
> Bugs: MESOS-6742
> https://issues.apache.org/jira/browse/MESOS-6742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for s390x architecture
> There are 2 issues addressed in this patch:
> 1. LdcacheTest.Parse test case fails on s390x machines.
> 2. From the value of flag docker_registry in slave/flags.cpp, amd64 images 
> get downloaded due to which test cases fail on s390x with "Exec format Error"
> 
> 
> Diffs
> -
> 
>   src/linux/ldcache.cpp 6260496672cd0226a24ea1b34003a48af9678a63 
>   src/slave/flags.cpp 1eccea920338032173be4df6c374ec50dbd2eaf3 
> 
> 
> Diff: https://reviews.apache.org/r/55065/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ayanampudi Varsha
> 
>

Re: Review Request 55065: Added support for s390x architecture

[Mesos ReviewBot]

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



Bad review!

Reviews applied: []

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos ReviewBot


On Dec. 28, 2016, 8:24 a.m., Ayanampudi Varsha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55065/
> ---
> 
> (Updated Dec. 28, 2016, 8:24 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-6742
> https://issues.apache.org/jira/browse/MESOS-6742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for s390x architecture
> There are 2 issues addressed in this patch:
> 1. LdcacheTest.Parse test case fails on s390x machines.
> 2. From the value of flag docker_registry in slave/flags.cpp, amd64 images 
> get downloaded due to which test cases fail on s390x with "Exec format Error"
> 
> 
> Diffs
> -
> 
>   src/linux/ldcache.cpp 6260496672cd0226a24ea1b34003a48af9678a63 
>   src/slave/flags.cpp 1eccea920338032173be4df6c374ec50dbd2eaf3 
> 
> Diff: https://reviews.apache.org/r/55065/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ayanampudi Varsha
> 
>

Review Request 55065: Added support for s390x architecture

[Ayanampudi Varsha]

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

Review request for mesos.


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


Repository: mesos


Description
---

Added support for s390x architecture
There are 2 issues addressed in this patch:
1. LdcacheTest.Parse test case fails on s390x machines.
2. From the value of flag docker_registry in slave/flags.cpp, amd64 images get 
downloaded due to which test cases fail on s390x with "Exec format Error"


Diffs
-

  src/linux/ldcache.cpp 6260496672cd0226a24ea1b34003a48af9678a63 
  src/slave/flags.cpp 1eccea920338032173be4df6c374ec50dbd2eaf3 

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


Testing
---


Thanks,

Ayanampudi Varsha