Merge lp:~stub/launchpad/memcache into lp:launchpad
| Status: | Merged |
|---|---|
| Approved by: | Stuart Bishop on 2010-03-06 |
| Approved revision: | no longer in the source branch. |
| Merged at revision: | not available |
| Proposed branch: | lp:~stub/launchpad/memcache |
| Merge into: | lp:launchpad |
| Diff against target: |
723 lines (+606/-6) 8 files modified
lib/canonical/testing/layers.py (+5/-0) lib/lp/app/stories/launchpad-root/xx-featuredprojects.txt (+5/-0) lib/lp/app/templates/root-index.pt (+10/-5) lib/lp/services/memcache/configure.zcml (+6/-0) lib/lp/services/memcache/doc/tales-cache.txt (+214/-0) lib/lp/services/memcache/interfaces.py (+1/-1) lib/lp/services/memcache/tales.py (+294/-0) lib/lp/services/memcache/tests/test_doc.py (+71/-0) |
| To merge this branch: | bzr merge lp:~stub/launchpad/memcache |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Gary Poster (community) | 2010-02-26 | Approve on 2010-02-26 | |
|
Review via email:
|
|||
Commit Message
Add syntax to page templates to cache chunks of rendered content in memcached.
| Stuart Bishop (stub) wrote : | # |
| Gary Poster (gary) wrote : | # |
merge-conditional
Hi Stuart. This is very cool! Thank you.
Might as well clean up the ``#level debug`` comments in launchpad.conf.
I question including the "anonymous" visibility. You bring up a good reason for not including it. Why don't we just exclude it this time around? It feels like something that might be interesting for upstream but not for right now--and like something that people will use unnecessarily. (On the other hand, if you want to push back, in the interest of saving you from waiting for me to wake up on the other side of the globe, you may merely imagine me acquiescing, and keep it, if you like.)
I liked your doctest.
Line 383 of the diff incorrectly describes the syntax, AIUI: <div tal:content=
I would be tempted to try to provide a more helpful error message when someone includes too many commas (re "self.visibility, max_age = (s.strip() for s in expr.split(','))" from line 392 of the diff. Line 403 ("value, unit = max_age.split(' ')") is similar. If you can't be bothered, I'll look the other way.
You say that "units is one of 'seconds', 'minutes', 'hours' or 'days'." However, you accept s*, m*, h* and d*. You are much stricter about the visibility strings. I'm inclined to favor enforcing readability, and enforce the full strings. This is negotiable (that is, under the circumstances, you may hear that as "ignorable" if you wish) but I feel more strongly about this one than some others I've brought up with similar deference.
You generate _valid_
I am curious why you are not using SPACE (32) as your delineator, as opposed to the colon, which forces your logic to have to be a bit trickier here and there. Maybe I'll see why later...
I figure you know this is OK because of the DB, but I was very mildly surprised by the confidence of "uid = str(logged_
The way you are handling repeats is very interesting. It's an interesting problem. I first thought that your approach of adding one to the counter_key in the request annotations would not work very well in the case of nested loops, because if something changed, then it would do a very odd cascade that might put an old sub-item from one top-level section into another top-level section entirely. However, if a collection changes significantly from one repeat to the next--an item is inserted somewhere, rather than appended, in particular--you are kind of hosed anyway. I guess I'm fine with what you have, though I'd like it if you added a warning in the doctest that cacheing things in a repeat is perhaps a less appealing prospect than some others.
OK, I asked why you are not using SPACE (32) as your delineator, and now I see those colons in "pt:%s:
| Stuart Bishop (stub) wrote : | # |
On Sat, Feb 27, 2010 at 5:47 AM, Gary Poster <email address hidden> wrote:
> Might as well clean up the ``#level debug`` comments in launchpad.conf.
Done.
> I question including the "anonymous" visibility. You bring up a good reason for not including it. Why don't we just exclude it this time around? It feels like something that might be interesting for upstream but not for right now--and like something that people will use unnecessarily. (On the other hand, if you want to push back, in the interest of saving you from waiting for me to wake up on the other side of the globe, you may merely imagine me acquiescing, and keep it, if you like.)
I'd like to keep it because it completes the visibility model and I'd rather not have to re implement it later if we push this upstream or if we have real use cases where we do need it. Also, my comments are just my guess - is our squid configured to cache pages with query strings? I don't really know.
> Line 383 of the diff incorrectly describes the syntax, AIUI: <div tal:content=
Fixed.
> I would be tempted to try to provide a more helpful error message when someone includes too many commas (re "self.visibility, max_age = (s.strip() for s in expr.split(','))" from line 392 of the diff. Line 403 ("value, unit = max_age.split(' ')") is similar. If you can't be bothered, I'll look the other way.
Fixed.
> You say that "units is one of 'seconds', 'minutes', 'hours' or 'days'." However, you accept s*, m*, h* and d*. You are much stricter about the visibility strings. I'm inclined to favor enforcing readability, and enforce the full strings. This is negotiable (that is, under the circumstances, you may hear that as "ignorable" if you wish) but I feel more strongly about this one than some others I've brought up with similar deference.
Ok. I did that because it made it simpler to accept plural forms. Fixed.
> You generate _valid_
Yes - that was cruft from earlier work.
> I am curious why you are not using SPACE (32) as your delineator, as opposed to the colon, which forces your logic to have to be a bit trickier here and there. Maybe I'll see why later...
Space is not a valid character in memcache keys, but colon is. I'm also using a mixture of colon and comma because I think I have seen memcache reporting tools using this to summarize memcache utilization, but it doesn't really matter provided it isn't a number or one of the magic tokens like 'p' or 'a'. These tools might be a figment of an overactive imagination.
> The way you are handling repeats is very interesting. It's an interesting problem. I first thought that your approach of adding one to the counter_key in the request annotations would not work very well in the case of nested loops, because if something changed, then it would do a very odd cascade that might put an old sub-item from one top-level section into another top-level section entirely. However, if a collection changes significantly from one repeat to the next--an item is inserted somewhere, rather than appended, in par...

Implements the ability to cache rendered chunks of our page templates in memcached.
Readable, tested documentation included describing the syntax and functionality.
To install this new functionality, I had to resort to monkey patching. The solution to this will be to move this feature upstream into zope.tal and zope.tales. We are not worrying about this yet as we are considering switching our TAL interpreter to chameleon.