date:20170814

[jira] [Commented] (CASSANDRA-12245) initial view build can be parallel

2017-08-14 Thread Paulo Motta (JIRA)

[
https://issues.apache.org/jira/browse/CASSANDRA-12245?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16126791#comment-16126791
]

Paulo Motta commented on CASSANDRA-12245:
-

Thanks for the patch and sorry for the delay! Had an initial look at the patch,
overall looks good, I have the following comments/questions/remarks:

bq. The newly created ViewBuilderController reads the local token ranges,
splits them to satisfy a concurrency factor, and runs a ViewBuilder for each of
them.

It would be nice to maybe try to reuse the {{Splitter}} methods if possible, so
we can reuse tests, or if that's not straightforward maybe put the methods on
splitter and add some tests to make sure it's working correctly.

bq. When ViewBuilderController receives the finalization signal of the last
ViewBuilder, it double-checks if there are new local ranges that weren't
considered at the beginning of the build. If there are new ranges, new
{{ViewBuilder}}s are created for them.

This will not work if the range movement which created the new local range
finishes after the view has finished building. This problem exists currently
and is unrelated to the view build process itself, but more related to the
range movement completion which should ensure the views are properly built
before the operation finishes, so I created CASSANDRA-13762 to handle this
properly.

bq. Given that we have a ViewBuilder per local range, the key of the table
system.views_builds_in_progress is modified to include the bounds of the token
range. So, we will have an entry in the table per each ViewBuilder. The number
of covered keys per range is also recorded in the table.

Can probably remove the generation field from the builds in progress table and
[remove this
comment|https://github.com/adelapena/cassandra/blob/94f3d0d02bb5f849e4d54857d0b33531a5650643/src/java/org/apache/cassandra/db/view/ViewBuilder.java#L124]

bq. I have updated the patch to use a new separate table,
system.views_builds_in_progress_v2

{{views_builds_in_progress_v2}} sounds a bit hacky, so perhaps we should call
it {{system.view_builds_in_progress}} (remove the s) and also add a NOTICE
entry informing the previous table was replaced and data files can be removed.

bq. The downside is that pending view builds will be restarted during an
upgrade to 4.x, which seems reasonable to me.

Sounds reasonable to me too.

bq. ViewBuilder and ViewBuilderController are probably not the best names.
Maybe we could rename ViewBuilder to something like ViewBuilderTask or
ViewBuilderForRange, and rename ViewBuilderController to ViewBuilder.

{{ViewBuilder}} and {{ViewBuilderTask}} LGTM

bq. The concurrency factor is based on conf.concurrent_compactors because the
views are built on the CompactionManager, but we may be interested in a
different value.

I'm a bit concerned about starving the compaction executor for a long period
during view build of large base tables, so we should probably have another
option like {{concurret_view_builders}} with a conservative default and perhaps
control the concurrency at the {{ViewBuilderController}}. WDYT?

bq. The patch tries to evenly split the token ranges in the minimum number of
parts to satisfy the concurrency factor, and it never merges ranges. So, with
the default 256 virtual nodes (and a lesser concurrency factor) we create 256
build tasks. We might be interested in a different planning. If we want the
number of tasks to be lesser than the number of local ranges we should modify
the ViewBuilder task to be responsible for several ranges, although it will
complicate the status tracking.

I think this is good to start with, we can improve the planning later if
necessary. I don't think there is much gain from merging ranges to have smaller
tasks.

bq. Probably there is a better way of implementing
ViewBuilder.getCompactionInfo. The patch uses
keysBuilt/ColumnFamilyStore.estimatedKeysForRange to estimate the completion,
which could deal to have task completion status over 100%, depending on the
estimation.

How about using {{prevToken.size(range.right)}} (introduced by CASSANDRA-7032)?
Even though this will not be available for BytesToken (used by
ByteOrderedPartitioner, which is rarely used, so could maybe fallback to the
current imprecise calculation in that case).

Other comments:

* Avoid submitting view builder when view is already built instead of checking
on the ViewBuilder
([here|https://github.com/adelapena/cassandra/blob/94f3d0d02bb5f849e4d54857d0b33531a5650643/src/java/org/apache/cassandra/db/view/ViewBuilder.java#L109])
* ViewBuilder seems to be reimplementing some of the logic of
{{PartitionRangeReadCommand}}, so I wonder if we shoud take this chance to
simplify and use that instead of manually constructing the commands via
ReducingKeyIterator and multiple {{SinglePartitionReadCommands}}? We can
totally do this in other ticket if

55 matches

Mail list logo