Posted by sun on January 10, 2012 at 4:20pm
19 followers
Jump to:
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | other |
| Category: | task |
| Priority: | normal |
| Assigned: | Dries |
| Status: | needs review |
Issue Summary
I'd like to get these time thresholds for RTBC patches formalized and documented on the issue statuses page:
Copying from @catch's announcement on http://groups.drupal.org/node/179984 :
-
- Minimum 3-5 days in RTBC
- I'll also try to leave a window of at least 3-5 days for any patch that's not trivial before committing it, especially if it's the first time that issue has been at RTBC. People who keep an eye on the RTBC queue can use this time to perform a final review and/or knock things back to needs work before they're committed. Note that Dries and webchick may handle this differently, and we can always re-open issues for follow-ups and/or roll-backs to repair breakage.
-
- Maximum 4 weeks in RTBC without a review from a committer
- At the other end, I'll try to make sure no patch stays in the RTBC queue for more than 4 weeks without at least being reviewed by me. To keep that manageable I'm going to prioritize the RTBC queue above my other Drupal core activities, (like reviewing other patches or working on my own).
@catch applies these thresholds to all RTBC patches since he became core maintainer. With great success.
Tasks
- Community agreement on that this is a good thing.
- Agreement from @Dries, @webchick, and possibly @GáborHojtsy (ahem, especially @Dries, who's known for even committing non-RTBC patches... ;))
Comments
#1
#2
The question is: does formalization solve any problem here? AFAIR we always had a problem that RTBCs were lying around for a long time. Core committers have a hard time to keep up to date with all the RTBC issues, if they want to do anything else besides looking at the drupal.org issue queue all day long. So IMO catch is currently delivering a super human effort to keep the RTBC issues under control, but this is not sustainable and IMO leads to burnout.
I know this has been discussed way too often, but I'm gonna say it anyway: we need subsystem core maintainers (with commit access!) for relatively good decoupled and independent core areas (I'm looking at poll.module or the "documentation" category which has 6 mostly trivial RTBC issues right now).
Yes, now you may shoot me.
#3
@klausi: Availability of core maintainers is a completely different issue than setting up rules for how (or rather "when") to deal with RTBC issues/patches. The proposed thresholds here apply always, regardless of how many core maintainers are currently available, or how many of them. Let's focus on the actual proposal, please. :)
#4
Ok, some focus:
Rule 1) does totally make sense and is a no-brainer. Nothing to discuss here.
Rule 2) will only increase pressure on core comitters, IMO 4 weeks is not realistic in the current single branch committer process. So I would phrase it like this:
#5
The way @catch formulated the second threshold actually has an important background:
There have been a lot of issues in the past that were stuck on RTBC for almost years, without any kind of reasoning provided by the assigned core maintainer on what's wrong with the respective patch or why it wasn't committed yet.
The max threshold addresses exactly that. In a sense, it could be mapped to your "pressure" thinking, but entirely differently, in fact, removing pressure:
Makes sense?
#6
On day 30 of RTBC with no activity, System Message automatically commits the patch. :-P [Edit: yes, I'm kidding].
#7
Well, I think we can simultaneously adopt this and also acknowledge that:
From the outside, it seems like Dries, webchick, and catch have a pretty good balance for getting things committed. RTBCs have not been going stale or blocking other work these past 4 months, and the only patch that I saw get more than a month old was API docs cleanup for trigger. :P So I think the 4-week point isn't unreasonable. (Hopefully they can comment on whether that's how they see it as well. )
I'm definitely most concerned with point 1, because there are a lot of people who review the RTBC issues every day or few. When an issue goes from NR (or less) to committed in less than 24 hours, that disrupts the review process and can cause headaches for everyone if followups and rollbacks are needed. Plus, the people who review RTBCs also save the core committers time by helping filter out issues that are definitely not ready to be committed. :)
#8
How about automatically escalating the priority of an RTBC if it has been in the queue too long? This is similar to what has been instituted in the project applications issue queue. (Define "too long" however you like.)
I'm against automatically committing an RTBC patch after 30 days, as suggested (perhaps jokingly?) in #6.
Personally, I'd rather not formalize how long an RTBC patch can exist in that state - the core committers don't need that additional pressure. This should be more of a "guideline" rather than a rule.
#9
I was called out in the opening notes, so here is my take on this policy vs. Drupal 6. I'm not sure these rules are good to apply to Drupal 6. Yes, there are painfully old issues in the Drupal 6 RTBC queue but people finding those patches in the Drupal 6 RTBC queue also tend to push them back to needs work because there is no automated testing in Drupal 6 so there is a lot more manual testing required before a patch can be considered ready. In a sense, Drupal 6 would need two distinct RTBC states, the (a) at least someone thought this worked/looked good and (b) many people thought this worked/looked good.
It is not very rare in the Drupal 6 RTBC queue for issues breaking backwards compatibility or not taking fixing the same bug in Drupal 7 or 8 into consideration because people just want to get their stuff done quick. I've not seen people making reviewing the Drupal 6 RTBC queue a priority for a year plus so it became part of the process to leave RTBC issues lingering waiting for even more feedback before they can get committed.
As for Drupal 8, working on the Drupal 8 Multilingual Initiative I'm working with a bazillion of small patches to get to goals. They in and of themselves need some discussion so merging them is often not an option. However, leaving them sitting in the RTBC queue for more than 3 days IMHO is a mistake and hampers the progress of the initiative considerably. D8MI needs a constant stream of patch flow to reach even some of its goals. I'm trying my best to feed that flow from the queue both by producing patches and trying to help people produce them, but I clearly need more reviewers and then more timely core committer attention. I understand the motivation for 3 days, however D8MI patches are often between the trivial and mid-complex and are very far from the high complexity of patches proposed by CMI or WSCCI. So I'm not sure there is a one size fits all solution there.
#10
The point about patches waiting 3 days hampering D8MI seems sort of extreme to me. It's 3 days... and more often, any delay is due to the core committer's availability. One can't expect all the patches in a certain category to be committed immediately. Is D8MI using a sandbox? If not, then that would seem like a better solution than saying those patches need to be committed to core right away. And if so, then how does the state of core relative to the sandbox for a few days hamper development?
I agree that D6 is a much harder question because of the lack of a test suite, and I'm less sure about that than D7/8. But maybe the right status for those issues that still need review is Needs Review?
#11
On the other hand, maybe it's different when an initiative lead explicitly signs off on a patch. Still, though, I think that expecting patches to be committed immediately is kinda questionable, so it seems like it would be good to have a process for initiatives where an interval of a few days does not hamper work.
#12
@xjm: I was referring to more than 3 days, not 3 days being a problem for the kind of smallish patches I'm talking about. Yes, its core maintainer availability that causes the delay usually. Its also one of the core maintainers who appointed initiative leads and painted desires of big results. If you look at all fixed/closed issues with D8MI you'll see 52 issues at the moment: http://drupal.org/project/issues/search/drupal?status%5B%5D=15&status%5B... There are 262 days since Dries announced me to try and lead a multilingual initiative (the tag was not used before). Comparing those two numbers, its an issue per 5 days on average since May 9th 2011. It is a steep pace, but it is due to the bite sized chunks the work is broken down to. And that defines the "certain category" as you named, the bite sized chunks, not whether it is D8MI or not. I deliberately break it down to bite sized chunks to get stuff in faster and avoid working in vain.
The fact that it is lots of bite sized chunks and more often dependent on each other than not is just the nature of having a "theme" for these patches, the initiative's goals. And what might in fact make it more pressing to review/commit those vs. random other patches is the expectation of the big results. This happened before and will surely happen outside of officially named initiatives. You can think of the DBTNG effort in D7 or when form API was introduced in a haste. It is certainly not politically correct to say that initiative patches should have a fast lane, and I was not suggesting that. I'm rather saying that when there is a stream of dependent patches, the whole thing gets severely delayed if patches are not reviewed and committed in a timely manner.
Yes, people having huge goals can use sandboxes. It would be great to see successful examples of that. In my experience sandboxes make it very easy to go on a path for quite much longer without "outside" review, and the D8MI "team" if there is even one is very-very tiny for that. Recent examples, where the WSCCI people worked on a context system for example for months on and then posted in the core queue which was shot back on shortly. People demanded those issues be discussed and maintained in the core queue. It is not uncommon for core committers to demand further changes or not agree with those who worked on the patch, which makes dependent patches fail and sometimes dependent work that needs to be redone.
We are not just talking about delayed commits. RTBC issues are not universally going through unquestioned. We are talking about delayed core committer feedback.
#13
Sorry, but -1. I work on Drupal 7 as much as I'm physically/mentally able to do, given all of the other demands I have on my time. And a lot of that time I'm not spending committing on D7/D8 patches is time spent taking pressure off of catch and Dries either directly (participating in/pushing forward "bikeshed" discussions, escalating issues that need them to their attention) or indirectly (finding out from initiative owners what they need, doing outreach to try and get reviewers involved), to enable them to focus their time better on D8. If I'm held to this standard, other areas of the project are going to suffer. Period.
If the concern is that people are waiting too long for reviews, or too long from initial RTBC to core committer feedback/commit, then what we need is simply a view that shows this information that core committers (and other patch reviewers) can refer to. None of us would EVER intentionally ignore a patch from someone trying to make Drupal better. But often what happens is a testbot re-test marks something "needs work" coincides very closely with us hammering through the RTBC queue, so it moves off our radar. Or the issue gets bumped (maybe someone asking "Why has no one committed this yet?" :P) and so it appears to have only been sitting there for RTBC 2 days instead of 38 weeks, so when I'm doing FIFO processing of the RTBC queue, I miss it. Etc.
We need tools to provide better visibility to problem areas, not more rules governing volunteers' time.
#14
#13 implies that you are preferring those bikeshed discussions over issues that are RTBC already. Thats non-ideal, IMO.
#15
Well, for me at least it's a matter of context switching.
When I have 10-15 minutes in between phone calls, that's not enough to really get into the RTBC queue. For that I need more like 30 minutes+, and so I tend to do most of my RTBC queue work in the afternoons/evenings and weekends. However, that 10-15 minute timeframe is perfect for "communication-related" tasks: unsticking biksehed discussions, coordinating with contributors, firing off an email or two, etc. Moving Drupal core development forward is more than just committing patches, and this "soft skills" stuff catch explicitly said he did NOT want to do when taking on D8 co-maintainership, so I try and help off-set those tasks as much as I am able to.
I can't do much to restructure my life so I have more 30+ minute blocks of time. But I'm happy to use my RTBC queue time more efficiently. If i had the ability to sort the RTBC queue by "time waiting in RTBC queue" I would certainly do that. Unfortunately, all I have is priority, time since last comment, and total time an issue has been opened, so often these "chronic" issues can fall by the wayside. This isn't due to lack of dedication, or lack of rules governing my time, but simply lack of visibility.
#16
There is more to talk about on this but just quickly. At the moment at least I'd be opposed to any formal maximum time at rtbc status. I have made a best effort to keep things under a week at RTBC and that has worked most of the past four months, but a couple of weeks with very restricted free time threw that way off, and that comes down to there being very little slack in terms if committers, which is a different problem altogether.
However like webchick says it would be good to be able to order the queue by time at RTBC rather than last updated so it's actually possible to go through fifo. Also issues that block other issues as Gabor points out, but personally I would just bump those to major if there's an actual dependency (we can add blocking issues to the major tasks definition if we need to) since those get fast tracked by me anyway.
I do personally really like leaving issues RTBC for at least three days though, since it makes a big difference in terms of people being able to look through the RTBC queue for duds and weed them out, including spotting things I would have missed. Not everyone has read that groups post though, and afaik no one's talked to Dries about it directly, but for me at least it feels worth trying to standardize on. However even that there are exceptions - rollbacks, small documentation patches, unbreaking head etc. where it doesn't make sense.
#17
It seems like there is a lot of (understandable) discomfort with some expectation of an upward bound. What about the 3-day min time in RTBC, though, outside of the obvious exceptions catch mentions? That's the point that concerns me as a reviewer and developer.
Edit: One would count the 3 days from when the issue was marked RTBC, not when it was last "bumped".
#18
I personally think that when core committers have time to commit patches, they should commit patches. If we accidentally screw something up, it's easy enough to revert, or mark something back to "needs work." If I see something RTBC and don't commit it today, then there's a really good chance I don't get to it for another 2+ weeks, thus running into the upper-bound problem which is where most morale problems come from.
#19
And for clarification. That 2+ weeks isn't because I only commit patches every 2 weeks. On non-travel weeks, I usually have one decent 4+ hour stint a week, plus a few other 1-hour chunks here and there. But because we don't have a view that shows time spent in the RTBC queue, chances are that other issues will appear that are more on fire (major/criticals), easier to commit (documentation/minor), older (because this issue got bumped with a +1) or whatever other factor that causes them to get passed over during a given commit sprint.
Have I made this clear yet? :) We need a view. That's all we need. Not more rules.
#20
I think the maximum time in RTBC should be an individual decision by the core co-maintainers. It's voluntary work after all, and life happens. Also, I can't see any core co-maintainers neglecting their role, so it will probably not become a problem (at this time). But you never know, maybe the Drupal 9 co-maintainer will be a real slacker ;)
The minimum time is a good policy and can be easily implemented.
#21
For max time, I have to agree this is too much to ask of our maintainers. I've opened #1423530: Help project maintainers manage RTBC queue on a first-in, first-out basis in the project queue.
#22
Talked this out a bit in IRC. Several folks feel that trying to make this consistent across the board doesn't really get at or explore the underlying concerns, which webchick outlined as follows:
I think these underlying concerns are part of why catch outlined the process he did in the original post, and I guess the goal of this issue was to explore whether making it a standard would help reduce some of these concerns. I guess there's not enough consensus that it would. The "better RTBC view," however, seems like a good actionable followup.
#23
Assinging to myself so I can comment.
#24
Copying a comment from #1216978: Clean up the CSS for Contextual module which is relevant to this discussion:
The "RTBC defense crew" would be much better served focusing their extremely precious and valuable time on patches that truly need their due consideration: major/critical bugs, non-trivial tasks, API changes, etc. We should let the easy ones go in quickly, both to help keep morale of core contributors up (if a simple CSS restructure takes 100+ comments, that sends a huge red flag to spend volunteer time elsewhere), and worst-case to help fill the novice queue with simple tasks for for new contributors in the case where a committed patch requires minor tweaks or clean-ups.
Treating all of these patches the same, from a typo fix to an API change, is completely silly. Let's focus that energy where it's most valuable, get the RTBC queue moving. That's going to make everyone happier.
#25
#26
Oh my god! If Drupal 8 is not the place to be able to make mistakes, then where it is? If it is a bad message that developers are making mistakes then we should encourage more people *not* to work on core to improve Drupal's PR. After all those who don't work don't make mistakes. Many people understand this and rightfully stay away from Drupal core. We put up bazillions of gates and checkpoints to avoid making mistakes and we still make mistakes! So then we are even more terrified that we made mistakes despite all those checks and gates! So the solution is to add more of those constraints! Right? Or at least enforce those constraints even more aggressively!
No. I think the solution is to understand that we are *human* *volunteers* working on a *development* version and just accept the consequences.
#27
I really, really wish Drupal.org had a "+1000" button for comment #26.
#28
Round of applause anyone? :)