Operations Working Group - First Meeting

[Gastón Kleiman]

Hi all,

Thank you for responding to my previous emails - I think that we have
quorum for a new working group!

Many of you who have expressed interest seem to be in Europe, so I tried
schedule the first meeting at a time that I hope will be friendly for
people in both GMT+1 and GMT-8:

*Date:* Wednesday July 25th from 9:00-10:00 AM PDT
*Agenda:*
https://docs.google.com/document/d/1XjJfoksz70vbTvvT6FQ1t_J0SD1XIoipmYSvEHJfXt8/
*Zoom URL:* https://zoom.us/j/310132146


You can also find the event in the Mesos Community Calendar.

Feel free to add more topics to the agenda.

Looking forward to meeting you all next week,

-Gastón

Re: Backport Policy

[Vinod Kone]

I like how you summarized it Greg and I would vote for leaving the decision
to the committer too. In addition to what others mentioned, I think
committer should've the responsibility because if things break in a point
release (after it is released), it is the committer and contributor who are
on the hook to triage and fix it and not the release manager.

Having said that, if "during" the release process (i.e., cutting an RC)
these backports cause delays for a release manager in getting the release
out (e.g., CI flakiness introduced due to backports), release manager could
be the ultimate arbiter on whether such a backport should be reverted or
fixed by the committer/contributor. Hopefully such issues are caught much
before a release process is started (e.g., CI running against release
branches).

On Mon, Jul 16, 2018 at 1:28 PM Jie Yu  wrote:

> Greg, I like your idea of adding a prescriptive "policy" when evaluating
> whether a bug fix should be backported, and leave the decision to committer
> (because they have the most context, and avoid a bottleneck in the
> process).
>
> - Jie
>
> On Mon, Jul 16, 2018 at 11:24 AM, Greg Mann  wrote:
>
> > My impression is that we have two opposing schools of thought here:
> >
> >1. Backport as little as possible, to avoid unforeseen consequences
> >2. Backport as much as proves practical, to eliminate bugs in
> >supported versions
> >
> > Do other people agree with this assessment?
> >
> > If so, how can we find common ground? One possible solution would be to
> > leave the decision on backporting up to the committer, without
> specifying a
> > project-wide policy. This seems to be the status quo, and would lead to
> > some variation across committers regarding what types of fixes are
> > backported. We could also choose to delegate the decision to the release
> > manager; I favor leaving the decision with the committer, to eliminate
> the
> > burden on release managers.
> >
> > Here's a thought: rather than defining a prescriptive "policy" that we
> > expect committers to abide by, we could enumerate in the documentation
> the
> > competing concerns that we expect committers to consider when making
> > decisions on backports. The committing docs could read something like:
> >
> > "When bug fixes are committed to master, the committer should evaluate
> the
> > fix to determine whether or not it should be backported to supported
> > versions. This is left to the committer, but they are expected to weigh
> the
> > following concerns when making the decision:
> >
> >- Every backported change comes with a risk of unintended
> >consequences. The change should be carefully evaluated to ensure that
> such
> >side-effects are highly unlikely.
> >- As the complexity of applying a backport increases due to merge
> >conflicts, the likelihood of unintended consequences also increases.
> Bug
> >fixes which require extensive rebasing should only be backported when
> the
> >bug is critical enough to warrant the risk.
> >- Users of supported versions benefit greatly from the resolution of
> >bugs in point releases. Thus, whenever concerns #1 and #2 can be
> allayed
> >for a given bug fix, it should be backported."
> >
> >
> > Cheers,
> > Greg
> >
> >
> > On Mon, Jul 16, 2018 at 3:06 AM, Alex Rukletsov 
> > wrote:
> >
> >> Back porting as little as possible is the ultimate goal for me. My
> >> reasons are closely aligned with what Andrew wrote above.
> >>
> >> If we agree on this strategy, the next question is how to enforce it. My
> >> intuition is that committers will lean towards back porting their
> patches
> >> in arguable cases, because humans tend to overestimate the importance of
> >> their personal work. Delegating the decision in such cases to a release
> >> manager in my opinion will help us enforce the strategy of minimal
> number
> >> backports. As a bonus, the release manager will have a much better
> >> understanding of what's going on with the release, keyword: "more
> >> ownership".
> >>
> >> On Sat, Jul 14, 2018 at 12:07 AM, Andrew Schwartzmeyer <
> >> and...@schwartzmeyer.com> wrote:
> >>
> >>> I believe I fall somewhere between Alex and Ben.
> >>>
> >>> As for deciding what to backport or not, I lean toward Alex's view of
> >>> backporting as little as possible (and agree with his criteria). My
> >>> reasoning is that all changes can have unforeseen consequences, which I
> >>> believe is something to be actively avoided in already released
> versions.
> >>> The reason for backporting patches to fix regressions is the same as
> the
> >>> reason to avoid backporting as much as possible: keep behavior
> consistent
> >>> (and safe) within a release. With that as the goal of a branch in
> >>> maintenance mode, it makes sense to fix regressions, and make
> exceptions to
> >>> fix CVEs and other critical/blocking issues.
> >>>
> >>> As for who should decide what to backport, I lean toward Ben's view of
> >>> the burden 

Re: Backport Policy

[Jie Yu]

Greg, I like your idea of adding a prescriptive "policy" when evaluating
whether a bug fix should be backported, and leave the decision to committer
(because they have the most context, and avoid a bottleneck in the process).

- Jie

On Mon, Jul 16, 2018 at 11:24 AM, Greg Mann  wrote:

> My impression is that we have two opposing schools of thought here:
>
>1. Backport as little as possible, to avoid unforeseen consequences
>2. Backport as much as proves practical, to eliminate bugs in
>supported versions
>
> Do other people agree with this assessment?
>
> If so, how can we find common ground? One possible solution would be to
> leave the decision on backporting up to the committer, without specifying a
> project-wide policy. This seems to be the status quo, and would lead to
> some variation across committers regarding what types of fixes are
> backported. We could also choose to delegate the decision to the release
> manager; I favor leaving the decision with the committer, to eliminate the
> burden on release managers.
>
> Here's a thought: rather than defining a prescriptive "policy" that we
> expect committers to abide by, we could enumerate in the documentation the
> competing concerns that we expect committers to consider when making
> decisions on backports. The committing docs could read something like:
>
> "When bug fixes are committed to master, the committer should evaluate the
> fix to determine whether or not it should be backported to supported
> versions. This is left to the committer, but they are expected to weigh the
> following concerns when making the decision:
>
>- Every backported change comes with a risk of unintended
>consequences. The change should be carefully evaluated to ensure that such
>side-effects are highly unlikely.
>- As the complexity of applying a backport increases due to merge
>conflicts, the likelihood of unintended consequences also increases. Bug
>fixes which require extensive rebasing should only be backported when the
>bug is critical enough to warrant the risk.
>- Users of supported versions benefit greatly from the resolution of
>bugs in point releases. Thus, whenever concerns #1 and #2 can be allayed
>for a given bug fix, it should be backported."
>
>
> Cheers,
> Greg
>
>
> On Mon, Jul 16, 2018 at 3:06 AM, Alex Rukletsov 
> wrote:
>
>> Back porting as little as possible is the ultimate goal for me. My
>> reasons are closely aligned with what Andrew wrote above.
>>
>> If we agree on this strategy, the next question is how to enforce it. My
>> intuition is that committers will lean towards back porting their patches
>> in arguable cases, because humans tend to overestimate the importance of
>> their personal work. Delegating the decision in such cases to a release
>> manager in my opinion will help us enforce the strategy of minimal number
>> backports. As a bonus, the release manager will have a much better
>> understanding of what's going on with the release, keyword: "more
>> ownership".
>>
>> On Sat, Jul 14, 2018 at 12:07 AM, Andrew Schwartzmeyer <
>> and...@schwartzmeyer.com> wrote:
>>
>>> I believe I fall somewhere between Alex and Ben.
>>>
>>> As for deciding what to backport or not, I lean toward Alex's view of
>>> backporting as little as possible (and agree with his criteria). My
>>> reasoning is that all changes can have unforeseen consequences, which I
>>> believe is something to be actively avoided in already released versions.
>>> The reason for backporting patches to fix regressions is the same as the
>>> reason to avoid backporting as much as possible: keep behavior consistent
>>> (and safe) within a release. With that as the goal of a branch in
>>> maintenance mode, it makes sense to fix regressions, and make exceptions to
>>> fix CVEs and other critical/blocking issues.
>>>
>>> As for who should decide what to backport, I lean toward Ben's view of
>>> the burden being on the committer. I don't think we should add more work
>>> for release managers, and I think the committer/shepherd obviously has the
>>> most understanding of the context around changes proposed for backport.
>>>
>>> Here's an example of a recent bugfix which I backported:
>>> https://reviews.apache.org/r/67587/ (for MESOS-3790)
>>>
>>> While normally I believe this change falls under "avoid due to
>>> unforeseen consequences," I made an exception as the bug was old, circa
>>> 2015, (indicating it had been an issue for others), and was causing
>>> recurring failures in testing. The fix itself was very small, meaning it
>>> was easier to evaluate for possible side effects, so I felt a little safer
>>> in that regard. The effect of not having the fix was a fatal and undesired
>>> crash, which furthermore left troublesome side effects on the system (you
>>> couldn't bring the agent back up). And lastly, a dependent project (DC/OS)
>>> wanted it in their next bump, which necessitated backporting to the release
>>> they were 

Re: Backport Policy

[Greg Mann]

My impression is that we have two opposing schools of thought here:

   1. Backport as little as possible, to avoid unforeseen consequences
   2. Backport as much as proves practical, to eliminate bugs in supported
   versions

Do other people agree with this assessment?

If so, how can we find common ground? One possible solution would be to
leave the decision on backporting up to the committer, without specifying a
project-wide policy. This seems to be the status quo, and would lead to
some variation across committers regarding what types of fixes are
backported. We could also choose to delegate the decision to the release
manager; I favor leaving the decision with the committer, to eliminate the
burden on release managers.

Here's a thought: rather than defining a prescriptive "policy" that we
expect committers to abide by, we could enumerate in the documentation the
competing concerns that we expect committers to consider when making
decisions on backports. The committing docs could read something like:

"When bug fixes are committed to master, the committer should evaluate the
fix to determine whether or not it should be backported to supported
versions. This is left to the committer, but they are expected to weigh the
following concerns when making the decision:

   - Every backported change comes with a risk of unintended consequences.
   The change should be carefully evaluated to ensure that such side-effects
   are highly unlikely.
   - As the complexity of applying a backport increases due to merge
   conflicts, the likelihood of unintended consequences also increases. Bug
   fixes which require extensive rebasing should only be backported when the
   bug is critical enough to warrant the risk.
   - Users of supported versions benefit greatly from the resolution of
   bugs in point releases. Thus, whenever concerns #1 and #2 can be allayed
   for a given bug fix, it should be backported."


Cheers,
Greg


On Mon, Jul 16, 2018 at 3:06 AM, Alex Rukletsov  wrote:

> Back porting as little as possible is the ultimate goal for me. My reasons
> are closely aligned with what Andrew wrote above.
>
> If we agree on this strategy, the next question is how to enforce it. My
> intuition is that committers will lean towards back porting their patches
> in arguable cases, because humans tend to overestimate the importance of
> their personal work. Delegating the decision in such cases to a release
> manager in my opinion will help us enforce the strategy of minimal number
> backports. As a bonus, the release manager will have a much better
> understanding of what's going on with the release, keyword: "more
> ownership".
>
> On Sat, Jul 14, 2018 at 12:07 AM, Andrew Schwartzmeyer <
> and...@schwartzmeyer.com> wrote:
>
>> I believe I fall somewhere between Alex and Ben.
>>
>> As for deciding what to backport or not, I lean toward Alex's view of
>> backporting as little as possible (and agree with his criteria). My
>> reasoning is that all changes can have unforeseen consequences, which I
>> believe is something to be actively avoided in already released versions.
>> The reason for backporting patches to fix regressions is the same as the
>> reason to avoid backporting as much as possible: keep behavior consistent
>> (and safe) within a release. With that as the goal of a branch in
>> maintenance mode, it makes sense to fix regressions, and make exceptions to
>> fix CVEs and other critical/blocking issues.
>>
>> As for who should decide what to backport, I lean toward Ben's view of
>> the burden being on the committer. I don't think we should add more work
>> for release managers, and I think the committer/shepherd obviously has the
>> most understanding of the context around changes proposed for backport.
>>
>> Here's an example of a recent bugfix which I backported:
>> https://reviews.apache.org/r/67587/ (for MESOS-3790)
>>
>> While normally I believe this change falls under "avoid due to unforeseen
>> consequences," I made an exception as the bug was old, circa 2015,
>> (indicating it had been an issue for others), and was causing recurring
>> failures in testing. The fix itself was very small, meaning it was easier
>> to evaluate for possible side effects, so I felt a little safer in that
>> regard. The effect of not having the fix was a fatal and undesired crash,
>> which furthermore left troublesome side effects on the system (you couldn't
>> bring the agent back up). And lastly, a dependent project (DC/OS) wanted it
>> in their next bump, which necessitated backporting to the release they were
>> pulling in.
>>
>> I think in general we should backport only as necessary, and leave it on
>> the committers to decide if backporting a particular change is necessary.
>>
>>
>> On 07/13/2018 12:54 am, Alex Rukletsov wrote:
>>
>>> This is exactly where our views differ, Ben : )
>>>
>>> Ideally, I would like a release manager to have more ownership and less
>>> manual work. In my imagination, a release manager has 

Re: mesos git commit: Added mpsc_linked_queue and use it as the concurrent event queue.

[Dario Rexin]

Btw. I use 128 bytes here because of PowerPC. We don’t depend on it to perform 
well on PPC, but since there has been some work around PPC support I thought 
I’d make it work for it as well. If you think we should default to x86 cache 
line size, I’ll change that as well. 

> On Jul 16, 2018, at 8:39 AM, Dario Rexin  wrote:
> 
> Hi Benjamin,
> 
> see comments inline.
> 
>> On Jul 16, 2018, at 5:48 AM, Benjamin Bannier 
>>  wrote:
>> 
>> Hi Dario,
>> 
>> this patch introduced two new clang-tidy warnings. Could we try to get these 
>> down to zero, even if the code does not look bad?
>> 
>> 
>> I already created a patch for the unused lambda capture,
>> 
>>   https://reviews.apache.org/r/67927/
>> 
>> While the code does look reasonable, as a somewhat weird exception C++ 
>> allows referencing some variables without capturing them.
>> 
> 
> Yes, I was a bit confused by the MSVC error there. I added the explicit 
> capture because I thought it would be preferable over implicit capture, but 
> I’m fine with either. Thanks for creating the patch!
> 
>> 
>> I also looked into the warning on the “excessive padding”. Adding some 
>> explicit padding seems to make clang-tidy content, but I wasn’t sure whether 
>> we just wanted to put `head` and `tail` on separate cache lines, or also 
>> cared about the padding added after `tail`.
>> 
>>  private:
>> std::atomic*> head;
>> 
>> char padding[128 - sizeof(std::atomic*>)];
>> 
>> // TODO(drexin): Programatically get the cache line size.
>> alignas(128) Node* tail; // FIXME: IMO no need for `alignas` to 
>> separate `head` and `tail`.
>> 
>> Could you put up a patch for that? You can run the linter yourself; it is 
>> `support/mesos-tidy.sh`.
>> 
> 
> That’s interesting. The padding after tail is a good point, we should 
> definitely add that to prevent false sharing. If we add the padding, is 
> alignas still necessary?
> 
> Thanks,
> Dario
> 
>> 
>> Cheers,
>> 
>> Benjamin
>> 
>> 
>>> On Jul 15, 2018, at 7:02 PM, b...@apache.org wrote:
>>> 
>>> Repository: mesos
>>> Updated Branches:
>>> refs/heads/master a11a6a3d8 -> b1eafc035
>>> 
>>> 
>>> Added mpsc_linked_queue and use it as the concurrent event queue.
>>> 
>>> https://reviews.apache.org/r/62515
>>> 
>>> 
>>> Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
>>> Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/b1eafc03
>>> Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/b1eafc03
>>> Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/b1eafc03
>>> 
>>> Branch: refs/heads/master
>>> Commit: b1eafc035426bc39df4dba81c5c46b8b2d970339
>>> Parents: a11a6a3
>>> Author: Dario Rexin 
>>> Authored: Sat Jul 7 13:20:22 2018 -0700
>>> Committer: Benjamin Hindman 
>>> Committed: Sun Jul 15 09:55:28 2018 -0700
>>> 
>>> --
>>> 3rdparty/libprocess/Makefile.am |   1 +
>>> 3rdparty/libprocess/src/event_queue.hpp | 168 ++---
>>> 3rdparty/libprocess/src/mpsc_linked_queue.hpp   | 179 +++
>>> 3rdparty/libprocess/src/tests/CMakeLists.txt|   1 +
>>> 3rdparty/libprocess/src/tests/benchmarks.cpp|  64 ++-
>>> .../src/tests/mpsc_linked_queue_tests.cpp   | 104 +++
>>> 6 files changed, 367 insertions(+), 150 deletions(-)
>>> --
>>> 
>>> 
>>> http://git-wip-us.apache.org/repos/asf/mesos/blob/b1eafc03/3rdparty/libprocess/Makefile.am
>>> --
>>> diff --git a/3rdparty/libprocess/Makefile.am 
>>> b/3rdparty/libprocess/Makefile.am
>>> index 2d356aa..631491a 100644
>>> --- a/3rdparty/libprocess/Makefile.am
>>> +++ b/3rdparty/libprocess/Makefile.am
>>> @@ -307,6 +307,7 @@ libprocess_tests_SOURCES =  
>>> \
>>> src/tests/loop_tests.cpp\
>>> src/tests/main.cpp  \
>>> src/tests/metrics_tests.cpp \
>>> +  src/tests/mpsc_linked_queue_tests.cpp\
>>> src/tests/mutex_tests.cpp   \
>>> src/tests/owned_tests.cpp   \
>>> src/tests/process_tests.cpp \
>>> 
>>> http://git-wip-us.apache.org/repos/asf/mesos/blob/b1eafc03/3rdparty/libprocess/src/event_queue.hpp
>>> --
>>> diff --git a/3rdparty/libprocess/src/event_queue.hpp 
>>> b/3rdparty/libprocess/src/event_queue.hpp
>>> index 21c522d..999d552 100644
>>> --- a/3rdparty/libprocess/src/event_queue.hpp
>>> +++ b/3rdparty/libprocess/src/event_queue.hpp
>>> @@ -17,10 +17,6 @@
>>> #include 
>>> #include 
>>> 
>>> -#ifdef LOCK_FREE_EVENT_QUEUE
>>> -#include 
>>> -#endif // LOCK_FREE_EVENT_QUEUE
>>> -
>>> #include 
>>> #include 
>>> 
>>> @@ -28,6 +24,10 @@
>>> #include 

Re: mesos git commit: Added mpsc_linked_queue and use it as the concurrent event queue.

[Dario Rexin]

Hi Benjamin,

see comments inline.

> On Jul 16, 2018, at 5:48 AM, Benjamin Bannier 
>  wrote:
> 
> Hi Dario,
> 
> this patch introduced two new clang-tidy warnings. Could we try to get these 
> down to zero, even if the code does not look bad?
> 
> 
> I already created a patch for the unused lambda capture,
> 
>https://reviews.apache.org/r/67927/
> 
> While the code does look reasonable, as a somewhat weird exception C++ allows 
> referencing some variables without capturing them.
> 

Yes, I was a bit confused by the MSVC error there. I added the explicit capture 
because I thought it would be preferable over implicit capture, but I’m fine 
with either. Thanks for creating the patch!

> 
> I also looked into the warning on the “excessive padding”. Adding some 
> explicit padding seems to make clang-tidy content, but I wasn’t sure whether 
> we just wanted to put `head` and `tail` on separate cache lines, or also 
> cared about the padding added after `tail`.
> 
>   private:
>  std::atomic*> head;
> 
>  char padding[128 - sizeof(std::atomic*>)];
> 
>  // TODO(drexin): Programatically get the cache line size.
>  alignas(128) Node* tail; // FIXME: IMO no need for `alignas` to 
> separate `head` and `tail`.
> 
> Could you put up a patch for that? You can run the linter yourself; it is 
> `support/mesos-tidy.sh`.
> 

That’s interesting. The padding after tail is a good point, we should 
definitely add that to prevent false sharing. If we add the padding, is alignas 
still necessary?

Thanks,
Dario

> 
> Cheers,
> 
> Benjamin
> 
> 
>> On Jul 15, 2018, at 7:02 PM, b...@apache.org wrote:
>> 
>> Repository: mesos
>> Updated Branches:
>> refs/heads/master a11a6a3d8 -> b1eafc035
>> 
>> 
>> Added mpsc_linked_queue and use it as the concurrent event queue.
>> 
>> https://reviews.apache.org/r/62515
>> 
>> 
>> Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
>> Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/b1eafc03
>> Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/b1eafc03
>> Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/b1eafc03
>> 
>> Branch: refs/heads/master
>> Commit: b1eafc035426bc39df4dba81c5c46b8b2d970339
>> Parents: a11a6a3
>> Author: Dario Rexin 
>> Authored: Sat Jul 7 13:20:22 2018 -0700
>> Committer: Benjamin Hindman 
>> Committed: Sun Jul 15 09:55:28 2018 -0700
>> 
>> --
>> 3rdparty/libprocess/Makefile.am |   1 +
>> 3rdparty/libprocess/src/event_queue.hpp | 168 ++---
>> 3rdparty/libprocess/src/mpsc_linked_queue.hpp   | 179 +++
>> 3rdparty/libprocess/src/tests/CMakeLists.txt|   1 +
>> 3rdparty/libprocess/src/tests/benchmarks.cpp|  64 ++-
>> .../src/tests/mpsc_linked_queue_tests.cpp   | 104 +++
>> 6 files changed, 367 insertions(+), 150 deletions(-)
>> --
>> 
>> 
>> http://git-wip-us.apache.org/repos/asf/mesos/blob/b1eafc03/3rdparty/libprocess/Makefile.am
>> --
>> diff --git a/3rdparty/libprocess/Makefile.am 
>> b/3rdparty/libprocess/Makefile.am
>> index 2d356aa..631491a 100644
>> --- a/3rdparty/libprocess/Makefile.am
>> +++ b/3rdparty/libprocess/Makefile.am
>> @@ -307,6 +307,7 @@ libprocess_tests_SOURCES =   
>> \
>>  src/tests/loop_tests.cpp\
>>  src/tests/main.cpp  \
>>  src/tests/metrics_tests.cpp \
>> +  src/tests/mpsc_linked_queue_tests.cpp \
>>  src/tests/mutex_tests.cpp   \
>>  src/tests/owned_tests.cpp   \
>>  src/tests/process_tests.cpp \
>> 
>> http://git-wip-us.apache.org/repos/asf/mesos/blob/b1eafc03/3rdparty/libprocess/src/event_queue.hpp
>> --
>> diff --git a/3rdparty/libprocess/src/event_queue.hpp 
>> b/3rdparty/libprocess/src/event_queue.hpp
>> index 21c522d..999d552 100644
>> --- a/3rdparty/libprocess/src/event_queue.hpp
>> +++ b/3rdparty/libprocess/src/event_queue.hpp
>> @@ -17,10 +17,6 @@
>> #include 
>> #include 
>> 
>> -#ifdef LOCK_FREE_EVENT_QUEUE
>> -#include 
>> -#endif // LOCK_FREE_EVENT_QUEUE
>> -
>> #include 
>> #include 
>> 
>> @@ -28,6 +24,10 @@
>> #include 
>> #include 
>> 
>> +#ifdef LOCK_FREE_EVENT_QUEUE
>> +#include "mpsc_linked_queue.hpp"
>> +#endif // LOCK_FREE_EVENT_QUEUE
>> +
>> namespace process {
>> 
>> // A _multiple_ producer (MP) _single_ consumer (SC) event queue for a
>> @@ -187,185 +187,55 @@ private:
>> #else // LOCK_FREE_EVENT_QUEUE
>>  void enqueue(Event* event)
>>  {
>> -Item item = {sequence.fetch_add(1), event};
>>if (comissioned.load()) {
>> -  queue.enqueue(std::move(item));
>> 

Re: mesos git commit: Added mpsc_linked_queue and use it as the concurrent event queue.

[Benjamin Bannier]

Hi Dario,

this patch introduced two new clang-tidy warnings. Could we try to get these 
down to zero, even if the code does not look bad?


I already created a patch for the unused lambda capture,

https://reviews.apache.org/r/67927/

While the code does look reasonable, as a somewhat weird exception C++ allows 
referencing some variables without capturing them.


I also looked into the warning on the “excessive padding”. Adding some explicit 
padding seems to make clang-tidy content, but I wasn’t sure whether we just 
wanted to put `head` and `tail` on separate cache lines, or also cared about 
the padding added after `tail`.

   private:
  std::atomic*> head;

  char padding[128 - sizeof(std::atomic*>)];

  // TODO(drexin): Programatically get the cache line size.
  alignas(128) Node* tail; // FIXME: IMO no need for `alignas` to 
separate `head` and `tail`.

Could you put up a patch for that? You can run the linter yourself; it is 
`support/mesos-tidy.sh`.


Cheers,

Benjamin


> On Jul 15, 2018, at 7:02 PM, b...@apache.org wrote:
> 
> Repository: mesos
> Updated Branches:
>  refs/heads/master a11a6a3d8 -> b1eafc035
> 
> 
> Added mpsc_linked_queue and use it as the concurrent event queue.
> 
> https://reviews.apache.org/r/62515
> 
> 
> Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
> Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/b1eafc03
> Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/b1eafc03
> Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/b1eafc03
> 
> Branch: refs/heads/master
> Commit: b1eafc035426bc39df4dba81c5c46b8b2d970339
> Parents: a11a6a3
> Author: Dario Rexin 
> Authored: Sat Jul 7 13:20:22 2018 -0700
> Committer: Benjamin Hindman 
> Committed: Sun Jul 15 09:55:28 2018 -0700
> 
> --
> 3rdparty/libprocess/Makefile.am |   1 +
> 3rdparty/libprocess/src/event_queue.hpp | 168 ++---
> 3rdparty/libprocess/src/mpsc_linked_queue.hpp   | 179 +++
> 3rdparty/libprocess/src/tests/CMakeLists.txt|   1 +
> 3rdparty/libprocess/src/tests/benchmarks.cpp|  64 ++-
> .../src/tests/mpsc_linked_queue_tests.cpp   | 104 +++
> 6 files changed, 367 insertions(+), 150 deletions(-)
> --
> 
> 
> http://git-wip-us.apache.org/repos/asf/mesos/blob/b1eafc03/3rdparty/libprocess/Makefile.am
> --
> diff --git a/3rdparty/libprocess/Makefile.am b/3rdparty/libprocess/Makefile.am
> index 2d356aa..631491a 100644
> --- a/3rdparty/libprocess/Makefile.am
> +++ b/3rdparty/libprocess/Makefile.am
> @@ -307,6 +307,7 @@ libprocess_tests_SOURCES =
> \
>   src/tests/loop_tests.cpp\
>   src/tests/main.cpp  \
>   src/tests/metrics_tests.cpp \
> +  src/tests/mpsc_linked_queue_tests.cpp  \
>   src/tests/mutex_tests.cpp   \
>   src/tests/owned_tests.cpp   \
>   src/tests/process_tests.cpp \
> 
> http://git-wip-us.apache.org/repos/asf/mesos/blob/b1eafc03/3rdparty/libprocess/src/event_queue.hpp
> --
> diff --git a/3rdparty/libprocess/src/event_queue.hpp 
> b/3rdparty/libprocess/src/event_queue.hpp
> index 21c522d..999d552 100644
> --- a/3rdparty/libprocess/src/event_queue.hpp
> +++ b/3rdparty/libprocess/src/event_queue.hpp
> @@ -17,10 +17,6 @@
> #include 
> #include 
> 
> -#ifdef LOCK_FREE_EVENT_QUEUE
> -#include 
> -#endif // LOCK_FREE_EVENT_QUEUE
> -
> #include 
> #include 
> 
> @@ -28,6 +24,10 @@
> #include 
> #include 
> 
> +#ifdef LOCK_FREE_EVENT_QUEUE
> +#include "mpsc_linked_queue.hpp"
> +#endif // LOCK_FREE_EVENT_QUEUE
> +
> namespace process {
> 
> // A _multiple_ producer (MP) _single_ consumer (SC) event queue for a
> @@ -187,185 +187,55 @@ private:
> #else // LOCK_FREE_EVENT_QUEUE
>   void enqueue(Event* event)
>   {
> -Item item = {sequence.fetch_add(1), event};
> if (comissioned.load()) {
> -  queue.enqueue(std::move(item));
> +  queue.enqueue(event);
> } else {
> -  sequence.fetch_sub(1);
>   delete event;
> }
>   }
> 
>   Event* dequeue()
>   {
> -// NOTE: for performance reasons we don't check `comissioned` here
> -// so it's possible that we'll loop forever if a consumer called
> -// `decomission()` and then subsequently called `dequeue()`.
> -Event* event = nullptr;
> -do {
> -  // Given the nature of the concurrent queue implementation it's
> -  // possible that we'll need to try to dequeue multiple times
> -  // until it returns an event even though we know there is an
> -  // event because 

Re: Backport Policy

[Alex Rukletsov]

Back porting as little as possible is the ultimate goal for me. My reasons
are closely aligned with what Andrew wrote above.

If we agree on this strategy, the next question is how to enforce it. My
intuition is that committers will lean towards back porting their patches
in arguable cases, because humans tend to overestimate the importance of
their personal work. Delegating the decision in such cases to a release
manager in my opinion will help us enforce the strategy of minimal number
backports. As a bonus, the release manager will have a much better
understanding of what's going on with the release, keyword: "more
ownership".

On Sat, Jul 14, 2018 at 12:07 AM, Andrew Schwartzmeyer <
and...@schwartzmeyer.com> wrote:

> I believe I fall somewhere between Alex and Ben.
>
> As for deciding what to backport or not, I lean toward Alex's view of
> backporting as little as possible (and agree with his criteria). My
> reasoning is that all changes can have unforeseen consequences, which I
> believe is something to be actively avoided in already released versions.
> The reason for backporting patches to fix regressions is the same as the
> reason to avoid backporting as much as possible: keep behavior consistent
> (and safe) within a release. With that as the goal of a branch in
> maintenance mode, it makes sense to fix regressions, and make exceptions to
> fix CVEs and other critical/blocking issues.
>
> As for who should decide what to backport, I lean toward Ben's view of the
> burden being on the committer. I don't think we should add more work for
> release managers, and I think the committer/shepherd obviously has the most
> understanding of the context around changes proposed for backport.
>
> Here's an example of a recent bugfix which I backported:
> https://reviews.apache.org/r/67587/ (for MESOS-3790)
>
> While normally I believe this change falls under "avoid due to unforeseen
> consequences," I made an exception as the bug was old, circa 2015,
> (indicating it had been an issue for others), and was causing recurring
> failures in testing. The fix itself was very small, meaning it was easier
> to evaluate for possible side effects, so I felt a little safer in that
> regard. The effect of not having the fix was a fatal and undesired crash,
> which furthermore left troublesome side effects on the system (you couldn't
> bring the agent back up). And lastly, a dependent project (DC/OS) wanted it
> in their next bump, which necessitated backporting to the release they were
> pulling in.
>
> I think in general we should backport only as necessary, and leave it on
> the committers to decide if backporting a particular change is necessary.
>
>
> On 07/13/2018 12:54 am, Alex Rukletsov wrote:
>
>> This is exactly where our views differ, Ben : )
>>
>> Ideally, I would like a release manager to have more ownership and less
>> manual work. In my imagination, a release manager has more power and
>> control about dates, features, backports and everything that is related to
>> "their" branch. I would also like us to back port as little as possible,
>> to
>> simplify testing and releasing patch versions.
>>
>> On Fri, Jul 13, 2018 at 1:17 AM, Benjamin Mahler 
>> wrote:
>>
>> +user, I probably it would be good to hear from users as well.
>>>
>>> Please see the original proposal as well as Alex's proposal and let us
>>> know
>>> your thoughts.
>>>
>>> To continue the discussion from where Alex left off:
>>>
>>> > Other bugs and significant improvements, e.g., performance, may be back
>>> ported,
>>> the release manager should ideally be the one who decides on this.
>>>
>>> I'm a little puzzled by this, why is the release manager involved? As we
>>> already document, backports occur when the bug is fixed, so this happens
>>> in
>>> the steady state of development, not at release time. The release manager
>>> only comes in at the time of the release itself, at which point all
>>> backports have already happened and the release manager handles the
>>> release
>>> process. Only blocker level issues can stop the release and while the
>>> release manager has a strong say, we should generally agree on what
>>> consists of a release blocking issue.
>>>
>>> Just to clarify my workflow, I generally backport every bug fix I commit
>>> that applies cleanly, right after I commit it to master (with the
>>> exceptions I listed below).
>>>
>>> On Thu, Jul 12, 2018 at 8:39 AM, Alex Rukletsov 
>>> wrote:
>>>
>>> > I would like to back port as little as possible. I suggest the
>>> following
>>> > criteria:
>>> >
>>> > * By default, regressions are back ported to existing release
>>> branches. A
>>> > bug is considered a regression if the functionality is present in the
>>> > previous minor or patch version and is not affected by the bug there.
>>> >
>>> > * Critical and blocker issues, e.g., a CVE, can be back ported.
>>> >
>>> > * Other bugs and significant improvements, e.g., performance, may be
>>> back
>>> > ported, the release manager