[
https://issues.apache.org/jira/browse/CASSANDRA-11475?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15264206#comment-15264206
]
Carl Yeksigian commented on CASSANDRA-11475:
--------------------------------------------
{quote}
You're completely right and I don't any obvious way to avoid this since we rely
on all updates to be on the same batchlog mutation. I guess my point was that
since we can already have a high footprint, all the more reason to lower it as
much as we can.
{quote}
Agreed; definitely a huge improvement to reduce our footprint by 2/3. We should
open a ticket to track this; maybe [~iamaleksey] has some idea how we might be
able to do this with some batchlog manipulations like writing multiple entries
but only setting them as writable when all of the entries are written.
{quote}
Is that a naming problem? I don't give that much importance to "patterns" so
having it not build a ViewUpdate doesn't bother me (it still "builds" updates
to views). But if you think that's a bit confusing, I don't mind renaming to
ViewMutationGenerator or something.
{quote}
I think that the {{Builder}}s seem like things you would be using if you were
interacting with C* from a unit test, or otherwise programmatically building
these; not necessarily internal usages. I think {{ViewMutationGenerator}} is
more explanatory; {{generateViewMutations}} and {{build}} don't really describe
to me how the API is supposed to be used. I would have expected
{{generateViewMutations}} to return the {{PartitionUpdate}}s; I guess
{{addRow}} and {{generateMutations}} would be more descriptive to me.
{quote}
I'm not sure to follow what you mean by that.
{quote}
Yeah, I was just saying that it would make more sense to be passing in 2
{{Partition}}s, one for the old and one for the merged, but it was impractical.
It ends up being a moot point since it is about the API change that was
necessary to prevent holding the partitions in memory.
> MV code refactor
> ----------------
>
> Key: CASSANDRA-11475
> URL: https://issues.apache.org/jira/browse/CASSANDRA-11475
> Project: Cassandra
> Issue Type: Bug
> Reporter: Sylvain Lebresne
> Assignee: Sylvain Lebresne
> Fix For: 3.0.x, 3.x
>
>
> While working on CASSANDRA-5546 I run into a problem with TTLs on MVs, which
> looking more closely is a bug of the MV code. But one thing leading to
> another I reviewed a good portion of the MV code and found the following
> correction problems:
> * If a base row is TTLed then even if an update remove that TTL the view
> entry remained TTLed and expires, leading to an inconsistency.
> * Due to calling the wrong ctor for {{LivenessInfo}}, when a TTL was set on
> the base table, the view entry was living twice as long as the TTL. Again
> leading to a temporary inconsistency.
> * When reading existing data to compute view updates, all deletion
> informations are completely ignored (the code uses a {{PartitionIterator}}
> instead of an {{UnfilteredPartitionIterator}}). This is a serious issue since
> it means some deletions could be totally ignored as far as views are
> concerned especially when messages are delivered to a replica out of order.
> I'll note that while the 2 previous points are relatively easy to fix, I
> didn't find an easy and clean way to fix this one on the current code.
> Further, I think the MV code in general has inefficiencies/code complexities
> that should be avoidable:
> * {{TemporalRow.Set}} is buffering both everything read and a pretty much
> complete copy of the updates. That's a potentially high memory requirement.
> We shouldn't have to copy the updates and we shouldn't buffer all reads but
> rather work incrementally.
> * {{TemporalRow}}/{{TemporalRow.Set}}/{{TemporalCell}} classes are somewhat
> re-inventing the wheel. They are really just storing both an update we're
> doing and the corresponding existing data, but we already have
> {{Row}}/{{Partition}}/{{Cell}} for that. In practice, those {{Temporal*}}
> class generates a lot of allocations that we could avoid.
> * The code from CASSANDRA-10060 to avoid multiple reads of the base table
> with multiple views doesn't work when the update has partition/range
> tombstones because the code uses {{TemporalRow.Set.setTombstonedExisting()}}
> to trigger reuse, but the {{TemporalRow.Set.withNewViewPrimaryKey()}} method
> is used between view and it does not preseve the {{hasTombstonedExisting}}
> flag. But that oversight, which is trivial to fix, is kind of a good thing
> since if you fix it, you're left with a correction problem.
> The read done when there is a partition deletion depends on the view itself
> (if there is clustering filters in particular) and so reusing that read for
> other views is wrong. Which makes that whole reuse code really dodgy imo: the
> read for existing data is in {{View.java}}, suggesting that it depends on the
> view (which again, it does at least for partition deletion), but it shouldn't
> if we're going to reuse the result across multiple views.
> * Even ignoring the previous point, we still potentially read the base table
> twice if the update mix both row updates and partition/range deletions,
> potentially re-reading the same values.
> * It's probably more minor but the reading code is using {{QueryPager}},
> which is probably an artifact of the initial version of the code being
> pre-8099, but it's not necessary anymore (the reads are local and locally
> we're completely iterator based), adding, especially when we do page. I'll
> note that despite using paging, the current code still buffers everything in
> {{TemporalRow.Set}} anyway .
> Overall, I suspect trying to fix the problems above (particularly the fact
> that existing deletion infos are ignored) is only going to add complexity
> with the current code and we'd still have to fix the inefficiencies. So I
> propose a refactor of that code which does 2 main things:
> # it removes all of {{TemporalRow}} and related classes. Instead, it directly
> uses the existing {{Row}} (with all its deletion infos) and the update being
> applied to it and compute the updates for the view from that. I submit that
> this is more clear/simple, but this also avoid copying every cell of both the
> existing and update data as a {{TemporalCell}}. We can also reuse codes like
> {{Rows.merge}} and {{Rows.diff}} to make the handling of deletions relatively
> painless.
> # instead of dealing with each view one at a time, re-iterating over all
> updates each time, it iterates over each individual updates once and deal
> with each view for that update. This makes it more clear that the reads has
> to care about every view involved, but more importantly allow to deal with
> the read data incrementally, never buffering it all.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
