Summary:

Reduce diff storage by hashing diff uploads

Review Request #2618 — Created Sept. 25, 2011 and submitted Nov. 6, 2011, 4:06 a.m.

Information
Owner:	ddruska
Repository:	Review Board
Branch:
Bugs:
Depends On:
Reviewers
Groups:	reviewboard
People:

Description

Reduce diff storage by hashing diff uploads

Diff files that already exist in the database will no longer be double-stored. Diffs are now hashed on upload and correspond to a new hash->binary table.
Existing table data is not hashed and will remain for backwards compatibility. Evolutions have been made to create the new table and rename existing fields, so that model-logic can override fields. Test data has also been modified for new field name compatibility.

Testing Done

Ran fill-database and all data was hashed accordingly. Manually posted reviews and posted parent diffs. Created a unit test to check if hashes match.

Issues

Description	From	Last Updated
These are both built-in Python modules, so no blank line.	chipx86	Sept. 29, 2011, 9:25 a.m.
I'm not really sure "Binary" is quite the term we want. FileDiffData might be better. Then "hash" and "data" fields.	chipx86	Sept. 29, 2011, 9:25 a.m.
Docstrings are in the format of: """One-line summary Option multi-line description. """ It should always be a proper sentence, punctuation …	chipx86	Sept. 29, 2011, 9:25 a.m.
Alignment problem. There's no need for \, since the parens take care of it.	chipx86	Sept. 29, 2011, 9:25 a.m.
Same here.	chipx86	Sept. 29, 2011, 9:25 a.m.
Blank line between these. A comment describing the next block of code should be separated. Also, comments should have proper …	chipx86	Sept. 29, 2011, 9:25 a.m.
This can be: self.diff_hash, is_new = \ FileDiffBinary.objects.get_or_create(binary_hash=hashkey, defaults={ 'binary': diff })	chipx86	Sept. 29, 2011, 9:25 a.m.
Same logic can be applied here for the get_or_create.	chipx86	Sept. 29, 2011, 9:25 a.m.
I?think that there should a corresponding py file for that?	ZH zhenli	Oct. 2, 2011, 4:58 a.m.
For consistencies sake, can we call that "kwargs"?	mike_conley	Oct. 12, 2011, 1:27 p.m.
Space after the if block	mike_conley	Oct. 12, 2011, 1:27 p.m.
Managers go in managers.py.	chipx86	Oct. 17, 2011, 9:33 a.m.
It'd be nice to pull defaults out, so you don't repeat yourself (and also, don't assume defaults is set): defaults …	chipx86	Oct. 17, 2011, 9:33 a.m.
Space before \	chipx86	Oct. 17, 2011, 9:34 a.m.
Put the comment inside the else:	chipx86	Oct. 17, 2011, 9:34 a.m.
Two blank lines here	david	Oct. 30, 2011, 7:54 a.m.
Imports should be alphabetized.	david	Oct. 30, 2011, 7:54 a.m.

Testing Done:

	+	Ran existing unit tests. These only test the backwards compatibility though, and don't test parent diffs.
	+	Ran fill-database and all data was hashed accordingly. Manually posted reviews and posted parent diffs. All tests passed.

Wow, you got that working fast! I have some comments. Mostly style issues, a couple better ways to use Django in some cases, and a name change. I'm really happy to see this though :)

reviewboard/diffviewer/models.py (Diff revision 1)

Show all issues

These are both built-in Python modules, so no blank line.

reviewboard/diffviewer/models.py (Diff revision 1)

Show all issues

I'm not really sure "Binary" is quite the term we want.

FileDiffData might be better. Then "hash" and "data" fields.

reviewboard/diffviewer/models.py (Diff revision 1)

Show all issues

Docstrings are in the format of:

"""One-line summary

Option multi-line description.
"""

It should always be a proper sentence, punctuation and all.

david Sept. 25, 2011, 9:02 a.m.

I think these days, for multi-line ones, the first line can safely go on the line after """. Single-line docstrings should have """ and the text all on the same line, though.

reviewboard/diffviewer/models.py (Diff revision 1)

Show all issues

Alignment problem.

There's no need for \, since the parens take care of it.

reviewboard/diffviewer/models.py (Diff revision 1)
Show all issues
```
Same here.
```

reviewboard/diffviewer/models.py (Diff revision 1)

Show all issues

Blank line between these. A comment describing the next block of code should be separated.

Also, comments should have proper punctuation (period at the end).

reviewboard/diffviewer/models.py (Diff revision 1)

Show all issues

This can be:

self.diff_hash, is_new = \
    FileDiffBinary.objects.get_or_create(binary_hash=hashkey,
                                         defaults={
                                             'binary': diff
                                         })

reviewboard/diffviewer/models.py (Diff revision 1)

Show all issues

Same logic can be applied here for the get_or_create.

reviewboard/diffviewer/evolutions/__init__.py (Diff revision 1)

I think it might be better to call it “add_diff_hash".

reviewboard/reviews/fixtures/test_reviewrequests.json (Diff revision 1)

Looks like, after the first line, the who block is identical, but still marked by ReviewBoard as different. Is it a bug?

chipx86 Sept. 28, 2011, 10:28 a.m.
```
Nope, it's just one really long line.
```

Change Summary:

Fixed issues pointed out by Christian.

Diff:

Revision 2 (+145 -90)

Show changes

	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/reviews/fixtures/test_reviewrequests.json

reviewboard/diffviewer/evolutions/__init__.py (Diff revision 2)

Show all issues

I?think that there should a corresponding py file for that?

ddruska Oct. 2, 2011, 4:59 a.m.

Good catch, Jacob. I forgot to add the file to my commit.

Change Summary:

Forgot to add the evolution file add_diff_hash.py

Diff:

Revision 3 (+154 -90)

Show changes

	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_diff_hash.py
	reviewboard/reviews/fixtures/test_reviewrequests.json

Change Summary:

Removed the AutoKey from FileDiffData and added a custom manager for the Base64Field AutoKey workaround

Diff:

Revision 4 (+361 -134)

Show changes

	docs/releasenotes/rbtools/0.3.4.txt
	docs/releasenotes/rbtools/index.txt
	reviewboard/manage.py
	reviewboard/admin/tests.py
	reviewboard/cmdline/rbsite.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/htdocs/media/rb/css/common.css
	11 more

Change Summary:

Removed the AutoKey from FileDiffData and added a custom manager for the Base64Field AutoKey workaround
(Disregard revision 4)

Diff:

Revision 5 (+168 -91)

Show changes

	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_diff_hash.py
	reviewboard/reviews/fixtures/test_reviewrequests.json

David:

This looks really good!  Just the two tiny nits I found below.  Once those are fixed, I'll happily give this my ship-it.

Thanks,

-Mike

reviewboard/diffviewer/models.py (Diff revision 5)
Show all issues
```
For consistencies sake, can we call that "kwargs"?
```
reviewboard/diffviewer/models.py (Diff revision 5)
Show all issues
```
Space after the if block
```
1. mike_conley Oct. 8, 2011, 1:38 p.m.
  And by "space", I of course mean "new line".

Change Summary:

Renamed kwds to kwargs, added spacing and formatting

Diff:

Revision 6 (+171 -91)

Show changes

	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_diff_hash.py
	reviewboard/reviews/fixtures/test_reviewrequests.json

Ship it!

```
This looks good to me!
```

Couple small things.

Do unit tests still run?

reviewboard/diffviewer/models.py (Diff revision 6)
Show all issues
```
Managers go in managers.py.
```

reviewboard/diffviewer/models.py (Diff revision 6)

Show all issues

It'd be nice to pull defaults out, so you don't repeat yourself (and also, don't assume defaults is set):

defaults = kwargs.get('defaults', {})

if defaults and defaults['binary']:
    defaults['binary'] = ...

reviewboard/diffviewer/models.py (Diff revision 6)
Show all issues
```
Space before \
```
reviewboard/diffviewer/models.py (Diff revision 6)
Show all issues
```
Put the comment inside the else:
```

Change Summary:

Moved FileDiffDataManager to managers.py, added a unit test for hashes, cleaned up tests.py with pep8.

Diff:

Revision 7 (+201 -95)

Show changes

	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_diff_hash.py
	reviewboard/reviews/fixtures/test_reviewrequests.json

Testing Done:

~		Ran existing unit tests. These only test the backwards compatibility though, and don't test parent diffs.
	~	Ran fill-database and all data was hashed accordingly. Manually posted reviews and posted parent diffs. Created a unit test to check if hashes match.
-		Ran fill-database and all data was hashed accordingly. Manually posted reviews and posted parent diffs. All tests passed.

Fix it!

```
Two trivial changes left:
```
reviewboard/diffviewer/managers.py (Diff revision 7)
Show all issues
```
Two blank lines here
```
reviewboard/diffviewer/models.py (Diff revision 7)
Show all issues
```
Imports should be alphabetized.
```

Diff:

Revision 8 (+202 -95)

Show changes

	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_diff_hash.py
	reviewboard/reviews/fixtures/test_reviewrequests.json

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Committed to master (8bfea51)