[jira] [Comment Edited] (CASSANDRA-11218) Prioritize Secondary Index rebuild

["Jeff Jirsa (JIRA)" <ji...@apache.org>]

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

Jeff Jirsa edited comment on CASSANDRA-11218 at 11/3/16 5:31 AM:
-----------------------------------------------------------------

Thanks much for the review [~beobal] 

Pushed https://github.com/jeffjirsa/cassandra/commit/0bb72b06ef14c4ab3e0b1748e9cd66e500adc0b5 to address your comments (same branch https://github.com/jeffjirsa/cassandra/commits/cassandra-11218-3.X ) . 

CI @ http://cassci.datastax.com/job/jeffjirsa-cassandra-11218-3.X-dtest/ and http://cassci.datastax.com/job/jeffjirsa-cassandra-11218-3.X-testall/ (started evening of Nov 2, should be ready by Nov 3, depending on when you come back to this).  

Some notes:

{quote}
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.
{quote}

Seems like a good idea to me - implemented.

{quote}
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?
{quote}

Index summary redistribution runs on its own {{org.apache.cassandra.concurrent.DebuggableScheduledThreadPoolExecutor}}, so the priority is ignored. Added notes to reflect that. 

{quote}
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.
{quote}

Now set to max value with a note that it's on its own executor. 

{quote}
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.
{quote}

Now runs with {{Cleanup}} priority (lowest). We don't really have an OperationType for {{CacheCleanupExecutor}}, we could add one to be explicit - perhaps this should really be same as key/row/counter cache save? I don't have a strong opinion on what it SHOULD be. 

{quote}
Some comments...
{quote}

... added. 


Also - I started with the idea that we could do much, much better with prioritizing compaction by getting very clever with subtype prioritization (compaction within the "normal" compaction priority). Thinking about starvation problems, and the 'right' way to generalize across compaction strategies makes me wonder if this is a good idea or not - in this patch, I've removed the remaining uses of subtype prioritization (where it was previously sorting tasks by bytes on disk), in favor of the current behavior (within a given type, tasks are ordered by timestamp). I've left the logic in place for later - if we eventually decide there's a "right" way to prioritize tasks within a type, we can add it back easily later. Some options that may be worth considering there are either hotness or a per-CF flag to allow user to prioritize certain CFs over others. In any case, I don't think it needs to be perfect - we have a huge improvement for operations by prioritizing 2i build and deprioritizing cleanup/scrub, so I think trying to get to perfect can wait.


was (Author: jjirsa):
Thanks much for the review [~beobal] 

Pushed https://github.com/jeffjirsa/cassandra/commit/96288f42f36ad80bff186707f0f7234dd0a9a8d9 to address your comments (same branch https://github.com/jeffjirsa/cassandra/commits/cassandra-11218-3.X ) . 

CI @ http://cassci.datastax.com/job/jeffjirsa-cassandra-11218-3.X-dtest/ and http://cassci.datastax.com/job/jeffjirsa-cassandra-11218-3.X-testall/ (started evening of Nov 2, should be ready by Nov 3, depending on when you come back to this).  

Some notes:

{quote}
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.
{quote}

Seems like a good idea to me - implemented.

{quote}
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?
{quote}

Index summary redistribution runs on its own {{org.apache.cassandra.concurrent.DebuggableScheduledThreadPoolExecutor}}, so the priority is ignored. Added notes to reflect that. 

{quote}
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.
{quote}

Now set to max value with a note that it's on its own executor. 

{quote}
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.
{quote}

Now runs with {{Cleanup}} priority (lowest). We don't really have an OperationType for {{CacheCleanupExecutor}}, we could add one to be explicit - perhaps this should really be same as key/row/counter cache save? I don't have a strong opinion on what it SHOULD be. 

{quote}
Some comments...
{quote}

... added. 


Also - I started with the idea that we could do much, much better with prioritizing compaction by getting very clever with subtype prioritization (compaction within the "normal" compaction priority). Thinking about starvation problems, and the 'right' way to generalize across compaction strategies makes me wonder if this is a good idea or not - in this patch, I've removed the remaining uses of subtype prioritization (where it was previously sorting tasks by bytes on disk), in favor of the current behavior (within a given type, tasks are ordered by timestamp). I've left the logic in place for later - if we eventually decide there's a "right" way to prioritize tasks within a type, we can add it back easily later. Some options that may be worth considering there are either hotness or a per-CF flag to allow user to prioritize certain CFs over others. In any case, I don't think it needs to be perfect - we have a huge improvement for operations by prioritizing 2i build and deprioritizing cleanup/scrub, so I think trying to get to perfect can wait.

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