[jira] [Commented] (CASSANDRA-15362) Add a resumable commit log reader for CDC consumption

["Josh McKenzie (Jira)" <ji...@apache.org>]

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

Josh McKenzie commented on CASSANDRA-15362:
-------------------------------------------

My brain's been chewing on something related to this in the background; I think I'd like to at least prototype an approach that doesn't cache any state or break out a new Resumable reader API and instead always assumes RAR recreation and scan before read with a signature such as
{noformat}
/**
 * @return offset of last successfully read SyncSegment for subsequent resuming
 */
long CommitLogReader.startReadAtSyncSegmentOffset(long startOffset, [other args])
{noformat}
This would require significantly less logic than the current patch and simplify {{SegmentIterator.computeNext}} as it'd only have to cache the last read good SyncSegment start and then return that on read completion as per the signature above. Would also get rid of the "have to allocate a Resumable reader object" on every read, etc.

Just read the {{CommitLogDescriptor}} for any read (just like now) and then {{RAR.seek}} on a resumed read w/a provided offset and you're off to the races.

Not going to have time to dig into that soon but felt it worth commenting on (mostly to get it out of my head). It wasn't until near the end of working on the patch that I realized we had the constraint of the {{CompressedChunkReader.maxCompressedLength}} inside the RAR's Rebufferer that would constrain us from re-using that Segmenter on resuming, which lead to the {{ResumableCommitLogReader.rebufferData}} and .readToExhaustion sentinel in the compressed case. Definitely felt that just re-initializing the RAR was better than poking any holes in or further extending the Rebufferer hierarchy, but that decision of course means that in the compressed case, the current approach basically consists functionally of the above _far_ simpler approach + some ease of use or friendliness on the API, and I'm not convinced it's a big enough UX delta to justify the iterator complexity.

> Add a resumable commit log reader for CDC consumption
> -----------------------------------------------------
>
>                 Key: CASSANDRA-15362
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-15362
>             Project: Cassandra
>          Issue Type: New Feature
>          Components: Feature/Change Data Capture
>            Reporter: Josh McKenzie
>            Assignee: Josh McKenzie
>            Priority: Normal
>
> [Link to wip branch|https://github.com/apache/cassandra/compare/trunk...josh-mckenzie:resumable_clreader]
> This ticket builds upon and supersedes CASSANDRA-15196.
> One of the initial shortcomings of CDC after its initial implementation is that, upon update of the related CDC index file if parsing an active segment, a user needs to re-scan files (specifically compressed or encrypted) as they're written and discard duplicate mutations. The primary push of this patch is to introduce a new class, the [ResumableCommitLogReader|https://github.com/josh-mckenzie/cassandra/blob/resumable_clreader/src/java/org/apache/cassandra/db/commitlog/ResumableCommitLogReader.java#L35-L53] that greatly simplifies and optimizes repeated reads from a commitlog file being actively written underneath you.
> The primary API consists of:
> {noformat}
> public void readPartial(int readLimit) throws IOException;
> public void readToCompletion() throws IOException
> public void close(); // is AutoCloseable
> {noformat}
> An example of the usage of this can be seen in the changed standard read implementation within the [CommitLogReader|https://github.com/josh-mckenzie/cassandra/blob/resumable_clreader/src/java/org/apache/cassandra/db/commitlog/CommitLogReader.java#L129-L134]
>  The simple "read straight through" case:
> {noformat}
> try(ResumableCommitLogReader resumableReader = new ResumableCommitLogReader(file, handler, minPosition, mutationLimit, tolerateTruncation))
> {
>     resumableReader.readToCompletion();
> }
> {noformat}
> The more complex "resume reading" case:
> {noformat}
> try(ResumableCommitLogReader resumableReader = new ResumableCommitLogReader(file, handler, minPosition, mutationLimit, tolerateTruncation))
> {
>     while(!parseCDCCompletionStatus(cdcIndexFile))
>     {
>         Thread.sleep(10000);
>         rr.readPartial(parseCDCOffset(cdcIndexFile));
>     }
> }
> {noformat}
> A brief enumeration of some of the most obvious pros and cons of this current design and implementation:
> Pros:
>  * Very simple external user interface
>  * All the complexity of resuming, RAR seeking, RAR re-creation hidden from end-users
>  * Minimal risk to existing CL reading code (isPartial() path very separate from default in computeNext())
> Cons:
>  * Creation of a resumable reader object required on each new segment file read (trivially fixable if a concern)
>  * Added complexity burden on computeNext of snapshotting Segmenter state and reverting: [reference|https://github.com/josh-mckenzie/cassandra/blob/resumable_clreader/src/java/org/apache/cassandra/db/commitlog/CommitLogSegmentReader.java#L92]
>  * Added complexity burden on computeNext of Sentinel state and resumability of iterator
>  * Generally breaking the "one way street" paradigm of iterators (biggest concern to me atm)
>  * Philosophical "add complexity to C* code-base to ease 'external tool' users" debate
> One other meta trade-off: this design puts the ResumableCommitLogReader at the center of the Commit log reading design; most other related classes now take that as an argument and it serves as both our parent logic coordinator + data holder. The obvious benefit of this design is it's a lot easier to pass things around and you have to grok really one thing to know what data and context you have to work with. The obvious downside is risk of calcification and lack of modularity plus the need to construct on any read. It's trivial to unwind that and keep CommitLogReader et al totally unaware of the resumable reader if we're so inclined.
> Anyway - figured I'd toss this ticket up here so it's visible to people that some of this is in flight. I've spoken with several engineers from different companies that are using CDC at scale and have backported things to 3.0 or 3.11 (CASSANDRA-12148 for instance), so having this code here visible may be of use to someone even in it's not quite yet polished or tested state.
> There are some obvious counter-designs (add a skipToNextSyncMarker call to Segmenter for example, or break out of iterator paradigm entirely and have an API for {{computeNextSegment}} or {{revertToLastKnownSegment}} and just walk across SyncSegments internal to a non-iterator based class), etc. I prefer not to rewrite things _too_ much when working on them (and we already have some complexity in {{SegmentReader.computeNext}} coming into this), and I'm getting more partial to the "keep things as stupidly simple for external users as possible" approach to APIs as I age so that's why my initial cut at this takes on the burden of this complexity internally to keep the consumption braindead easy.
> Currently passes CommitLogReader + new ResumableCommitLogReader tests; haven't run it through the entire gamut yet. Figured it'd be worth kicking it out here to see if anyone has feedback that significantly alters the design before going into the full gauntlet of tests.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org