[
https://issues.apache.org/jira/browse/CASSANDRA-10155?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14739696#comment-14739696
]
Robert Stupp commented on CASSANDRA-10155:
------------------------------------------
>From a very quick view the patches look very good. Have some (mostly minor)
>comments, though:
* {{DatabaseDescriptor#getSerializedCachePath}} can you replace the
StringBuilder with a simple string concatenation? Feels more readable than a
StringBuilder.
* {{AutoSavingCache.Writer.saveCache()}} can you just remove the {{os}}
variable and just initialie {{writer}} using {{writer = new
DataOutputStreamPlus(streamFactory.getOutputStream(tempCacheFile));}}?
* {{AutoSavingCache.loadSavedImpl()}} can be moved to loadSaved(). Both methods
try-catch and call JVMStabilityInspector (in the catch, of course).
* {{AutoSavingCache.loadSavedAsync(final String name)}}: can’t we just derive
the name from the {{cacheType}} instance field?
* {{AutoSavingCache.loadSavedImpl()}} please add a comment describing where
{{String ksname = in.readUTF();}} (and cfname) are serialized.
* {{CassandraDaemon}}: please remove the new, unused imports.
* {{CassandraDaemon}}: change the error/warn message {{"Error loading row and
key cache asynchronously”}} to {{“Error loading key or row cache”}} (so leaving
out asynchronously, as it isn’t from a user’s perspective). Similar for
{{StorageService.initServer()}} where it loads the counter cache.
* {{CacheService.*CacheSerializer.serializer()}} please add a comment
describing why {{out.write(cfs.metadata.ksAndCFBytes);}} are not deserialised
in the corresponding {{deserialize}} but where these are deserialized
(AutoSavingCache).
* {{CacheKey}}: just thinking whether it’s better to replace
{{Pair<String,String> ksAndCFName}} with a {{byte[] ksAndCFName}} as it is
cheaper to serialize and deserialize. OTOH you need some object to pass to
{{getColumnFamilyStoreIncludingIndexes()}}. Or put both {{byte[]}} and {{Pair}}
fields into {{CacheKey}} so that serialization can just use the {{byte[]}} but
we still have the {{Pair}} available. WDYT? (Latter applies to 2.2/3.0)
* {{CacheKey}}: BTW: good catch making this an abstract class. Can you make the
derived classes {{final}}?
* Field name {{ksAndCFName}} doesn’t make really clear that it can also contain
the name of the secondary index. {{ksAndCfAndMaybe2iName}} doesn’t feel better
though - don’t even know whether we have a known term for this at all.
* Regarding your comment in {{Schema.getColumnFamilyStoreIncludingIndexes()}}
(3.0) _”Shouldn't ask for a backing table if there is none so just throw?”_ -
that’s actually a good question. How’s that handled in 2.1/2.2? I think there
should be a unit test that tests exactly this behaviour - a C* 2i on one column
and a custom, table-less index on another column. Only the 2i should get
entries in the key-cache at all - non-table backed indexes must not.
Will continue with some code digging.
> 2i key cache load fails
> -----------------------
>
> Key: CASSANDRA-10155
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10155
> Project: Cassandra
> Issue Type: Bug
> Reporter: Robert Stupp
> Assignee: Ariel Weisberg
> Fix For: 3.0.0 rc1, 2.1.10, 2.2.2
>
>
> CASSANDRA-9265 changed how key cache content is serialized to disk. It uses
> {{UUID cfId}} to generate the file path for each {{ColumnFamilyStore}}.
> Since {{cfId}} of a secondary index is the same as for the base table, the
> key-cache files for 2i's and the base are the same. This will/may lead to
> deserialization failures on restart for tables with at least one 2i.
> /cc [~aweisberg] [~danchia]
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
