Closed
Bug 1246691
Opened 9 years ago
Closed 6 years ago
Store commit information in custom tables rather than JSON blobs
Categories
(MozReview Graveyard :: General, defect)
MozReview Graveyard
General
Tracking
(Not tracked)
RESOLVED
INVALID
People
(Reporter: smacleod, Unassigned)
References
Details
Attachments
(1 file)
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•9 years ago
|
||
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 2•9 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/#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•9 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•9 years ago
|
Attachment #8717030 -
Flags: review?(dminor) → review+
Comment 4•9 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•9 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.
Assignee | ||
Updated•9 years ago
|
Product: Developer Services → MozReview
Comment 6•8 years ago
|
||
This got r+s but never landed. What's the status here?
Flags: needinfo?(smacleod)
Reporter | ||
Comment 7•8 years 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)
Comment 8•6 years ago
|
||
MozReview is now obsolete. Please use Phabricator instead. Closing this bug.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•