Fix and improve caching of diff fragments.

Review Request #7118 — Created March 25, 2015 and submitted

Information

Review Board
release-2.0.x
10da309...

Reviewers

Diff fragments were cached server-side, using memcached, but we had no
caching client-side, due to the lack of any caching headers. It turns
out that we could somewhat frequently invalidate these caches as well,
without meaning to.

The cache keys and the URLs were making use of the AJAX_SERIAL value to
help with invalidation when things change. However, this value wasn't
stable. It originally was computed from the last modified timestamp of
the bundled templates, but in order to invalidate those when extensions
changed, we incorporated an extension sync value with this. That sync
value would be different every time a thread/process starts, or when
extension configuration changed, or when the matching sync key fell out
of cache. When these things happened, diff caches would invalidate.

We now have a new variable, settings.TEMPLATE_SERIAL, which represents
the actual modified timestamp of the templates. All the diff caches have
been updated to factor this in instead (since extensions don't modify
these templates).

Along with the new cache key values, caching headers were added for diff
fragment responses. The end result is that you can load a series of
diffs, restart the web server, load the diffs again, and all requests
will consistently come back as Not Modified.

Loaded the diff viewer, and saw it fetch each request. Inspected the
requests and saw ETags assigned.

Reloaded and saw 302 Not Modified responses. Verified through code that
the ETags requested and generated matched.

Restarted the dev server and reloaded the diff viewer. Again, 302 Not Modified.

Modified a template and restarted the dev server. Saw it re-fetched the diffs.

Unit tests pass.

Description From Last Updated

'reviewboard' imported but unused

reviewbotreviewbot

'datetime' imported but unused

reviewbotreviewbot

'utc' imported but unused

reviewbotreviewbot

'set_last_modified' imported but unused

reviewbotreviewbot

'os' imported but unused

reviewbotreviewbot

redefinition of unused 'FileDiffMigrationTests' from line 1041

reviewbotreviewbot

'reviewboard' imported but unused

reviewbotreviewbot

While I don't every expect the cache key to contain non-ascii characters, this should call .encode('utf-8') just to make sure …

daviddavid

redefinition of unused 'FileDiffMigrationTests' from line 1039

reviewbotreviewbot
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/renderers.py
        reviewboard/test.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/tests.py
        reviewboard/__init__.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/tests/diffFragmentQueueViewTests.js
        reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js
        reviewboard/templates/base.html
        reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/renderers.py
        reviewboard/test.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/tests.py
        reviewboard/__init__.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/tests/diffFragmentQueueViewTests.js
        reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js
        reviewboard/templates/base.html
        reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
  2. reviewboard/__init__.py (Diff revision 1)
     
     
    Show all issues
     'reviewboard' imported but unused
    
  3. reviewboard/diffviewer/renderers.py (Diff revision 1)
     
     
    Show all issues
     'datetime' imported but unused
    
  4. reviewboard/diffviewer/renderers.py (Diff revision 1)
     
     
    Show all issues
     'utc' imported but unused
    
  5. reviewboard/diffviewer/renderers.py (Diff revision 1)
     
     
    Show all issues
     'set_last_modified' imported but unused
    
  6. reviewboard/diffviewer/tests.py (Diff revision 1)
     
     
    Show all issues
     'os' imported but unused
    
  7. reviewboard/diffviewer/tests.py (Diff revision 1)
     
     
    Show all issues
     redefinition of unused 'FileDiffMigrationTests' from line 1041
    
  8. 
      
chipx86
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/renderers.py
        reviewboard/test.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/tests.py
        reviewboard/__init__.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/tests/diffFragmentQueueViewTests.js
        reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js
        reviewboard/templates/base.html
        reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/renderers.py
        reviewboard/test.py
        reviewboard/diffviewer/views.py
        reviewboard/diffviewer/tests.py
        reviewboard/__init__.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/tests/diffFragmentQueueViewTests.js
        reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js
        reviewboard/templates/base.html
        reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
  2. reviewboard/__init__.py (Diff revision 2)
     
     
    Show all issues
     'reviewboard' imported but unused
    
  3. reviewboard/diffviewer/tests.py (Diff revision 2)
     
     
    Show all issues
     redefinition of unused 'FileDiffMigrationTests' from line 1039
    
  4. 
      
david
  1. 
      
  2. reviewboard/diffviewer/renderers.py (Diff revision 2)
     
     
    Show all issues

    While I don't every expect the cache key to contain non-ascii characters, this should call .encode('utf-8') just to make sure that hashlib doesn't ValueError.

  3. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (a0c57d0)
Loading...