1246691 - Store commit information in custom tables rather than JSON blobs

Status: RESOLVED INVALID

(MozReview Graveyard :: General, defect)

Description

[Steven MacLeod [:smacleod]]

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.

Comment 1

[Steven MacLeod [:smacleod]]

Attachment: 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/

Comment 2

[Mauro Doglio [:mdoglio]]

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

Comment 3

[Steven MacLeod [:smacleod]]

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 :)

Comment 4

[Dan Minor [: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/#review30717

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

Why 64?

Comment 5

[Gregory Szorc [:gps]]

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.

Comment 6

[Mark Côté [:mcote]]

This got r+s but never landed.  What's the status here?

Comment 7

[Steven MacLeod [:smacleod]]

(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.

Comment 8

[BMO Automation]

MozReview is now obsolete. Please use Phabricator instead. Closing this bug.