cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sylvain Lebresne (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CASSANDRA-3143) Global caches (key/row)
Date Thu, 22 Dec 2011 16:37:31 GMT

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

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

We're getting there :)

A few more comments however:
* When saving the cache, the 'writeKeyQuietly' make it possible to write a corrupted file
(maybe only part of a key has been written) and has the risk of flooding the log (if one key
throw an IOException, chances are the next one on this file will too). I think we'd have less
problem just stop all saving if an IOError occurs.
* In CFS.initRowCache(), the test {{  if (cachedRowsRead++ > rowCache.getCapacity()) }}
is now not correct since getCapacity is a memory size. For now, I'm fine removing the test
and say that 'if you reduce the size, you may not get you hottest keys loaded on startup'.
Though ultimately we'll probably need to fix that.

On what we expose through MBean:
* I think I was preferring the old way of using the InstrumentingCacheMBean, rather than to
have lots of {get,set}RowCache, {get,set}KeyCache method that forward to the InstrumentingCache
ones. Basically I think it's more clear to have Cache->RowCache->infos and Cache->KeyCache->infos,
rather than CacheServices->allInfos. It's also more easily extensible if we ever add some
new cache.
* In any case, we don't register the InstrumentingCacheMBean anymore and the CacheService
one doesn't expose the hit rate nor the number of requests processed (and we can remove InstrumentingCacheMBean
if we're not going to use it).
* Both set{Key,Row}CacheSavePeriodInSeconds and saveCaches disregard the {Key,Row}CacheKeysToSave
setting, while they probably shouldn't.
* I'd rename getRowCacheCapacity() to getRwoCacheCapacityInMB() to match the set method.

And a bunch of very minor nitpicks that I just happened to gather during the review:
* In AutoSavingCache.saveCache, I'd log the "Deleting old files" message at DEBUG.
* In DatabaseDescriptor, there's a wrongly placed import
* In CFS, we should probably remove the getKeyCache method, to emphasis it's now a global
thing. Same in DataTracker.
* The comment from {{ data.addSSTables(sstables); // this will call updateCacheSizes() for
us }} in CFS.loadNewSSTables is outdated
* DK.java uselessly import RowCacheKey (and only have a very gratuitous codeStyle change btw).
* I would rename the CacheService MBean name to more simply "org.apache.cassandra.db:type=Caches".

I'll also note for posterity that by removing the DK from the cache keys, we're trading off
memory for cpu (since we have to redecorate for each use).  Don't get me wrong, it's likely
a good trade-off, just wanted to write it down.

                
> 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: CASSANDRA-3143-squashed.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

        

Mime
View raw message