Re: [infinispan-dev] How to improve our PR queue
by Tristan Tarrant
[Moving to infinispan-dev]
I agree with all of you.
It is not respectful towards the developer who has opened the PR to let
it linger for longer than necessary, and also for the developer who has
opened it not to react on the comments (that happens too!)
Let's set some ground rules for Pull Requests (this should go in the
Contribution guide):
Rules for everybody
- Each and everyone of the core engineers looks at the PR queue at least
once per day. More frequently if possible, especially if engaging in an
active collaborative review.
- We have more than one project: cachestores, console, integrations,
quickstarts, clients, website...
Rules for PR owners:
- PRs *must* be kept "applicable" (no conflicts)
- non-trivial PRs *must* have a corresponding Jira and the link to it
*must* be present in the PR description. Viceversa, the Jira's workflow
must be advanced to "Pull Request Sent" and include the link to the PR
- if the PR is for multiple branches and it cherry-picks cleanly to all
of them, no need to open multiple PRs. Use the "Backport" label to
indicate this
- if there is a need for separate PRs for different branches, clearly
indicate the non-master branch it needs to be applied to in the PR title
using the [branch] notation
- Ensure the correct labels are applied when opening a PR or when the
status changes
- The owner *must* react to comments / CI failures / requests for rebase
within 24 hours. Reaction means acknowledgement, not necessarily being
able to fix the issues
- If the owner cannot fix the issues related to comments / CI within a
week, the PR should be closed and reopened when they can be addressed
- While we still have some intermittent CI failures, we're in a
situation where it is quite reliable. Exceptions obviously happen.
Rules for PR reviewers
- A PR should be acted upon (commented, merged) within 24 hours of its
opening
- A PR can be commented upon even if CI hasn't finished its job
- PRs opened by external developers won't have the appropriate labels
for permission reasons. Add them.
- If the owner has addressed all comments / CI failures, the PR should
be merged within a week
- Some effects of a PR cannot be detected by CI. This for example
includes verifying that docs / PDFs / etc render correctly, that
distribution packages contain the appropriate files, etc. Do your best
to evaluate these.
With that being said, the solution for the current situation is to
divide the queue among ourselves and work through it using the sprint
approach (but we need to be careful with stability: "commit storms" can
be detrimental).
Tristan
On 05/10/16 09:39, Radim Vansa wrote:
> I think that can be combined: sprint just explicitly prioritizes PRs
> instead of normal development, so you won't be reviewing PRs only when
> "you have a free spot" but "until you can't review anything else". So
> once the PR count hits a threshold, Tristan schedules sprint.
>
> R.
>
> On 10/05/2016 09:17 AM, Sebastian Laskawiec wrote:
>> That's an interesting idea... but after a month or two, the PR queue
>> will pile up again and we will need another sprint...
>>
>> IMO we should have a day-to-day strategy for doing this.
>>
>> On Wed, Oct 5, 2016 at 9:07 AM, Gustavo Fernandes <gfernand(a)redhat.com
>> <mailto:gfernand@redhat.com>> wrote:
>>
>> I propose doing with PRs the same we've been doing to areas that
>> require some care, the so called 'sprints'.
>>
>> We are in more than 10, a couple of PRs integrated each and we can
>> get
>> rid of most of the queue.
>>
>> Gustavo
>>
>> On Wed, Oct 5, 2016 at 7:36 AM, Sebastian Laskawiec
>> <slaskawi(a)redhat.com <mailto:slaskawi@redhat.com>> wrote:
>> > Hey guys!
>> >
>> > I'm not sure what do you think about our PR queue but for me it
>> simply
>> > sucks...
>> >
>> > Some of our PRs are sitting there since I remember (like this
>> one [1] - Nov
>> > 2015!!! I guess we should prepare an anniversary cake, November
>> will is
>> > really soon :D) and some of them have more than 150 comments [2]
>> (maybe this
>> > sounds weird since it's my PR, but if something is not good
>> enough after 150
>> > comments, maybe we should throw it away and try to implement it
>> again -
>> > differently?).
>> >
>> > After thinking about this for a little while, I would like to
>> propose one
>> > rule for reviewing PRs - when you have a free spot and you'd
>> like to
>> > integrate a couple of PRs - always start with the oldest and go
>> through all
>> > "Ready for review" PRs.
>> >
>> > I know that integrating some new stuff will be tempting but
>> please don't do
>> > that. Even if the new PR modifies only one small line.. noooo.
>> Go through
>> > some old stuff instead. This way we should revisit old PRs much
>> more
>> > frequently than new ones and this will force us to make some
>> tough decisions
>> > that we avoided for quite some time (e.g. whether or not we
>> should integrate
>> > [1] or [2] - but those are only examples).
>> >
>> > And finally, I think we should have more courage to integrate
>> PRs especially
>> > into master branch... After all, we have all machinery in place
>> - nightly CI
>> > jobs, stress tests, performance tests - what bad can happen? We
>> break the
>> > build? So what? We can always fix it by git revert.
>> >
>> > What do you think? Maybe you have better ideas?
>> >
>> > Thanks
>> > Seb
>> >
>> > [1] https://github.com/infinispan/infinispan/pull/3860
>> <https://github.com/infinispan/infinispan/pull/3860>
>> > [2] https://github.com/infinispan/infinispan/pull/4348
>> <https://github.com/infinispan/infinispan/pull/4348>
>>
>>
>
>
--
Tristan Tarrant
Infinispan Lead
JBoss, a division of Red Hat
7 years, 7 months
if (trace) logger.tracef - it makes sense
by Sebastian Laskawiec
Hey!
A while ago I asked Radim and Dan about these kind of constructs [1]:
private boolean trace = logger.isTraceEnabled(); //stored in a field
... called in some method ...
if(trace)
logger.tracef(...);
...
At first they seemed wrong to me, because if one changes logging level
(using JMX for example), the code won't notice it. I also though it's quite
ok to use tracef directly, because JIT will inline and optimize it.
Unfortunately my benchmarks [2] show that I was wrong. Logger#tracef indeed
checks if the logging level is enabled but since JBoss Logging may use
different backends, the check is not trivial and is not inlined (at least
with default settings). The performance results look like this:
Benchmark Mode Cnt Score Error Units
MyBenchmark.noVariable thrpt 20 *717252060.124* ± 13420522.229 ops/s
MyBenchmark.withVariable thrpt 20 *2358360244.627* ± 50214969.572 ops/s
So if you even see a construct like this: logger.debuf or logger.tracef -
make sure you check if the logging level is enabled (and the check result
is stored in a field).
That was a bit surprising and interesting lesson :D
Thanks
Sebastian
[1] https://github.com/infinispan/infinispan/pull/4538#discussion_r80666086
[2] https://github.com/slaskawi/jboss-logging-perf-test
7 years, 7 months
WF Swarm - first approach
by Sebastian Laskawiec
Hey!
I've created very short demo of WF Swarm and Infinispan Embedded mode:
https://github.com/slaskawi/infinispan-wf-swarm-example/tree/master
I have a couple of thoughts after doing this demo:
- Wildfly Swarm generator [2] allows to add Infinispan integration
module. Unfortunately it doesn't provide Infinispan classes to the
classpath (one needs to add infinispan-core in provided scope for example).
I created JIRA to fix this [3].
- We should probably make JPA (Hibernate) an optional dependency.
Created JIRA for it [4].
- Injecting beans into JAX-RS classes require
using org.wildfly.swarm:jaxrs-cdi. This will be addressed at WF Swarm side
[5].
I must admit, implementing this demo was a really nice experience (there
were some problems, but with Martin's and Tomas' help I managed to work
them out (thanks a lot guys!)).
If you have some time, please experiment with the demo code and tell me
what you think. If there won't be any comments, I plan to write a small
blog entry about WF Swarm and Infinispan.
Thanks
Sebastian
[2] http://wildfly-swarm.io/generator/
[3] https://issues.jboss.org/browse/ISPN-7071
[4] https://issues.jboss.org/browse/ISPN-7072
[5] https://issues.jboss.org/browse/SWARM-714
7 years, 7 months
