[
https://issues.apache.org/jira/browse/SHINDIG-943?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12679868#action_12679868
]
Chris Chabot commented on SHINDIG-943:
--------------------------------------
I've committed the latest patch (set 2) from
http://codereview.appspot.com/25066/show
Since it's a rather large patch I'm eager to get it into svn, however there are
a few small issues, can you please submit a separate patch for those?
Since codereview tool is giving me a "500 Server Error" when i try to post it,
I'll inline my comments here:
Cache.php:
- abstract function delete($key);
+ private $storage = null;
(Draft) 2009/03/07 11:33:10 We usually put all var declarations at the top of
We usually put all var declarations at the top of the class definition and not
between functions
+ private function __construct($type, $name, $time) {
+ if ($type == 'CacheFile') {
(Draft) 2009/03/07 11:45:51 This breaks the internal contract of how the Confi
This breaks the internal contract of how the Config values == Class names, in
this case that's rather important since large sites pretty much always create /
re-use their internal custom cache classes and not our provided ones. In this
situation someone would set 'data_cache' => 'FooCache', things would break
without really having any contextual or documentation clues to *why* that
doesn't work for the cache classes. Better solution would be to edit the
default config (& add a comment explaining the change) and put the real class
name there, and instance the cache class using: $this->storage = new
$cacheClass($name); That way the config class name == class assumption is true
again, and people can use their custom classes again
+ $data = unserialize($data);
+ return array('found' => true, 'time' => $data['time'],
(Draft) 2009/03/07 11:38:57 It would be simpler to do: return array_merge(arra
It would be simpler to do: return array_merge(array('found' => true), $data);
(arg1 overwrites arg2 if they have the same keys btw, see
http://www.php.net/array_merge)
CacheStorage.php:
+abstract class CacheStorage {
(Draft) 2009/03/07 11:49:42 It would be better to force this class to be a sin
It would be better to force this class to be a singleton, especially in cases
where the cache backend makes a connection (like the frequently used memcache
based implementations), this code would cause multiple connections per request,
which slows it down and hogs resources. If you would make the constructor
private, have a private $instance var, and a static function for getInstance()
(which either returns the already instanced class or creates it if no instance
was made yet), and change the code in the Cache class to instance the storage
objects using $cacheClass::getInstance($name).
CacheStorageApc.php :
+ private function storageKey($key) {
+ return $prefix . '_' . $key;
(Draft) 2009/03/07 12:16:57 typo, should be $this->prefix
typo, should be $this->prefix
CacheStorageMemcache.php:
+ private function storageKey($key) {
+ return $prefix . '_' . $key;
(Draft) 2009/03/07 12:17:34 typo, should be $this->prefix
typo, should be $this->prefix
+ public function __construct($name) {
+ $this->prefix = $name;
+ if (!self::$memcache) {
(Draft) 2009/03/07 11:56:41 If we'd make the base CacheStorage class a singlet
If we'd make the base CacheStorage class a singleton, we wouldn't need this
hack'ish solution anymore
> add support for 0.9 limited invalidation
> ----------------------------------------
>
> Key: SHINDIG-943
> URL: https://issues.apache.org/jira/browse/SHINDIG-943
> Project: Shindig
> Issue Type: New Feature
> Components: PHP
> Reporter: Pan Jie
> Attachments: 20090225.patch, 20090226.patch
>
>
> http://wiki.opensocial.org/index.php?title=Limited_Invalidation
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
