[
https://issues.apache.org/jira/browse/CASSANDRA-6477?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14623168#comment-14623168
]
Joshua McKenzie commented on CASSANDRA-6477:
--------------------------------------------
Thus far, the implementation looks pretty solid. Fairly self-documenting
(pending some of the naming issues we discussed above). I have more code to go
through but have some more feedback.
Questions:
# Currently we're submitting the MaterializedViewBuilder to the standard
executor in CompactionManager. Is it logical to have these 2 operations share a
thread pool and resources?
# Why the removal of updatePKIndexes from Cells.java.reconcile?
Feedback:
h6. {{General}}
* Why are concurrent_batchlog_writes and concurrent_materialized_view_writes
both hard-coded to 32 and not-in the yaml?
h6. {{AlterTableStatement}}
* mv.included.isEmpty() as a check to see if all columns are included is
counter-intuitive. Add a helper function mv.included.containsAll() (mentioned
prior, may be addressed in subsequent commits)
* announceMigration: materializedViewDrops is never initialized or used
h6. {{CreateMaterializedViewStatement}}
* in CreateMaterializedViewStatement.announceMigration, while turning
ColumnIdentifier.Raw into ColumnIdentifier, we allow <= 1 non-pk column in a MV
partition key however the error message we log on multiple attempts reads
"Cannot include non-primary key column '%s' in materialized view partition
key". We should log that <= 1 are allowed instead. We should also document why
this restriction is in-place in the code.
* refactor out duplication w/building targetPartitionKeys and
targetPartitionColumns w/nonPKTarget
h6. {{DropMaterializedViewStatement}}
* In findMaterializedView, you don't need to iterate across all the members of
cfm.getMaterializedViews().values() as it's a Map, you can just check for
whether or not it contains a member at columnFamily() index.
h6. {{SSTableIterator}}
* l252. Was this addressing a bug you uncovered during development or an
accidental change? Your update is changing what we're comparing to
indexes.size() by pre-incrementing currentIndexIdx before said comparison.
h6. {{MaterializedViewBuilder}}
* MaterializedViewBuilder.getCompactionInfo isn't giving us particularly good
information about the # Tokens built vs. total. We discussed this offline -
needs some comments in the code as to why it's currently limited in this
fashion.
h6. {{MaterializedViewUtils}}
* Should probably add a comment explaining why baseNaturalEndpoints and
viewNaturalEndpoints always have the same # of entries, so .get on baseIdx is
safe. Otherwise it takes a lot of knowledge about the MV RF implementation to
understand it (thinking about future developers here)
h6. {{AbstractReadCommandBuilder}}
* In {{.makeColumnFilter()}} - Why the change to the temporary stack ptr for
CFMetaData?
* It's not immediately clear to me why you changed from the allColumnsBuilder
to the selectionBuilder - could use some clarification on that (for me here,
not necessarily comments on code)
h6. {{ColumnFamilyStore}}
* Pull contents of initRowCache into init() -> rather than breaking into 2
separate methods, just have the 1 renamed w/MVM init in it
h6. Nits:
* SingleColumnRestriction: unnecessary whitespace changes
* AlterTableStatement:
** extraneous whitespace in announceMigration
** tab in announceMigration modified to break 120 char
* CreateMaterializedViewStatement:
** unused imports
** Double whitespace between methods
* DropMaterializedViewStatement: "Cannot drop non existing" should be "Cannot
drop non-existent"
* CompactionManager: Need space after submitMaterializedViewBuilder method close
> 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 beta 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)
