mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benno Evers <bev...@mesosphere.com>
Subject Re: Review Request 68484: Enforced disabling boost debug mode.
Date Fri, 24 Aug 2018 11:18:21 GMT


> On Aug. 24, 2018, 10:44 a.m., Benjamin Bannier wrote:
> > src/master/master.hpp
> > Lines 33-34 (patched)
> > <https://reviews.apache.org/r/68484/diff/1/?file=2076510#file2076510line33>
> >
> >     IMO this solution is not maintable and too low level in Mesos proper. I believe
we should instead do one of following:
> >     
> >     1. Provide a header and type in `stout` wrapping `boost/circular_buffer.hpp`
which performs this setup. That way Mesos does not have to worry about low-level details which
are nicely encapsulated in `stout`.
> >     2. Add these defines whenever we make use of Boost (simple in cmake, slightly
more messy in autotools). That way users cannot forget to add them when using `boost/circular_buffer.hpp`.
> >     
> >     I personally would strongly prefer 1 over 2.

We certainly shouldn't do (2), I just recently had a review where I removed exactly such code
(https://reviews.apache.org/r/67632/) because it can lead to hard-to-understand build errors
when trying to build things outside the Mesos, i.e. building Frameworks.

We might think about (1), but it's not as clear-cut as you suggest because Boost's behaviour
is not unconditionally a bug, it only becomes one due to the specific interaction with the
request batching code in master. If any of our public headers end up including this new `stout/circular_buffer.hpp`,
this again might create problems for users including this header outside of the Mesos build
system.

Either way, I would suggest not doing any of this in *this* review, since there are two many
open design questions to be discussed (i.e. do we make `stout/boost.hpp` or one header for
every individual feature used, do we provide methods to configure the default options, etc.),
and we'd be holding up the 1.7 release while we argue over them.


- Benno


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


On Aug. 23, 2018, 4:25 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68484/
> -----------------------------------------------------------
> 
> (Updated Aug. 23, 2018, 4:25 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-9177
>     https://issues.apache.org/jira/browse/MESOS-9177
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Always disable boost debug mode.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 85ef14c1cc72180b746a5f4375769b653cbe511d 
> 
> 
> Diff: https://reviews.apache.org/r/68484/diff/1/
> 
> 
> Testing
> -------
> 
> * Compiled mesos master against boost 1.53 from https://downloads.mesosphere.com/pkgpanda-artifact-cache/boost_1_53_0.tar.gz.
> * Wrote a script that can trigger a segfault by reading from a circular buffer in parallel
(pasted in the linked ticket) and verified that it crashes mesos master.
> * Recompiled mesos with this patch applied.
> * Verified that the above script does not produce a segfault anymore.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>


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