GNOME.org
 

Re: Project is under heavy load

On Tue, 2018-04-24 at 15:39 +0000, Sander Striker wrote:

Hi Tristan,

I assume you've written this while experiencing the load on your
shoulders, and with the best intentions.  Please take my comments
inline in the same vain (sans the weight of maintainership on my
shoulders).



Hi Sander,

I found your reply to be refreshing and an enjoyable read :)


On Fri, Apr 20, 2018 at 12:18 PM Tristan Van Berkom <tristan vanberkom codethink co uk> wrote:

Hi all,

I have been tied up a lot these days, the project is under a lot of
load, and as a result the pressure on myself and individual
contributors alike is mounting, which is not helping, so lets try to
alleviate some of this pressure.

My goal with this email is to improve the efficiency of this project,
in terms of safely landing new features.


Debunking the magic bullet fallacy
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
A lot of people around here want to land a lot of new *features*, and I
want to be very clear that we are not talking about patches, we are
talking about features, most of which have a great impact on core
architecture and design.

Because of a number of circumstances, not least of which is an increase
an overall load, review cycles are becoming less efficient.

This has led some people to believe that:

  a.) We dont have enough people with the power to review patches.

      I agree with this, which is why I have delegated a lot of feature
      branch reviews to Jürg, while only discussing things between us
      at a high level in some (not all) circumstances.

      I want to find another lieutenant who can review feature
      additions, but for this to happen people need to understand the
      codebase as a whole, care about how the codebase evolves as a
      whole, and beyond this they have to earn my trust such that I am
      assured we are united in a common vision.

      Unfortunately this takes time, and I find myself lacking in
      suitable candidates, but believe me that I am hungry to find one.


I think we can be doing something to make it easier for new members of the
project to find their way in the codebase.  HACKING.rst does explain rules
around submissions, the coding style, etc, but it doesn't refer to an overall
architecture.
I think we can make it more feasible for people to understand the whole
codebase by providing an architecture overview with references to the
codebase.
One perspective is to also follow the flow of the `bst` commandline client,
and explaining what happens where.  In light of the BuildStream as a
library thread, other perspectives will follow further down the line.
I'll try to contribute something in this space, but will solicit some
help :)


Yes, I'd like to have an architectural document, which of course is a
tricky balance because, it adds to the list of things to be maintained.

I have some initial design material which I can perhaps reuse, which
includes some cute libreoffice diagrams describing the data model which
can be of help... I will try to find time to make something like this
happen.


Lastly, I am not sure I agree with the need to understand the codebase
as a whole for everyone with the commit-bit set.


Note that in this section (a), I'm talking about lieutenants who can
review feature additions almost independently, as I have been able to
trust Jürg with only some minor touch base discussions here and there.
Since either Jürg or I are usually tied up in other things, it would be
really nice if we could grow a third.

The bar for this is much higher than the bar for simply having the
commit-bit set, which is what I discuss in (b) below.


As the project
progresses and becomes more complex (we hopefully are able to
guard against over-complexity), specialism is bound to happen.


Right, I would love for an "Artifact Cache Server Developer" to
manifest themselves and be able to trust that person to review a lot of
things in that domain with very little oversight (as an example).



  Either
out of interest or for other reasons.  One example that springs to mind
is documentation.  I could also see this happen for other areas (e.g.
Sandbox).  Which is a segway into:
 

  b.) A core developer should be able to land patches without review,
      and fix later if a problem arises.

      In my experience I have often worked this way, and we currently
      do have a short list of committers in BuildStream. It works well
      as long as said core developers are personally responsible and
      accountable for the patches they are sure about.

      For a contributor to retain committer status, I have to know that
      the "fix it later" part has an immediate turnaround and takes
      precedence over all other ongoing work, otherwise this is a
      violation of trust, and it means that "being sure about the
      commit" is in fact a lie.


I wouldn't quite put it so negative.  Or rather it depends on your
definition of being sure.  Mistakes happen, and should be addressed.
If another helpful soul addresses the issue before the original
committer turns around a fix, that is totally fine.  Timezones, life,
are reasons why someone could not _immediately_ turn something
around.  And we can mitigate negativity here by a) putting a
reasonable turnaround time on a fix, and b) being able to revert
the change and that being ok to do in absence of an immediate
fix.


Fair, I am being a bit hard with the hopes of driving my point home,
lets try to rephrase this then.

This is not so much about being sure as it is about backing up your
sureness with accountability. Sureness is rather meaningless, if the
decision to be sure bears no gravity.

If I see that there are regressions left unattended while the
responsible developers continue to focus on additional features (and I
have certainly been seeing at least some of this), this is proof that I
cannot entrust said developers with direct commit rights.

I'll admit that for the more serious and hard to fix regressions, I
haven't been giving other developers a chance to fix their own
regressions. This is because in the current climate, I can't afford to
let a serious regression live for over a day, I have to be sure it's
fixed right now, so I have to fix it myself.

This situation should become a bit more relaxed as we make more stable
releases, currently our main user base needs to use 1.1.x bleeding
edge, so stress levels are certainly heightened by this.


      Also, this cannot mean unreviewed landing of feature additions,
      which necessarily impact API stable interfaces or make
      significant changes to core architecture and design, which may
      not need detailed code review, but needs discussion an approval.


I think that is fair.  Agreeing upon a design, upon an API, and potentially
an approach beforehand will prevent surprises.  Ideally this also identifies
others that are interested in the feature, which should help with the
code review process.
 

      We are not suffering from an influx of easily reviewable bug
      fixes which are not landing, which happens sometimes in larger
      and more mature software projects: We are suffering from an
      influx of many large and complicated feature branches that all
      want to land now, so it should be noted that having more
      committers in this scenario does not fix the current problem.


There's also small and less complicated features, which do take away
review time from the more complex ones.

And the burden of actually landing the changes, where it's not as simple
as hitting the Merge button on the GitLab UI...

One of my big concerns here is that Jurg and you, due to the burden that
comes with being a maintainer, are spending time that would otherwise
bring more value to the community in the form of other contributions.


I do understand this and feel this burden.

When writing this email I've compared this project to other projects
I've been either maintainer or part of a maintainer group for, and it
dawned on me that we are not strict enough in terms of what we expect
of contributors, we're doing too much legwork on the behalf of our
contributors.

There are a lot of situations which have arisen in BuildStream in
which, if this were another FOSS project I would expect people to just
go think of a solution that works, solve the problem on their own and
formally propose a solution once they've done the initial legwork and
fully considered all the edge cases themselves.

I think the excessive hand holding we've been doing has multiple
negative outcomes:

  * We're doing the hard part of contributors work for them, solving
    the problems and just letting them code the solution; this detracts
    a lot from our ability to also write software, as you correctly
    point out.

  * We can't really expect contributors to grow into core developers
    which we can entrust with commit rights, if we keep holding their
    hands to such a high degree. We need to throw them in the deep end
    so they can learn to swim.



  c.) The bar is too high for landing patches.

      This is just patently false. The last two weeks I've spent fixing
      regressions, and untangling structural weakness in our codebase
      is proof enough to me that the bar needs to be raised, not
      lowered.


Just some thoughts... is the current model mentally enabling
non-maintainers to offload the responsibility of overall codebase quality
to the maintainers?  Sense of ownership and pride and joy are to an
extent enabled by having control and responsibility.  It also allows
the load to be spread more evenly (or at least less disproportionally).


I fully agree, and I think this ties into the same above, this is a
third negative which is rooted in excessive hand holding in the design
and conception phase.

  * If we hold peoples hands every step of the way, solving their own
    problems for them, they will not gain the full sense of pride and
    ownership from the resulting work.

    As you point out, this means that they might not appreciate the
    quality of codebase as a virtue in itself, and not have the drive
    to protect that quality moving forward.


In summary here, there is no magic bullet which will allow your
features to land faster, even if we had more core maintainers, large
core features could not safely land any faster, and relaxing the review
process here can only result in a complete disaster.


I have a less apocalyptic view :).  I understand your perspective though.


What can we do to make the project more efficient ?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Asides from the fact that you just cannot land a lot of features at the
same time safely, there are a few things we can do to greatly improve
the efficiency in how we land features.

On my part, I'm going to try to do a better job in roadmapping, I had
this working much better last year but in recent times I've been too
tied up with specific goals to paint a better picture here.


There were also a lot less people involved :).  Even if we do roadmap,
does that preclude others from working on things not on the roadmap?
If someone has a hypothetical itch to scratch, I assume we still welcome
those contributions?


To a certain extent, of course we welcome features we haven't thought
of yet or roadmapped yet, but people working on these should probably
attend the meetings and make some attempt to get it on the roadmap.

The point of the roadmap I think is to reduce friction between features
landing in parallel; sometimes it pays to wait for something essential
to have landed first before starting on your feature which may be
severely impacted by a core change.

Further, there has to be limits to what we accept in given timeframes,
this is not about excluding features but rather being realistic about
what is ready to land before the next stable release.

I can't say for sure that publishing and maintaining the roadmap will
significantly improve our lives, however I think that as it stands
there are certain members who are "in the know" about what is up and
coming, and it would probably benefit contributors outside of that
immediate circle to have an idea of our direction.


In order for Jürg and I to handle review of your features more
efficiently, I'm going to lay out a list of things you can do to make
all of our lives easier.


  Round tripping is more expensive than actual review
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  The life of a maintainer is wrought with micro interruptions and
  context switches, as such it is important to state all of the facts
  at once and avoid round tripping.

  Unless it is clearly in context in a mailing list thread or issue
  report thread: Never assume we know what you are talking about, state
  all of the facts and provide links.

  When writing an email to the list, or a comment on an issue report,
  provide _all_ of the details at once. Dont attempt to entice me into
  a long discourse, instead strive to provide a clearly thought out
  problem statement with the goal of having it solvable in a single
  reply.

  You may find that half of the time, going through the exercise of
  clearly stating a problem in an email results in your having
  understood the problem, and sending the email becomes unnecessary
  (while the activity of writing it was valuable).


There are cases where still posting it with a TL;DR disclaiming it is
a self answering message, is adding value for the community.


Agreed :)

I think this is one of the more important exercises for beginners and
something you learn in FOSS, and from mailing list style communication
in general:

   You think you have a question, but you just dont know if you really
   have a question until you've spent the time accurately stating it.



  Only submit patches that you believe will land
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  We've been very relaxed in the last year about how we do reviews,
  patch review has been a place for mentoring and discussion about
  how to land the patch.

  Remove this from our workflow by only submitting patches for review
  that you are personally near certain will land.

  This is impossible for a first time submitter, and we understand
  this. Don't be shy to submit your first patch, and if you have team
  mates who regularly submit patches to BuildStream, get them to review
  it first before submitting it for review in our tracking system.


 

  I guarantee that the patches you think are ready to land, will
  most often not be ready. But I also guarantee that this will
  *greatly* reduce the round tripping which is bogging us down and
  clogging up the review pipeline.


An alternative/addition is to do non-maintainer round tripping.  That is,
work with others and get someone to give it a thumbs up before landing
it in the maintainer queue.
Don't take this out of view into private repos, because other community
members (and potentially maintainers) may still be interested in how you
are making the feature sausage.
Submitting with a WIP prefix should help this process, right?


Yes.

This is exactly what I expect WIP merge requests to be used for; peer
review and sharing purposes before proposing to upstream.

Unfortunately gitlab is not giving me any button to completely filter
out WIP merge requests, which is frustrating, but beside the point.

I would further ask people: Please stop creating WIP merge requests
unless they have *some* kind of purpose. I think people just create
them because gitlab gives them a link to create them when pushing a
branch, I've seen WIP merge requests which have *no patches* at all on
them, like they are just a location where they intend to eventually do
some work, this adds unnecessary noise.

It's perfectly fine to do work on a branch before creating a WIP merge
request, the branch wont disappear.



  Try to leave the code in a better state than how you found it
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  In many cases, there are multiple ways to implement a feature,
  usually these consist of a quick and dirty way, and a way which makes
  sense.

  Choose the way which makes sense, even though it almost certainly
  costs a bit more legwork in the short term.

  The quick and dirty fix is almost always going to be percieved as a
  threat to the longevity of the project at large, and it is our
  position as maintainers to defend the project against such threats.

  Dont make us feel threatened by your patches, instead try to share
  in the responsibility of making the whole codebase better all the
  time.


The above, intentional or not, reads really combative.  And for me
at least introduces an "us vs them" storyline :/.  Again, I'd like us to
use more positive messaging, which can achieve the same goal.


I'm not sure how I can rephrase this one, it's difficult: the point is
exactly to please not create a combative atmosphere around patch
review.

Lets try some other combination of words then:

    We always have the expectation of good faith for any submitted
    patch, and you are not walking into a snake pit by submitting your
    first patch.

    If we find that it makes a mess, or brings one part forward while
    leaving another part behind, and possibly susceptible to bit rot,
    we will tell you why.

    As long as you are not extremely obstinate and opinionated about
    it, there will not be any friction.

I'll note that I have not had a very bad experience in this regard
*yet* in BuildStream in comparison with other projects, but it always
happens and is a fact of life: If patches dont care about quality, and
leave a mess behind, and it becomes a pattern, it makes us lose
interest in reviewing those patches.



  Try your best to suggest a solution, or more than one solution
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  When you have a feature to implement and you see the current
  architecture does not "fit" with what you intend to achieve,
  this means that we need to reconsider the architectural design
  to afford for the feature you want to bring to our users.

  In these cases, try to be creative and think of a solution which
  best fits the architectural design of the current codebase, and
  propose that solution.


This goes back to the ability to easily verify against the architectural
design of the current codebase.


I really dont think it's as simple as this; having an architectural
design draft may prove to be more of a maintenance burden than a
benefit; it would be more ideal if the codebase itself were more self
explaining.

Granted, it can help a bit for newcomers to more easily contribute, but
it wont go as far as enabling developers to foresee edge cases and
write fault tolerant code, that requires testing, deeper thought
processes, and in depth reading of code I think.


  In other words, try to make it easier for the core maintainers to do
  their jobs: it's much easier for us to look at a proposed solution
  and identify problems with that, and possibly point out problems with
  the proposal for you to work on, than to do legwork and come up with
  a plan ourselves to solve the problem you want to have solved.


Of course.  That doesn't mean feature proposals without solutions are
invalid and should not be filed, correct?  Maybe set the expectation that
if it's not on the roadmap, maintainers won't contribute to features that
have no solution proposals.


Certainly it does not mean that; we're talking about contributors who
are proposing that we accept a body of work into the codebase.

Anyone who is not even a developer should feel welcome to enter an
issue report saying:

    "Damn it would be nice if BuildStream did foo".

I think that only half answers your question, though.

The amount of guidance that a maintainer can provide is going to vary
depending on project load; and even for the most seasoned contributors,
I would expect to have to spend a good initial 30 min to 1 hour of IRC
discussion to get the contributor off the ground; lets call this
kickstarting vs. design.

Kickstarting:

    For a complex feature, I would expect to point the contributor to
    the relevant code paths which are in some way impacted by the
    feature, and raise some issues, complications and roadblocks which
    I would expect the contributor to solve.

Design:

    This involves actually solving the complications and roadblocks,
    and ensuring that the solutions fit with other current ongoing
    developments. Also this includes ironing out the structural changes
    to the architecture such that the codebase is maintainable
    afterwards and moves forward as a whole instead of growing weird
    limbs. This part results in a proposed solution.

I think that it's fair to say that we should hopefully always have time
for Kickstarting. For Design on the other hand, the amount of attention
we can give is completely contingent on project load.

While help in Design might very often be provided, it is unfair for it
to be expected, the expectation should be that the contributor comes up
with a solution to propose themselves.



  Solve the mentoring problem outside of the maintainership
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  Last year I spent a great deal of time coaching younger developers in
  the ways of writing maintainable software.

  I actually love teaching, so I am particularly at risk of falling
  prey to an invitation to mentor you on how to write your patch.


:)
 

  In the present climate however, it is unreasonable for me to be
  spending my time in such activities - when I happen to find time, I
  will indulge myself.

  If you have juniors on your team, then have your senior developers
  mentor them as they develop features for BuildStream, help them write
  proposals to the mailing list before hand to ensure that we are first
  in agreement on landing the feature and that the architectural
  changes involved are going to be accepted.

  Mentor them to be good at submitting changes to FOSS projects.


  Respect the style in place and be consistent
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  It is of great importance that the codebase remain consistent such
  that it can be read and understood as a whole to any newcomers.
  Linters are not sufficient to address this.

  Not all of our coding practices are the best, if you do not agree
  with a practice, propose that we change it, and if we agree to change
  it, we will expect a patch which changes the practice *unilaterally*
  across the codebase.

  It is not acceptable to leave the codebase in a state where things
  are done in one way here, and in another way there.

  This should really go without saying: Don't let your personal taste
  in coding style get in the way of having a consistent code base, this
  can only be a constant source of friction in the review process which
  will waste time and leave everyone dissatisfied.


 


Summary
~~~~~~~
In closing, there is a lot we can do to improve the flow of reviews for
the many features that all want to land - increasing the volume and
pressuring for more reviews more often from the maintainership is not
going to magically make the feature branches safe to land sooner.

Let's be more professional and productive, share the responsibility
more evenly, and while we wont have any magic bullet, we will certainly
improve the situation.
And by all means, lets reserve some measure of time for horsing around,
poking fun at one another, and laughing a bit from time to time.

Keep it light hearted and fun, and we'll all have a great time while
accomplishing great things.


I'm glad you closed out with these positive notes :).


And I'm grateful that you've managed to interpret the intents of this
email very well.

Cheers,
    -Tristan
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]