[
https://issues.apache.org/jira/browse/CASSANDRA-11218?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15615708#comment-15615708
]
Sam Tunnicliffe commented on CASSANDRA-11218:
---------------------------------------------
[~jjirsa], sorry it's taken so long to pick up this review, I got stuck on some
unrelated stuff until just now.
The concept and general approach are straightforward & seem reasonable enough.
I had a bunch of remarks about the implementation, so I thought it would be
simpler to just push a branch
([here|https://github.com/beobal/cassandra/tree/CASSANDRA-11218) with
suggestions rather than trying to spell them out here. Let me know what you
think, the summary is:
* Refactored {{OperationType}} a bit. Aside from removing duplication, the
version in my branch normalizes the property names so that the suffix is always
the lowercase version of the type name (there are some inconsistencies in the
original version - e.g. {{KEY_CACHE_SAVE:keycachesave}} vs
{{USER_DEFINED_COMPACTION:user_defined_compaction}} vs
{{GARBAGE_COLLECT:garbage_collection}}).
* Simplified {{CompactionManager.CompactionPriorityComparator}}.
* I feel like the way priorities are carried between the various
{{IPrioritizedX}} classes is a bit messy. For example,
{{CompactionManager::submitUserDefined}} creates an anonymous instance of
{{PriotitizedCompactionWrappedRunnable}} that has a timestamp set in it's
constructor. That's then submitted to the compaction executor, where
{{newTaskFor}} wraps it in an {{IPrioritizedCompactionFutureTask}} using the
runnable's type and subtype priority, but taking a new timestamp, dropping the
one on the runnable. Ultimately, I ended up encapsulating the 3 priority values
and just using that {{Priorities}} class everywhere (I also renamed
{{IPrioritizedCompactionComparable}} to {{Prioritized}}).
** Was there any special reasoning behind making the priority fields in
{{PrioritizedCompactionFutureTask}}/{{PrioritizedCompactionCallable}}/{{PrioritizedCompactionWrappedRunnable}}
atomics rather than just final? I couldn't see how they would ever be mutated,
did I overlook something?
One thought that I didn't have chance to explore in that branch:
* The {{instanceof}} checks and special casing to handle regular vs prioritized
runnable is also duplicated between the {{newTaskFor}} methods and the
comparator. Could we make {{CompactionExecutor::newTaskFor}} always return an
{{IPrioritizedCompactionFutureTask}}, so that when the supplied
runnable/callable is already prioritized it just uses its existing values, and
when a non-prioritized runnable/callable is received it makes it prioritized
with the default 0/0/0 values (which is functionally equivalent to what the
comparator will do anyway). Then the comparator can be further simplified by
not having to consider non-prioritized tasks.
And a few more minor points:
* On JIRA you suggested prioritizing anticompaction & index summary
redistribution joint highest, but in the patch the latter has max priority (and
is not overridable). Is that intentional?
* Validation tasks are run a dedicated executor, which doesn't use a priority
queue. This, and the fact that validation is orthogonal to compaction strategy
means that all validation tasks are equal priority & also don't block the other
tasks. So it makes the reasoning for explicity setting validation priority to
256 a little unclear.
* On a related note, {{CacheCleanupExecutor}} is also dedicated to a single
task type. This *does* end up using a priority queue, but only ever has vanilla
runnables submitted to it.
Finally:
Some comments in {{IPrioritizedCompactionComparable/Prioritized}} describing
its purpose & maybe the comparison algorithm (e.g. what's described in the
comments here, above) could be useful.
Also some comments in {{OperationType}} on the fact that priority relates to
task scheduling in compaction manager, and why some types have un-overridable
priority of {{MAX_VALUE}} might be handy.
> Prioritize Secondary Index rebuild
> ----------------------------------
>
> Key: CASSANDRA-11218
> URL: https://issues.apache.org/jira/browse/CASSANDRA-11218
> Project: Cassandra
> Issue Type: Improvement
> Components: Compaction
> Reporter: sankalp kohli
> Assignee: Jeff Jirsa
> Priority: Minor
>
> We have seen that secondary index rebuild get stuck behind other compaction
> during a bootstrap and other operations. This causes things to not finish. We
> should prioritize index rebuild via a separate thread pool or using a
> priority queue.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
