cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joshua McKenzie (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CASSANDRA-6477) Materialized Views (was: Global Indexes)
Date Tue, 28 Jul 2015 17:42:09 GMT

    [ 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)

Mime
View raw message