[
https://issues.apache.org/jira/browse/CASSANDRA-6477?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14644731#comment-14644731
]
Joshua McKenzie commented on CASSANDRA-6477:
--------------------------------------------
946e221 -> 61aad0e:
* The removal of the RowUpdateBuilder usage makes the code both more verbose
and less readable. Net negative - I'd prefer we make RowUpdateBuilder adhere to
some performance guarantees so we can continue using that abstraction to clean
up these implementation details. That being said, not worth blocking this
ticket for that effort, so maybe a different ticket would be better.
* Clarify flow of ref acquisition and release in StreamReceiveTask, even if
it's just to comment when it's grabbed vs. when it's released. Right now it
looks very odd that we're calling reader.selfRef().release() in the
hasMaterializedViews path and we're not doing that in the else path. That
implies to me that we're taking a ref (or not removing a ref) that we aren't in
the else clause but the code doesn't read as such to me. The ISSTableScanner
doesn't seem to grab a ref in the BigTableScanner impl (and should release it
in .close() if it does since it's AutoClosable), so it looks like we just have
an extra selfRef().release() in the hasMaterializedViews path that's
unnecessary. I could be missing something, but I expected to see something like
the following for write-path repair on a mutation that impacts a CF w/MV's:
{code}
//We have a special path for Materialized view.
//Since the MV requires cleaning up any pre-existing state, we must put
//All partitions through the same write path as normal mutations.
if (hasMaterializedViews)
{
for (SSTableReader reader : readers)
{
try (ISSTableScanner scanner = reader.getScanner())
{
while (scanner.hasNext())
{
try (UnfilteredRowIterator rowIterator = scanner.next())
{
new Mutation(PartitionUpdate.fromIterator(rowIterator)).apply();
}
}
}
}
}
task.txn.finish();
task.sstables.clear();
try (Refs<SSTableReader> refs = Refs.ref(readers))
{
// add sstables and build secondary indexes
cfs.addSSTables(readers);
cfs.indexManager.maybeBuildSecondaryIndexes(readers,
cfs.indexManager.allIndexesNames());
}
{code}
* To follow on the above, in StreamReceiveTask.run, we don't add sstables or
cfs.indexManager.maybeBuildSecondaryIndexes on the hasMaterializedViews path.
Is that an oversight or am I just missing something fundamental here?
* Nit: MaterializedViewTest, line 631 - dead code left in
* Nit: Some unused imports
Do we have / could we run a code-coverage report for the new MV code that's
added? I'm specifically interested in MaterializedView* and the contents of
TemporalRow, especially given the amount of changes you two had to put in post
CASSANDRA-9705.
> Materialized Views (was: Global Indexes)
> ----------------------------------------
>
> Key: CASSANDRA-6477
> URL: https://issues.apache.org/jira/browse/CASSANDRA-6477
> Project: Cassandra
> Issue Type: New Feature
> Components: API, Core
> Reporter: Jonathan Ellis
> Assignee: Carl Yeksigian
> Labels: cql
> Fix For: 3.0 alpha 1
>
> Attachments: test-view-data.sh, users.yaml
>
>
> Local indexes are suitable for low-cardinality data, where spreading the
> index across the cluster is a Good Thing. However, for high-cardinality
> data, local indexes require querying most nodes in the cluster even if only a
> handful of rows is returned.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
