Store commit information in custom tables rather than JSON blobs

NEW
Unassigned

Status

MozReview
General
2 years ago
a year ago

People

(Reporter: smacleod, Unassigned)

Tracking

(Blocks: 2 bugs)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
Our usage of extra_data for storing all of our important commit information is extremely limiting. We should transition to using custom tables so we can ensure proper data formatting and perform efficient queries on various pieces of the data.
(Reporter)

Comment 1

2 years ago
Created attachment 8717030 [details]
MozReview Request: mozreview: Add models to hold commit information (Bug 1246691). r=mdoglio r=dminor

Throughout MozReview's history, commit information has been stored in
JSON blobs attached to specific Review Board objects. While this has
been convenient for introducing new pieces of data it has also been
extremely limiting. New models designed for storing specific information
about commits and sets of commits are introduced by this commit.

With the new data storage we'll have control over indices and should be
able to take advantage of this to query things directly and add features
that weren't possible before.

Review commit: https://reviewboard.mozilla.org/r/34019/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34019/
Attachment #8717030 - Flags: review?(mdoglio)
Attachment #8717030 - Flags: review?(dminor)
Comment on attachment 8717030 [details]
MozReview Request: mozreview: Add models to hold commit information (Bug 1246691). r=mdoglio r=dminor

https://reviewboard.mozilla.org/r/34019/#review30661

::: pylib/mozreview/mozreview/commits/models.py:139
(Diff revision 1)
> +        db_index=True,

this should be redundant (db_index is True by default on ForeignKey objects)

::: pylib/mozreview/mozreview/commits/models.py:142
(Diff revision 1)
> +    commit_id = models.CharField(

should this be unique?

::: pylib/mozreview/mozreview/commits/models.py:174
(Diff revision 1)
> +    commit = models.ForeignKey('Commit')
> +    commit_set = models.ForeignKey('CommitSet')

these 2 fields should be in a unique_together
Attachment #8717030 - Flags: review?(mdoglio) → review+
(Reporter)

Comment 3

2 years ago
https://reviewboard.mozilla.org/r/34019/#review30661

> should this be unique?

No, it's possible for two separate users to upload the same commit and get two different diffsets. I also don't want to block someone from discarding a series and starting a new one which ahs the same commit in it.

> these 2 fields should be in a unique_together

Good call :)

Updated

2 years ago
Attachment #8717030 - Flags: review?(dminor) → review+

Comment 4

2 years ago
Comment on attachment 8717030 [details]
MozReview Request: mozreview: Add models to hold commit information (Bug 1246691). r=mdoglio r=dminor

https://reviewboard.mozilla.org/r/34019/#review30717

::: pylib/mozreview/mozreview/commits/models.py:203
(Diff revision 1)
> +        max_length=64,

Why 64?

Comment 5

2 years ago
https://reviewboard.mozilla.org/r/34019/#review30769

::: pylib/mozreview/mozreview/commits/models.py:129
(Diff revision 1)
> +    replaced = models.ForeignKey(
> +        'self',
> +        blank=True,
> +        null=True,
> +        help_text=_('The Commit which was matched to this Commit and '
> +                    'replaced.'))

So this is the previous commit for this commit? What purpose does this serve?

::: pylib/mozreview/mozreview/commits/models.py:199
(Diff revision 1)
> +class Identifier(models.Model):
> +    """Information representing a given review identifier."""

I want to kill review identifiers eventually. But this probably needs to exist to port our existing system to the new schema initially.

Updated

2 years ago
Blocks: 1247477
(Assignee)

Updated

2 years ago
Product: Developer Services → MozReview
This got r+s but never landed.  What's the status here?
Flags: needinfo?(smacleod)
(Reporter)

Comment 7

a year ago
(In reply to Mark Côté [:mcote] from comment #6)
> This got r+s but never landed.  What's the status here?

The r+s were for a database schema we wanted to convert things to, but actually landing this would require converting all of our code that uses the data. In the future I'd like to revisit this, but it's not being actively worked on.
Assignee: smacleod → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(smacleod)
You need to log in before you can comment on or make changes to this bug.