Closed Bug 1056849 Opened 7 years ago Closed 6 years ago

[rbbz / push2rb] - Do not serialize rid's to strings until the very last moment when we need to print them out

Categories

(MozReview Graveyard :: General, defect, P4)

Production
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mconley, Assigned: dminor)

References

Details

(Whiteboard: [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/2760] )

Attachments

(1 file, 1 obsolete file)

The push2rb wire protocol for hg serializes the review request IDs to strings because we end up printing those IDs out to the client console.

Unfortunately, a side effect of this is that we end up storing the review request IDs as strings in the extra_data stash for review requests, which is kinda confusing. It requires us to do an extra cast to int before we can do requests on those review request IDs.

We should probably keep them as ints, and format them into a string when we need to print them out.
Product: bugzilla.mozilla.org → Developer Services
OS: Windows 7 → All
Priority: -- → P4
Hardware: x86 → All
Whiteboard: [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/2753]
Whiteboard: [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/2753] → [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/2760]
Soft blocker for proper p2rb field migration... not critical but something we might want to do at the same time.
Blocks: 1142615
Assignee: nobody → dminor
Attached file MozReview Request: bz://1056849/dminor (obsolete) —
/r/7713 - Do not serialize rid's to strings until necessary (bug 1056849); r=gps

Pull down this commit:

hg pull -r a1b6b558ad58c05419e847e89f6241c98acb895d https://reviewboard-hg.mozilla.org/version-control-tools/
Attachment #8598212 - Flags: review?(gps)
https://reviewboard.mozilla.org/r/7713/#review6641

I like this cleanup. But you'll need to handle rids stored as strings to cope with existing data in the database.

::: hgext/reviewboard/hgrb/proto.py:343
(Diff revision 1)
> -                rid = relation[1].encode('utf-8')
> +                rid = str(relation[1]).encode('utf-8')

One should never .encode a `str` type. You are supposed to either unicode.encode() or str.decode().

If `rid` is stored as an int, `str(rid)` should be all that is needed here.

::: pylib/mozreview/mozreview/pulse/__init__.py:65
(Diff revision 1)
> -        child_rrids.append(int(rrid))
> +        assert isinstance(rrid, int)

This will fail in production because existing p2rb.commits fields will have string, not int, values. We either need a one-time migration script or code to deal with "upgrading" values. The latter is preferred, as one-time migrations necessitate site downtime.

::: pylib/rbbz/rbbz/extension.py:304
(Diff revision 1)
> -                id_str = str(child.id)
> +                if child.id in unpublished_rids:

Can we trust that unpublished_rids has values of the proper type? I suspect the answer is "no" because we change the type of unpublished_rids below.
Attachment #8598212 - Flags: review?(gps)
(In reply to Gregory Szorc [:gps] from comment #3)

> This will fail in production because existing p2rb.commits fields will have
> string, not int, values. We either need a one-time migration script or code
> to deal with "upgrading" values. The latter is preferred, as one-time
> migrations necessitate site downtime.

I'm starting to think that the cure here is worse than the disease. Unless I misinterpreted you, anyone who wants to access the p2rb.commits field will have to deal with possibly getting string values. It was easy enough to change the current code, but I'm afraid when new code is written this will be easy to forget. The tests will pass, but it will fail in production. If this small cleanup isn't worth a one-time migration script, I think we might be better off closing this WONTFIX.
Flags: needinfo?(gps)
Yes, a proper fix will be more effort than you initially signed up for :/

Given that the data is already in an inconsistent state, I think the best we can do is write the data in the preferred format and continue using helper functions to fetch and normalize the data on read. Over time, more and more data is stored in the proper format. But we still have data lingering in the old format. Down the road, we can perform a one-time migration when it becomes too painful to ignore. I think this is more or less what we have now.
Flags: needinfo?(gps)
Attachment #8598212 - Flags: review?(gps)
Comment on attachment 8598212 [details]
MozReview Request: bz://1056849/dminor

/r/7713 - Do not serialize rid's to strings until necessary (bug 1056849); r=gps

Pull down this commit:

hg pull -r 436abfb31afde1009ef8bb762a615bf6fb290701 https://reviewboard-hg.mozilla.org/version-control-tools/
Comment on attachment 8598212 [details]
MozReview Request: bz://1056849/dminor

/r/7713 - Do not serialize rid's to strings until necessary (bug 1056849); r=gps

Pull down this commit:

hg pull -r d3c6c2b0fa06b0d3c2960e03f3ab304dadcf432a https://reviewboard-hg.mozilla.org/version-control-tools/
https://reviewboard.mozilla.org/r/7713/#review7791

This looks good to me. But this change still scares me. Changing the types of things is scary. I fully expect there to be bugs as a result of this. But that is the price we pay for progress.

Let's land it and be prepared to rapidly iterate on any bugs we find.

::: hgext/reviewboard/tests/test-commits-added.t:183
(Diff revision 3)
> +  $ echo 'foo4' > foo
> +  $ hg commit -m 'Bug 1 - Foo 4' 
> +  $ hg push

What value does this extra test have?
Comment on attachment 8598212 [details]
MozReview Request: bz://1056849/dminor

https://reviewboard.mozilla.org/r/7711/#review7793

Ship It!
Attachment #8598212 - Flags: review?(gps) → review+
https://reviewboard.mozilla.org/r/7713/#review8037

> What value does this extra test have?

It's a small test that we can handle rids that are still strings.
Thanks, pushed to: https://hg.mozilla.org/hgcustom/version-control-tools/rev/3c3bf3dca15a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attachment #8598212 - Attachment is obsolete: true
Attachment #8618282 - Flags: review+
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.