[jira] [Issue Comment Deleted] (CASSANDRA-13064) Add stream type or purpose to stream plan / stream

["Paulo Motta (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/CASSANDRA-13064?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Paulo Motta updated CASSANDRA-13064:
------------------------------------
    Comment: was deleted

(was: 
Thanks for your patch!  Overall the patch and approach looks good. Since this changes the behavior of streaming for both MVs and CDC, which have different implications, I will split review into those 2 parts.

MVs
- I like the extraction of stream descriptions into an enumeration, can you just add an {{OTHER}} type for backward compatibility in case another tool or system uses another stream description to avoid having  a null type. This can be probably be used on tests instead of creating custom {{TEST_TRANSFER}} or {{TEST_LEGACY_STREAMING}}.
- Can we also perhaps make the enum name a bit more descriptive, such as {{StreamingOperation}} or {{StreamingPurpose}}?
- It would be nice if you could also add a simple unit test to make sure descriptions are being converted correctly to {{StreamType}}.
- I don't like tying logic to "toString()" since that can change, can you add a {{getDescription}} method to {{StreamType}} and use that instead?
- I think we only need to run mutations through the write path on repair, since this is done to make base table consistent with the view which is the objective of repair, but not general streaming.
- Minor typo: {{sendThroughWritePatch}} -> {{sendThroughWritePath}}

CDC
- Add unit test showing that commit log segments are properly discarded when mutations do not go to memtable.
- It seems there are no dtests to verify CDC behavior on streaming (repair, bootstrap, etc), so I'd like to see some dtests showing that CDC behavior with streaming will be maintained with this. In order not to block the fix for MVs, if you want we can split the CDC part of this into another ticket.)

> Add stream type or purpose to stream plan / stream
> --------------------------------------------------
>
>                 Key: CASSANDRA-13064
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-13064
>             Project: Cassandra
>          Issue Type: Improvement
>            Reporter: Benjamin Roth
>            Assignee: Benjamin Roth
>             Fix For: 4.0
>
>
> It would be very good to know the type or purpose of a certain stream on the receiver side. It should be both available in a stream request and a stream task.
> Why?
> It would be helpful to distinguish the purpose to allow different handling of streams and requests. Examples:
> - In stream request a global flush is done. This is not necessary for all types of streams. A repair stream(-plan) does not require a flush as this has been done shortly before in validation compaction and only the sstables that have been validated also have to be streamed.
> - In StreamReceiveTask streams for MVs go through the regular write path this is painfully slow especially on bootstrap and decomission. Both for bootstrap and decommission this is not necessary. Sstables can be directly streamed down in this case. Handling bootstrap is no problem as it relies on a local state but during decommission, the decom-state is bound to the sender and not the receiver, so the receiver has to know that it is safe to stream that sstable directly, not through the write-path. Thats why we have to know the purpose of the stream.
> I'd love to implement this on my own but I am not sure how not to break the streaming protocol for backwards compat or if it is ok to do so.
> Furthermore I'd love to get some feedback on that idea and some proposals what stream types to distinguish. I could imagine:
> - bootstrap
> - decommission
> - repair
> - replace node
> - remove node
> - range relocation
> Comments like this support my idea, knowing the purpose could avoid this.
> {quote}
>                 // TODO each call to transferRanges re-flushes, this is potentially a lot of waste
>                 streamPlan.transferRanges(newEndpoint, preferred, keyspaceName, ranges);
> {quote}
> And alternative to passing the purpose of the stream was to pass flags like:
> - requiresFlush
> - requiresWritePathForMaterializedView
> ...
> I guess passing the purpose will make the streaming protocol more robust for future changes and leaves decisions up to the receiver.
> But an additional "requiresFlush" would also avoid putting too much logic into the streaming code. The streaming code should not care about purposes, the caller or receiver should. So the decision if a stream requires as flush before stream should be up to the stream requester and the stream request receiver depending on the purpose of the stream.
> I'm excited about your feedback :)



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)