[
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
