Deployment update

[Mark Côté]

So we had partial success setting up a review hg repo on reviewboard-dev
with George's hook. There were a few miscommunicated assumptions, namely
that rbbz (the Review Board extension) requires exactly one bug ID and
reviewer in order to publish a review request, and the hook wasn't
setting the bug ID on the dependent reviews nor was it setting a
reviewer at all. The first should be easy to fix in the hook, and for
the latter I'm working on supporting both zero and multiple reviewers
when a review is published (this also involves making sure it updates
existing Review-Board-url attachment flags properly).

We hacked it a bit just to try to get something working, and this is the
result (click on the link in the "Bugs" field to see it on bugzilla-dev):

https://reviewboard-dev.allizom.org/r/23/

Pretty good, however, as you can see from the diff, there are some
unknown files. smacleod tells me that these are added files and that hg
diffs are kind of broken in this regard. They've worked around the
problem in the rbt command-line tool. Unfortunately, the current hg hook
is written in JS. smacleod has offered to rewrite it in Python so we can
leverage the rbt features. I'm not sure if there's an easier solution,
but I'd like to investigate a bit more tomorrow.

We ran into some Bugzilla-database-locking errors as well, but as far as
I know those won't be an issue in production (I'm going to follow up on
them with the BMO team tomorrow).

Mark

[George Miroshnykov]

Hi Mark,

Great news!

Let’s look into what I can fix in the hook.

AFAIR the hook sets “bugs_closed” field on parent review request, but not on child review requests.

Do we need it set on child review requests as well?

Regarding reviewer, I thought that this may be out of scope of the hook, as the hook is not in position to figure out the appropriate reviewer(s) during the push.

If you have any suggestions on how we can figure it out (some heuristics?), I can surely implement that in the hook.

Also, if Steven could describe what needs to be implemented in the hook to work around that diff issue, I can probably implement it.

But I don’t mind if the hook is going to be rewritten in Python as well :)

Thanks,

George

--
You received this message because you are subscribed to the Google Groups "mozilla-code-review" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mozilla-code-re...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Ehsan Akhgari]

On Tue, Jun 3, 2014 at 3:21 AM, George Miroshnykov <gmiros...@rebbix.com> wrote:

Hi Mark,

Great news!

Let’s look into what I can fix in the hook.

AFAIR the hook sets “bugs_closed” field on parent review request, but not on child review requests.

Do we need it set on child review requests as well?

That would mean that the child review requests won't be associated with the bug as far as RB is concerned, right?  That seems bad!

 

Regarding reviewer, I thought that this may be out of scope of the hook, as the hook is not in position to figure out the appropriate reviewer(s) during the push.

If you have any suggestions on how we can figure it out (some heuristics?), I can surely implement that in the hook.

Yeah, I was also under the impression that we would set the reviewer inside the RB UI as well.

 

Also, if Steven could describe what needs to be implemented in the hook to work around that diff issue, I can probably implement it.

But I don’t mind if the hook is going to be rewritten in Python as well :)

Thanks George!  I'm also interested to know more about the nature of this issue.  Can someone please explain what's happening with the diff?

Thanks!
Ehsan

[Mark Côté]

On 2014-06-03, 10:36 AM, Ehsan Akhgari wrote:

On Tue, Jun 3, 2014 at 3:21 AM, George Miroshnykov <gmiros...@rebbix.com> wrote:

Hi Mark,

Great news!

Let’s look into what I can fix in the hook.

AFAIR the hook sets “bugs_closed” field on parent review request, but not on child review requests.

Do we need it set on child review requests as well?

That would mean that the child review requests won't be associated with the bug as far as RB is concerned, right?  That seems bad!

Yeah I agree, all child requests should be associated with the bug as well. It's more convenient for the user and easier for the plugin, since we want all the review-request URLs to show up in the bug, and if the bug ID is in the review request, I don't have to trace up to the parent.

 

Regarding reviewer, I thought that this may be out of scope of the hook, as the hook is not in position to figure out the appropriate reviewer(s) during the push.

If you have any suggestions on how we can figure it out (some heuristics?), I can surely implement that in the hook.

Yeah, I was also under the impression that we would set the reviewer inside the RB UI as well.

Sure, we can do that. I know bzexport allows you to specify the reviewer, but the UI is probably a better place once we implement suggested reviewers in Review Board.

I think the thing that surprised me is that the hg hook actually publishes the review. Should we not leave it in draft mode until the author has added reviewers?

 

Also, if Steven could describe what needs to be implemented in the hook to work around that diff issue, I can probably implement it.

But I don’t mind if the hook is going to be rewritten in Python as well :)

Thanks George!  I'm also interested to know more about the nature of this issue.  Can someone please explain what's happening with the diff?

smacleod, could you explain?

Mark

[Gregory Szorc]

On 6/2/14, 6:56 PM, Mark Côté wrote:
> So we had partial success setting up a review hg repo on reviewboard-dev
> with George's hook. There were a few miscommunicated assumptions, namely
> that rbbz (the Review Board extension) requires exactly one bug ID and
> reviewer in order to publish a review request, and the hook wasn't
> setting the bug ID on the dependent reviews nor was it setting a
> reviewer at all. The first should be easy to fix in the hook, and for
> the latter I'm working on supporting both zero and multiple reviewers
> when a review is published (this also involves making sure it updates
> existing Review-Board-url attachment flags properly).
>
> We hacked it a bit just to try to get something working, and this is the
> result (click on the link in the "Bugs" field to see it on bugzilla-dev):
>
> https://reviewboard-dev.allizom.org/r/23/
>
> Pretty good, however, as you can see from the diff, there are some
> unknown files. smacleod tells me that these are added files and that hg
> diffs are kind of broken in this regard. They've worked around the
> problem in the rbt command-line tool. Unfortunately, the current hg hook
> is written in JS. smacleod has offered to rewrite it in Python so we can
> leverage the rbt features. I'm not sure if there's an easier solution,
> but I'd like to investigate a bit more tomorrow.

I'm responsible for a lot of the rbt patches to make Mercurial work
better. I can vouch for the correctness of the rbt implementation. There
is still some performance tweaking to be done, but it should be
technically correct.

Also, the hook should almost certainly be implemented in Python so we
can reuse code and gain access to Mercurial internals, if necessary.
This is non-trivial code and I fear the redundancy in JS is increasing
our surface area exposure to bugs. If nothing else, the Python should
perform better courtesy of executing in the same process.

I will be more than happy to review the hook when it is implemented.

[Mark Côté]

So I'm a little concerned that it may take smacleod a little while to
get to this, as he is swamped with other work. Does anyone else have
time to work on this? I'm finishing up a few small changes to how we
mirror r+s back to Bugzilla.

Mark

[Gregory Szorc]

I have a high-priority project (Firefox update hotfix) that I need to
work on. If you find someone to do the work, I can find time to do a
brain dump, if needed.

[George Miroshnykov]

I can fix up the hook, as soon as I have some details on what should be fixed in diff generation.

AFAIR the diffs are generated with ‘--git’ flag right now, what else should be done?

Generally I can commit to maintaining the hook in JS for as long as it’s needed, just throw bugs at me :)

Performance won’t be a problem as the hook is IO-bound, not CPU-bound.

But I can also rewrite the hook in Python to improve our "bus factor", though that will have to wait as I’m tied up in other RelEng projects.

Regards,

George

[Ehsan Akhgari]

George, do you happen to have some spare cycles?

Thanks!

--
Ehsan
<http://ehsanakhgari.org/>

--
You received this message because you are subscribed to the Google Groups "mozilla-code-review" group.

To unsubscribe from this group and stop receiving emails from it, send an email to mozilla-code-review+unsub...@googlegroups.com.

[Steven MacLeod]

I can fix up the hook, as soon as I have some details on what should be fixed in diff generation.

AFAIR the diffs are generated with ‘--git’ flag right now, what else should be done?

The diffs you're sending to Review Board should not be generated with '--git'. That could be the problem but I haven't had a chance to actually look into it.

 

[Gregory Szorc]

On 6/4/14, 12:12 PM, George Miroshnykov wrote:
> I can fix up the hook, as soon as I have some details on what should be
> fixed in diff generation.
> AFAIR the diffs are generated with ‘--git’ flag right now, what else
> should be done?
>
> Generally I can commit to maintaining the hook in JS for as long as it’s
> needed, just throw bugs at me :)
> Performance won’t be a problem as the hook is IO-bound, not CPU-bound.
>
> But I can also rewrite the hook in Python to improve our "bus factor",
> though that will have to wait as I’m tied up in other RelEng projects.
>
> Regards,
> George
>
> On June 4, 2014 at 21:59:18 , Mark Côté (mc...@mozilla.com

FWIW, having the hook invoke `hg` again will add a considerable amount
of latency, especially for large repos, as the new Mercurial process
will need to re-load all the context about the repository. This context
is likely already available (and cached) in the process that handled the
push. This is the main reason why in-process hooks are so much better.
You can gain some of this performance back by using a command server
process (see `hg help serve`) to have a persistent process talking to
the repo.

Also, Python process start-up time kind of sucks. See [1] and [2]. My
measurements tell me node's start-up time is in the same ballpark's as
Python. This information is worth knowing for anyone implementing
services that spawn tens, hundreds, or thousands of processes. But your
big performance concern here is overhead of new `hg` processes. You
should make every attempt to minimize `hg` process creation. I would do
that by writing an-process Python hook. If you are set on Node, have
Node talk to a hg command server.

[1] https://mail.python.org/pipermail/python-dev/2014-May/134528.html
[2] http://www.selenic.com/pipermail/mercurial-devel/2014-May/058854.html

[Ehsan Akhgari]

I'm not sure if we've seen and measured any perf problems with the current hook yet... I have nothing against rewriting the hook in python if we need to, but for now I'm much more interested in fixing the diff issue and get this initial deployment working.  Let's focus on that as the #1 priority for now.

Cheers,
Ehsan

[Mark Côté]

Agreed. We are extremely, extremely close to having something workable. Let's get it out the door! :)

Mark

[Mark Côté]

So it appears we are still stuck on a few things related to the review repo. I believe we absolutely have to sort them out before getting anyone to use this, but please correct me if I'm wrong.

I filed a tracker bug at https://bugzilla.mozilla.org/show_bug.cgi?id=1021929 that blocks the remaining issues that I have identified. Let's sort them out as quickly as possible.

Mark

[George Miroshnykov]

Hey everyone,

I think I’ve managed to fix all cases of diff generation bug.

There’s also no need for this patch:

https://github.com/mozilla/reviewboard/commit/668126c18fdd4c08c2aa02772db2b03e02391a30

So I’ve reverted it.

The cause was Mercurial client misconfiguration (it was generating diffs with --git flag) and incorrect parent diff generation.

Both were my faults, Review Board performed fine, though better error reporting would be very helpful :)

We can deploy and test the new version of the hook.

Thanks,

George

[Gregory Szorc]

The RB 2.0.2 release notes mention a suspiciously similar bug. I can't
help but feel there's still a component in RB we need to address.

http://www.reviewboard.org/docs/releasenotes/reviewboard/2.0.2/

On 6/13/14, 4:18 AM, George Miroshnykov wrote:
> Hey everyone,
>
> I think I’ve managed to fix all cases of diff generation bug.
>
> There’s also no need for this patch:
> https://github.com/mozilla/reviewboard/commit/668126c18fdd4c08c2aa02772db2b03e02391a30
> So I’ve reverted it.
>
> The cause was Mercurial client misconfiguration (it was generating diffs
> with --git flag) and incorrect parent diff generation.
> Both were my faults, Review Board performed fine, though better error
> reporting would be very helpful :)
>
> We can deploy and test the new version of the hook.
>
> Thanks,
> George
>

> On June 6, 2014 at 11:52:56 PM, Mark Côté (mc...@mozilla.com

>>> <mailto:mozilla-code-re...@googlegroups.com>.

>>> For more options, visit https://groups.google.com/d/optout.
>>
>> --
>> You received this message because you are subscribed to the Google
>> Groups "mozilla-code-review" group.
>> To unsubscribe from this group and stop receiving emails from it, send
>> an email to mozilla-code-re...@googlegroups.com

>> <mailto:mozilla-code-re...@googlegroups.com>.

>> For more options, visit https://groups.google.com/d/optout.
>
> --
> You received this message because you are subscribed to the Google
> Groups "mozilla-code-review" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to mozilla-code-re...@googlegroups.com

> <mailto:mozilla-code-re...@googlegroups.com>.

[Mark Côté]

I tested this out on Friday and Monday. It appears to be working but is somewhat brittle. When I did a push of couple mq commits, it overwhelmed the stdout buffer, which had to be increased to 20 MB. It was doing a diff from a commit from several days ago; not sure why it chose that particular commit, but I worry that perhaps 20 MB won't always be enough.

I also got some 500 ISE errors when the hook was trying to publish to Review Board; I discarded all my old reviews and tried again, and then it worked. This is also worth investigation.

smacleod has gotten authorization from his manager to spend a week tying up these loose ends. He's going to start by rewriting the hook in Python, which is something we want to do at some point, after which he'll investigate the above issues to make sure we're solid and that we understand everything that's going on.

Mark

[Gregory Szorc]

Steven asked for help with the Mercurial integration. So, I pieced
together a pair of extensions that transfer the review metadata to the
server.

https://hg.mozilla.org/hgcustom/version-control-tools/rev/ea875443cfd6

You specify the tip revision of the review and it uses Mercurial's phase
information (which should be reliably pulled from central, fx-team, etc)
to determine the base changeset. It also extracts the review identifier
from the commit messages. Later, I may have it use the bookmark or mq
patch name. (It was unclear what the requirement for identifiers is.)

Sample usage:

$ hg commit -m 'Bug 123 - second commit'
$ hg push http://localhost:$HGPORT
pushing to http://localhost:$HGPORT/
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files
Attempting to create a code review...
Identified 1 changesets for review
Review identifier: 123

Your review request can be seen at https://...

(Steven needs to finish the server-side bit.)

I also have a feature request: I'd like the server to transfer down the
mapping of changeset to RB review # back to Mercurial. That way, we can
store this information in the local Mercurial repo. If obsolescence is
enabled (locally, not necessarily on the server), we can carry over the
review ID to the successor changeset and seamlessly replace the correct
review. If we don't do this, we risk patch reordering, etc making things
all whacky. This is not a launch requirement.

>> On June 6, 2014 at 11:52:56 PM, Mark Côté (mc...@mozilla.com

>>>> <mailto:mozilla-code-re...@googlegroups.com>.

>>>> For more options, visit https://groups.google.com/d/optout.
>>>
>>> --
>>> You received this message because you are subscribed to the Google
>>> Groups "mozilla-code-review" group.
>>> To unsubscribe from this group and stop receiving emails from it,
>>> send an email to mozilla-code-re...@googlegroups.com

>>> <mailto:mozilla-code-re...@googlegroups.com>.

>>> For more options, visit https://groups.google.com/d/optout.
>
> --
> You received this message because you are subscribed to the Google
> Groups "mozilla-code-review" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to mozilla-code-re...@googlegroups.com

> <mailto:mozilla-code-re...@googlegroups.com>.

[Gregory Szorc]

I hacked a bit more on this today.

The Mercurial extension now stores the review IDs returned from the
server in the Mercurial repository. When you submit a review for a
changeset, the client will look in the local store to see if a review
has already been associated with the changeset. If it has, the client
sends it to the server so the server can reuse the review.

That's pretty basic stuff and there's little value add beside making the
server work less.

The real win comes from hooking up obsolescence markers (the feature
powering changeset evolution). If you rewrite history (amend, rebase,
etc) and the SHA-1 of a changeset changes, Mercurial records that Y is a
successor to X. At review time, the review extension will ask Mercurial,
"what are the previous versions of Y?" When Mercurial returns X, we see
we already have a review for that changeset and we recycle the review
ID. It's pretty magical.

A win here is that clients can start leveraging obsolescence and
changeset evolution before the server supports obsolescence marker
exchange - something that probably won't scale up to Mozilla's needs for
the review repo for a while.

Another win is that it makes the impact of history rewriting on ongoing
review requests pretty bullet proof. For example, say your history looks
like:

* A Parent of review base
* B Pre work foo
* C Pre work bar
* D Foo bar

You push these to the server and get review IDs:

* B -> 1
* C -> 2
* D -> 3

Say two different people are reviewing patches B and C and that you get
review on C first and want to check it in. Or, say that it makes sense
for C to come before B for whatever reason. You reorder patches and are
left with:

* A Parent of review base
* C' Pre work bar (2)
* B' Pre work foo (1)
* D' Foo bar (3)

If you push C', B', and D' for review, a naive implementation (let's
call it the "Git" implementation) may assume that the first pushed
commit (C') is a newer version of B because B was the first commit last
time. Sure, you can attempt some heuristic matching, but that's an
inexact science with lots of cracks. Things start to get *really*
difficult when you fold or drop commits. Anyway, with obsolescence
markers, Mercurial knows that C' is a successor of C and that C was
review 2. This gets passed to the server and the proper reviews are
updated accordingly.

The magic here is beautiful. I can't wait to see this running outside of
my test environment :D

[Gregory Szorc]

One of my accomplishments yesterday was moving "push2rb" (the generic
code for turning a series of commits into RB reviews into the
version-control-tools repo. The part that took the most work (and still
isn't bullet proof) is integrating a local Review Board server into the
tests for the Mercurial hooks and push2rb.

As of [1], the tests will spin up a local Review Board server (using a
temporary/new sqlite db) and pushes from Mercurial will hit that
temporary server. This means we have end-to-end and close-to-real-life
testing. Before, we were mocking everywhere and failing to identify bugs.

Mercurial's "t tests" [2] mix shell commands with expected output [2].
So, if people complain about bugs, we just need them to submit steps to
reproduce and they can be converted to tests pretty easily.

This should all drastically speed up the development cycle and give us
confidence when rolling out updates.

FWIW, we can use Mercurial's "t tests" for Git as well. I'm not sure
where the Git hook and related code lives. We should consider bringing
it into the version-control-tools repo so it can share the same testing
infrastructure.

[1] https://hg.mozilla.org/hgcustom/version-control-tools/rev/27a1f27cb0ac
[2]
http://mercurial.selenic.com/wiki/WritingTests#Writing_a_shell_script_test
[3]
https://hg.mozilla.org/hgcustom/version-control-tools/file/27a1f27cb0ac/hgext/reviewboard/tests/test-obsolescence.t