Review Board and Mercurial pretxnchangegroup hook

[George Miroshnykov]

I've encountered an interesting problem when integrating a Mercurial repository into Review Board via pretxnchangegroup hook.

The initial idea should have worked like this:

Client does a special "hg push dance" to review repository.

This push triggers a hook that creates review requests inside Review Board.

If review request creation fails for some reason, client can simply try that hg push dance again until it succeeds.

In order to satisfy the last requirement (retry the push to retry review request creation), I initially planned to use Mercurial's pretxnchangegroup hook.

That hook runs inside an unconfirmed isolated transaction that is not yet a part of main history.

If the hook returns a non-zero exit code, transaction is rolled back and repository remains unchanged, allowing a client to simply push again.

So far so good, but it turns out that in order to create a review request, Review Board needs to inspect a changeset inside Mercurial repository.

As that changeset is inside the isolated transaction, it's not visible to Review Board, so review request can not be created.

This is kind of chicken-and-egg problem: we need to confirm a transaction in order to create review request, but we can't confirm the transaction until review request is successfully created.

As a temporary workaround, I've switched from pretxnchangegroup hook to changegroup hook.

The downside is that if there was an error creating review request, client can't simply push again and expect that missing review requests will be created automatically.

We'd have to rewrite history inside changegroup hook to allow that and it could create more problems than it'd solve.

Actually, right now we have to use both pretxnchangegroup and changegroup hooks, because we have to validate the bug number and auth credentials in pretxnchangegroup hook, then actually try to create review requests in changegroup hook (and that can fail still).

I was looking for some low-level CLI commands for dealing with Mercurial transactions, but couldn't find anything online.

Eventually I figured out that Mercurial uses a special HG_PENDING env var to access changesets inside a pending transaction.

So we could patch Review Board to use this env var when inspecting the repository, but there may be some side effects that I haven't considered.

Any advice?

Thanks,

George

[Ehsan Akhgari]

Ah, this is unfortunate. :(

Here's a question: what kinds of RB side failures are we trying to protect against?  IOW, what are the reasons why creating a review request might fail?

--
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-re...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[George Miroshnykov]

The fails I saw were mostly during hook development / debugging, but I can imagine lots of stuff can go wrong.

You could try to push into the bug that someone else has pushed before - AFAIR this will currently result a permission error.

Sometimes RB would randomly return exceptions and just re-running the query fixes it, but that may be just my development setup.
My point is that the push triggers at least three distinct systems that should work together: the hook itself, RB + Mercurial repo and Bugzilla.

If at least one of those components will be unavailable during the push, we won’t be able to gracefully recover from this situation.

Cheers,

George

[Gregory Szorc]

You don't want to even attempt to access contents from a pending
transaction outside of a hook. That's playing with fire. I'm pretty sure
Mercurial won't expose the in-transaction data to external/new
"connections" to the repo until it is committed anyway.

To achieve robustness, you will want the client to communicate the
to-be-reviewed changeset as part of the push metadata. This can be
facilitated via an extension.

I would implement things as such:

1) Client performs push as expected
2) Client sends a "pushkey" command to the server to initiate the
review. The pushkey command will contain all necessary metadata
(changeset to review, bug number, auth, etc).

We'll be using a mach command to initiate reviews. We should not have
qualms about silently activating a custom extension on the client. `hg
--config extensions.foo=/path/to/foo.py` can do this without hgrc
manipulation. There is nothing to fear.

[Ehsan Akhgari]

On Tue, Apr 8, 2014 at 2:05 PM, George Miroshnykov <gmiros...@rebbix.com> wrote:

The fails I saw were mostly during hook development / debugging, but I can imagine lots of stuff can go wrong.

You could try to push into the bug that someone else has pushed before - AFAIR this will currently result a permission error.

That's something that we can change during pretxn phase right?

 

Sometimes RB would randomly return exceptions and just re-running the query fixes it, but that may be just my development setup.

That seems very bad!  We should try to debug and fix these kinds of issues, since they will be frustrating bugs for our users no matter how we deal with the failures.

 

My point is that the push triggers at least three distinct systems that should work together: the hook itself, RB + Mercurial repo and Bugzilla.

If at least one of those components will be unavailable during the push, we won’t be able to gracefully recover from this situation.

Ideally, failures will be handled by rejecting the push, which means that we should keep the hook in pretxn as much as possible.  I wonder how other similar systems deal with this.  Have you looked at what gerrit or critic do here for example?

(Note that I think as long as we keep the failure situations restricted to edge cases such as the Bugzilla server being down etc, it's not too bad if they're not handled very gracefully.)

[Ehsan Akhgari]

How does that fix the failure scenario though?  It seems like if the pushkey command fails, we'll be in no better spot than where we currently are.

[George Miroshnykov]

Pushkey is actually a neat idea, but it turns out we won’t have to do it - Steven just told me how to do everything in pretxnchangegroup using RB’s parent diffs.

Sorry for that lengthy story you had to read :)

Thanks,

George

[Ehsan Akhgari]

So is this a non-issue then?

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

[George Miroshnykov]

Yeah, at this point I know how to create RB review requests and roll back a pending Mercurial transaction if something fails.

[Ehsan Akhgari]

\o/

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