[jira] [Commented] (CASSANDRA-3143) Global caches (key/row)

["Sylvain Lebresne (Commented) (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CASSANDRA-3143?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13135156#comment-13135156 ] 

Sylvain Lebresne commented on CASSANDRA-3143:
---------------------------------------------

I did a few pass over the patch. I haven't applied the patches yet (they need rebase anyway) and a few stuffs from the first patches is fixed in the later ones, which I realize only when reading those later patches, so even if I've tried to update my comments, there may be a few outdated ones, sorry about that.

First, some "top-level" remarks/comments:
* At least for the row cache, I fear this may sometimes be less efficient than what we have now, because some cf with less than good hit rate may evict rows of cf with very good hit rate, which wouldn't happen in the current implementation with reasonably tuned cache sizes. Aren't we screwing people that are doing fine tuning in the name of simplicity?
* Preceding point apart, we would at least need a way to deactivate row caching on a per-cf basis. We may also want that for key cache, though this seems less critical. My initial idea would be to either have a boolean flag if we only want to allow disabling row cache, or some multi-value caches option that could be "none", "key_only", "row_only" or "all".
* We probably need to have a story as far as upgrading to this patch is concerned (cleaning old saved caches, migrating to the new global options, ...). It's probably fine just leaving instructions in the NEWS file as a start, but I'd rather do it with the patch to avoid forgetting.
* I think it'd be better to size the key cache by it's size in bytes rather than by number of entries, like for the row cache, since 'size alloted to the cache' is really the only measure that make sense for a user.  

Then a bit more specifically on some patches:

patch 2:
* For the Row cache key, we should really use the cfId instead of table and cfname.
* I think it would be worth factoring what can be of readSaved. In particular, we could create a KeyCacheKey (like RowCacheKey and that would really just be the usual pair of (Descriptor, DecoratedKey), have it share an interface with RowCacheKey (serialize and deserialize basically) and use that.
* Putting the clone of the key into the constructor of RowCacheKey is inefficient, we don't care about cloning when we invalidate or query the cache (nor when we deserialize RowCacheKeys).
* nit: not fond of declaration like public abstract Set<?> readSaved(), making it harder to know what the method returns just to save a few characters.

patch 3:
* In DatabaseDescriptor:
  {noformat}
    if (CacheService.instance == null)
        logger.error("Could not initialize Cache Service.");
  {noformat}
  I believe the CacheService.instance will load the CacheService that will trigger an exception if there is a problem, not set the instance to null. So in particular, this message will never be written. That code is moved by the 4th patch but the problem remains I think (note that we do want to make sure CacheService get loaded quickly to throw an exception if we have an
  initialization problem, it's just not the right way I believe).
* I'm not super fond of that key cache preloading. If the key cache save is outdated, we'll have a bunch of uselessly preloaded stuff (not a huge deal but...). Maybe we could keep doing as before and just save the set of keys for each column family instead. That would needs buffering of the keys during the cache save though, but not sure it's such a huge deal and it would reduce the size of the saved cache quite a bit.
* In CacheService, setRowCacheSavePeriodInSecond directly schedule the saving to the new value, but the get method grabs the value from DatabaseDescriptor, so it will always return the initially set value, which is not what we want. I think we should keep the Integer that were previously in CFS (but I'm fine moving them to CacheService).
* Why does the getRowCacheKeysToSave() option disappeared?

patch 4:
* The patch does the following change:
  {noformat}
  -            int newCapacity = (int) (DatabaseDescriptor.getReduceCacheCapacityTo() * size());
  +            int newCapacity = (int) (DatabaseDescriptor.getReduceCacheCapacityTo() * getCapacity());
  {noformat}
  but that's not the semantic of the operation. The initial was the right one.
* I really think that making DecoratedKey equals method deal with RowCacheKey is asking for trouble. Not sure why it would be useful either?

                
> Global caches (key/row)
> -----------------------
>
>                 Key: CASSANDRA-3143
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-3143
>             Project: Cassandra
>          Issue Type: Improvement
>            Reporter: Pavel Yaskevich
>            Assignee: Pavel Yaskevich
>            Priority: Minor
>              Labels: Core
>             Fix For: 1.1
>
>         Attachments: 0001-global-key-cache.patch, 0002-global-row-cache-and-ASC.readSaved-changed-to-abstra.patch, 0003-CacheServiceMBean-and-correct-key-cache-loading.patch, 0004-key-row-cache-tests-and-tweaks.patch, 0005-cleanup-of-the-CFMetaData-and-thrift-avro-CfDef-and-.patch
>
>
> Caches are difficult to configure well as ColumnFamilies are added, similar to how memtables were difficult pre-CASSANDRA-2006.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira