[jira] [Commented] (CASSANDRA-9975) Flatten Iterator call hierarchy with a shared Transformer
[
https://issues.apache.org/jira/browse/CASSANDRA-9975?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14976121#comment-14976121
]
Benedict commented on CASSANDRA-9975:
-
Thanks. Committed as 609497471441273367013c09a1e0e1c990726ec7.
> Flatten Iterator call hierarchy with a shared Transformer
> -
>
> Key: CASSANDRA-9975
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9975
> Project: Cassandra
> Issue Type: Sub-task
> Components: Core
>Reporter: Benedict
>Assignee: Benedict
> Fix For: 3.0.0
>
> Attachments: after - limit query.png, after - schema query.png,
> before - limit query.png, before - schema query.png
>
>
> Stepping through a read response is made exceedingly difficult by the sheer
> depth of the call hierarchy, and how rapidly your context jumps around. This
> ticket intend to partially address that, by flattening one of the main causes
> of this: iterator transformations.
> I have a patch that attempts to mitigate (but not entirely eliminate) this,
> through the introduction of a {{RowTransformer}} class that all
> transformations are applied through. If a transformation has already been
> applied, the {{RowTransformer}} class does not wrap a new iterator, but
> instead returns a new {{RowTransformer}} that wraps the original underlying
> (untransformed) iterator and both transformations. This can accumulate an
> arbitrary number of transformations and, quite importantly, can apply the
> filtration step {{Unfiltered -> Row}} in the same instance as well. The
> intention being that a majority of control flow happens inside this
> {{RowTransformer}}, so there is far less context jumping to cope with.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9975) Flatten Iterator call hierarchy with a shared Transformer
[
https://issues.apache.org/jira/browse/CASSANDRA-9975?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14974234#comment-14974234
]
Branimir Lambov commented on CASSANDRA-9975:
Took another look -- patch looks good to me.
> Flatten Iterator call hierarchy with a shared Transformer
> -
>
> Key: CASSANDRA-9975
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9975
> Project: Cassandra
> Issue Type: Sub-task
> Components: Core
>Reporter: Benedict
>Assignee: Benedict
> Fix For: 3.0.0
>
> Attachments: after - limit query.png, after - schema query.png,
> before - limit query.png, before - schema query.png
>
>
> Stepping through a read response is made exceedingly difficult by the sheer
> depth of the call hierarchy, and how rapidly your context jumps around. This
> ticket intend to partially address that, by flattening one of the main causes
> of this: iterator transformations.
> I have a patch that attempts to mitigate (but not entirely eliminate) this,
> through the introduction of a {{RowTransformer}} class that all
> transformations are applied through. If a transformation has already been
> applied, the {{RowTransformer}} class does not wrap a new iterator, but
> instead returns a new {{RowTransformer}} that wraps the original underlying
> (untransformed) iterator and both transformations. This can accumulate an
> arbitrary number of transformations and, quite importantly, can apply the
> filtration step {{Unfiltered -> Row}} in the same instance as well. The
> intention being that a majority of control flow happens inside this
> {{RowTransformer}}, so there is far less context jumping to cope with.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9975) Flatten Iterator call hierarchy with a shared Transformer
[
https://issues.apache.org/jira/browse/CASSANDRA-9975?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14970829#comment-14970829
]
Benedict commented on CASSANDRA-9975:
-
bq. Do you have any results showing what the stack used to look like vs what it
looks like now?
I don't follow, sorry. What do you mean by results? There aren't really any
metrics I know of to quantify this. Do you mean in the earlier version of this
ticket, or prior to this ticket?
To describe qualitatively the change: currently, transformations are applied
via classes extending a wrapping iterator (often anonymous). This means that,
on iteration, each step of each transformation typically involves around 3
nested method calls that jump between the same abstract super classes (so not
at all clear which outer class you're actually stepping through), and so that
seeing where you are wrt the start/end of the transformation is extremely
challenging. It involves moving through the call stack (which can get dozens
deep) to look behind you, and walking through an inconsistent object graph to
see what's ahead of you. Concatenation makes this even worse, as it's not
immediately obvious which of multiple iterators you will next be consuming (or
even that there are multiple options), and establishing this varies with each
different point of conatenation, as is somewhat true for each regular
transformation. The _source_ iterator is also heavily obscured as a result.
This patch changes the stack of transformations to be applied _iteratively_ (as
opposed to recursively) and be stored as an explicit array of helpfully-named
objects, all within a single iterator. So, the call depth is constant during
the application of every update, and you can see exactly which functions have
been applied, and which are yet to be applied, both available consistently via
the same mechanism. The state of all of these functions is available
side-by-side, as is the underlying iterator, and you can easily step through
all of them from a single point of insertion (the {{BaseRow}} or
{{BasePartition}} {{hasNext}} methods).
This patch further flattens the concept of unfiltered/filtered internally,
since there is very little distinction. The external API still disambiguates
between the two kinds of iteration, but internally the transition is just
another function to be applied, so again the transition and control flow are
easier to follow and debug.
Concatenation is also now consistently applied. It does have some slightly more
complex logic than I would have liked when transitioning from one iterator to
the next, but none of that needs to be stepped through or understood by anyone
outside of modifiers to this logic.
Does that clarify sufficiently?
bq. You mention it improves performance do we have any results to support that?
Performance is an explicit non-goal of this ticket, and I have no expectation
of improvement. The latest patch improves performance _wrt the prior version_
which had a slight performance regression due to increased GC when the
transforming iterators were immutable (since this meant a lot of array
allocations). I have kicked off another comparison for confirmation.
> Flatten Iterator call hierarchy with a shared Transformer
> -
>
> Key: CASSANDRA-9975
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9975
> Project: Cassandra
> Issue Type: Sub-task
> Components: Core
>Reporter: Benedict
>Assignee: Benedict
> Fix For: 3.0.0
>
>
> Stepping through a read response is made exceedingly difficult by the sheer
> depth of the call hierarchy, and how rapidly your context jumps around. This
> ticket intend to partially address that, by flattening one of the main causes
> of this: iterator transformations.
> I have a patch that attempts to mitigate (but not entirely eliminate) this,
> through the introduction of a {{RowTransformer}} class that all
> transformations are applied through. If a transformation has already been
> applied, the {{RowTransformer}} class does not wrap a new iterator, but
> instead returns a new {{RowTransformer}} that wraps the original underlying
> (untransformed) iterator and both transformations. This can accumulate an
> arbitrary number of transformations and, quite importantly, can apply the
> filtration step {{Unfiltered -> Row}} in the same instance as well. The
> intention being that a majority of control flow happens inside this
> {{RowTransformer}}, so there is far less context jumping to cope with.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9975) Flatten Iterator call hierarchy with a shared Transformer
[
https://issues.apache.org/jira/browse/CASSANDRA-9975?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14971143#comment-14971143
]
T Jake Luciani commented on CASSANDRA-9975:
---
bq. Aleksey suggested you may be looking for some screenshots of debug points
for comparison.
Yes, sorry I should have been clearer.
Ok I can see that the state is mostly self-contained now, thanks
> Flatten Iterator call hierarchy with a shared Transformer
> -
>
> Key: CASSANDRA-9975
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9975
> Project: Cassandra
> Issue Type: Sub-task
> Components: Core
>Reporter: Benedict
>Assignee: Benedict
> Fix For: 3.0.0
>
> Attachments: after - limit query.png, after - schema query.png,
> before - limit query.png, before - schema query.png
>
>
> Stepping through a read response is made exceedingly difficult by the sheer
> depth of the call hierarchy, and how rapidly your context jumps around. This
> ticket intend to partially address that, by flattening one of the main causes
> of this: iterator transformations.
> I have a patch that attempts to mitigate (but not entirely eliminate) this,
> through the introduction of a {{RowTransformer}} class that all
> transformations are applied through. If a transformation has already been
> applied, the {{RowTransformer}} class does not wrap a new iterator, but
> instead returns a new {{RowTransformer}} that wraps the original underlying
> (untransformed) iterator and both transformations. This can accumulate an
> arbitrary number of transformations and, quite importantly, can apply the
> filtration step {{Unfiltered -> Row}} in the same instance as well. The
> intention being that a majority of control flow happens inside this
> {{RowTransformer}}, so there is far less context jumping to cope with.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9975) Flatten Iterator call hierarchy with a shared Transformer
[
https://issues.apache.org/jira/browse/CASSANDRA-9975?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14969544#comment-14969544
]
Benedict commented on CASSANDRA-9975:
-
bq. Wondering if BasePartitions.hasNext/applyOne shouldn't take responsibility
of closing next if applyToPartitions returned null since other aspects of the
transformer do close automatically?
I have been back and forth on this one myself. I just started to do it again,
and remembered why I chose not to: half of the places we close the iterator in
a transformation still return a non-null result. So the help offered is
inconsistent. It also (slightly) makes the main transformation loop uglier. I'm
happy to do this, as I don't have a strong feeling on it, but both times I've
looked at it I've ultimately reversed course.
bq. toQuery in checkForShortRead no longer has division-by-zero protection. Is
it no longer necessary?
Line 345 has added a check for if we've received any live data in the most
recent request, which captures this AFAICT. i.e., if we get zero rows first
time, there is no point asking for more, since there aren't any.
I've rebased and split out into a separate package. I've also run
eclipse-warnings, but all I see is "[java] incorrect classpath:
/home/benedict/git/cassandra/build/cobertura/classes". [~tjake], is this
expected / indicates we're otherwise good? I assume I'm meant to see a warning
or two (else Branimir probably wouldn't have mentioned it)
CI looked goodish last run. AFAICT it was equivalent to mainline. I'll see if I
can get some certainty before commit.
> Flatten Iterator call hierarchy with a shared Transformer
> -
>
> Key: CASSANDRA-9975
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9975
> Project: Cassandra
> Issue Type: Sub-task
> Components: Core
>Reporter: Benedict
>Assignee: Benedict
> Fix For: 3.0.0
>
>
> Stepping through a read response is made exceedingly difficult by the sheer
> depth of the call hierarchy, and how rapidly your context jumps around. This
> ticket intend to partially address that, by flattening one of the main causes
> of this: iterator transformations.
> I have a patch that attempts to mitigate (but not entirely eliminate) this,
> through the introduction of a {{RowTransformer}} class that all
> transformations are applied through. If a transformation has already been
> applied, the {{RowTransformer}} class does not wrap a new iterator, but
> instead returns a new {{RowTransformer}} that wraps the original underlying
> (untransformed) iterator and both transformations. This can accumulate an
> arbitrary number of transformations and, quite importantly, can apply the
> filtration step {{Unfiltered -> Row}} in the same instance as well. The
> intention being that a majority of control flow happens inside this
> {{RowTransformer}}, so there is far less context jumping to cope with.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9975) Flatten Iterator call hierarchy with a shared Transformer
[
https://issues.apache.org/jira/browse/CASSANDRA-9975?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14969574#comment-14969574
]
T Jake Luciani commented on CASSANDRA-9975:
---
That's fine/unrelated.
[~benedict] Since I'm on this ticket now some questions. Do you have any
results showing what the stack used to look like vs what it looks like now? You
mention it improves performance do we have any results to support that?
> Flatten Iterator call hierarchy with a shared Transformer
> -
>
> Key: CASSANDRA-9975
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9975
> Project: Cassandra
> Issue Type: Sub-task
> Components: Core
>Reporter: Benedict
>Assignee: Benedict
> Fix For: 3.0.0
>
>
> Stepping through a read response is made exceedingly difficult by the sheer
> depth of the call hierarchy, and how rapidly your context jumps around. This
> ticket intend to partially address that, by flattening one of the main causes
> of this: iterator transformations.
> I have a patch that attempts to mitigate (but not entirely eliminate) this,
> through the introduction of a {{RowTransformer}} class that all
> transformations are applied through. If a transformation has already been
> applied, the {{RowTransformer}} class does not wrap a new iterator, but
> instead returns a new {{RowTransformer}} that wraps the original underlying
> (untransformed) iterator and both transformations. This can accumulate an
> arbitrary number of transformations and, quite importantly, can apply the
> filtration step {{Unfiltered -> Row}} in the same instance as well. The
> intention being that a majority of control flow happens inside this
> {{RowTransformer}}, so there is far less context jumping to cope with.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9975) Flatten Iterator call hierarchy with a shared Transformer
[
https://issues.apache.org/jira/browse/CASSANDRA-9975?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14969268#comment-14969268
]
Branimir Lambov commented on CASSANDRA-9975:
The code is complicated, but looks correct and is well documented. I think it's
practically ready to commit once CI is clean.
Wondering if {{BasePartitions.hasNext/applyOne}} shouldn't take responsibility
of closing {{next}} if {{applyToPartitions}} returned {{null}} since other
aspects of the transformer do close automatically?
[toQuery in
checkForShortRead|https://github.com/apache/cassandra/compare/trunk...belliottsmith:9975#diff-8781f9483cca1cfc87145c767295cc79R358]
no longer has division-by-zero protection. Is it no longer necessary?
You should run the {{eclipse-warnings}} ant target and suppress any new
warnings.
> Flatten Iterator call hierarchy with a shared Transformer
> -
>
> Key: CASSANDRA-9975
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9975
> Project: Cassandra
> Issue Type: Sub-task
> Components: Core
>Reporter: Benedict
>Assignee: Benedict
> Fix For: 3.0.0
>
>
> Stepping through a read response is made exceedingly difficult by the sheer
> depth of the call hierarchy, and how rapidly your context jumps around. This
> ticket intend to partially address that, by flattening one of the main causes
> of this: iterator transformations.
> I have a patch that attempts to mitigate (but not entirely eliminate) this,
> through the introduction of a {{RowTransformer}} class that all
> transformations are applied through. If a transformation has already been
> applied, the {{RowTransformer}} class does not wrap a new iterator, but
> instead returns a new {{RowTransformer}} that wraps the original underlying
> (untransformed) iterator and both transformations. This can accumulate an
> arbitrary number of transformations and, quite importantly, can apply the
> filtration step {{Unfiltered -> Row}} in the same instance as well. The
> intention being that a majority of control flow happens inside this
> {{RowTransformer}}, so there is far less context jumping to cope with.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9975) Flatten Iterator call hierarchy with a shared Transformer
[
https://issues.apache.org/jira/browse/CASSANDRA-9975?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14969306#comment-14969306
]
Branimir Lambov commented on CASSANDRA-9975:
It does serve a lot of purposes, that's where the complexity comes from.
It may be better if the {{Transformer}} class grew into a package, so that each
of its subclasses can be in its own source file, to turn them into easier
chunks for digesting.
> Flatten Iterator call hierarchy with a shared Transformer
> -
>
> Key: CASSANDRA-9975
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9975
> Project: Cassandra
> Issue Type: Sub-task
> Components: Core
>Reporter: Benedict
>Assignee: Benedict
> Fix For: 3.0.0
>
>
> Stepping through a read response is made exceedingly difficult by the sheer
> depth of the call hierarchy, and how rapidly your context jumps around. This
> ticket intend to partially address that, by flattening one of the main causes
> of this: iterator transformations.
> I have a patch that attempts to mitigate (but not entirely eliminate) this,
> through the introduction of a {{RowTransformer}} class that all
> transformations are applied through. If a transformation has already been
> applied, the {{RowTransformer}} class does not wrap a new iterator, but
> instead returns a new {{RowTransformer}} that wraps the original underlying
> (untransformed) iterator and both transformations. This can accumulate an
> arbitrary number of transformations and, quite importantly, can apply the
> filtration step {{Unfiltered -> Row}} in the same instance as well. The
> intention being that a majority of control flow happens inside this
> {{RowTransformer}}, so there is far less context jumping to cope with.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9975) Flatten Iterator call hierarchy with a shared Transformer
[
https://issues.apache.org/jira/browse/CASSANDRA-9975?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14969292#comment-14969292
]
Sylvain Lebresne commented on CASSANDRA-9975:
-
bq. The code is complicated, but looks correct and is well documented.
Do you have ideas/suggestions to simplify it out of curiosity? Or does it
appears justified/hard to avoid.
> Flatten Iterator call hierarchy with a shared Transformer
> -
>
> Key: CASSANDRA-9975
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9975
> Project: Cassandra
> Issue Type: Sub-task
> Components: Core
>Reporter: Benedict
>Assignee: Benedict
> Fix For: 3.0.0
>
>
> Stepping through a read response is made exceedingly difficult by the sheer
> depth of the call hierarchy, and how rapidly your context jumps around. This
> ticket intend to partially address that, by flattening one of the main causes
> of this: iterator transformations.
> I have a patch that attempts to mitigate (but not entirely eliminate) this,
> through the introduction of a {{RowTransformer}} class that all
> transformations are applied through. If a transformation has already been
> applied, the {{RowTransformer}} class does not wrap a new iterator, but
> instead returns a new {{RowTransformer}} that wraps the original underlying
> (untransformed) iterator and both transformations. This can accumulate an
> arbitrary number of transformations and, quite importantly, can apply the
> filtration step {{Unfiltered -> Row}} in the same instance as well. The
> intention being that a majority of control flow happens inside this
> {{RowTransformer}}, so there is far less context jumping to cope with.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9975) Flatten Iterator call hierarchy with a shared Transformer
[
https://issues.apache.org/jira/browse/CASSANDRA-9975?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14969381#comment-14969381
]
Benedict commented on CASSANDRA-9975:
-
bq. It may be better if the Transformer class grew into a package
More than happy to make that change. I agree there's a lot in there, especially
boilerplate (static methods etc), which might be easier to digest in smaller
chunks.
As for general complexity: this should just be taken as a first step, in my
book. There's a lot that can potentially be done in follow ups, but I didn't
want to eat the whole world. An imperfect first step is still a step in the
right direction. The interaction with counters, for instance, could be more
tightly coupled to perhaps reduce complexity wrt "stopping early". Possibly the
control flow can be simplified for the {{MoreContents}} work. There are many
avenues to follow, in fact.
My main goal here wasn't to make _this_ code simple (although I tried to make
it as good as I could personally manage in a reasonable time frame, given the
headline goal), but to make layers of the transformations themselves simple to
follow.
> Flatten Iterator call hierarchy with a shared Transformer
> -
>
> Key: CASSANDRA-9975
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9975
> Project: Cassandra
> Issue Type: Sub-task
> Components: Core
>Reporter: Benedict
>Assignee: Benedict
> Fix For: 3.0.0
>
>
> Stepping through a read response is made exceedingly difficult by the sheer
> depth of the call hierarchy, and how rapidly your context jumps around. This
> ticket intend to partially address that, by flattening one of the main causes
> of this: iterator transformations.
> I have a patch that attempts to mitigate (but not entirely eliminate) this,
> through the introduction of a {{RowTransformer}} class that all
> transformations are applied through. If a transformation has already been
> applied, the {{RowTransformer}} class does not wrap a new iterator, but
> instead returns a new {{RowTransformer}} that wraps the original underlying
> (untransformed) iterator and both transformations. This can accumulate an
> arbitrary number of transformations and, quite importantly, can apply the
> filtration step {{Unfiltered -> Row}} in the same instance as well. The
> intention being that a majority of control flow happens inside this
> {{RowTransformer}}, so there is far less context jumping to cope with.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9975) Flatten Iterator call hierarchy with a shared Transformer
[
https://issues.apache.org/jira/browse/CASSANDRA-9975?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14742477#comment-14742477
]
Benedict commented on CASSANDRA-9975:
-
On writing the follow up, CASSANDRA-9976, it became apparent it was easier and
cleaner to do both at once, since they ultimately share quite a bit of
functionality and approach. This is now available
[here|https://github.com/belliottsmith/cassandra/tree/9975].
This version also goes a step further to completely eliminate the wrapping
partition iterators, and all but a handful of wrapping row iterators (the
remainder of which can be eliminated following CASSANDRA-9986). This is
achieved by supporting concatenation / short read protection via a
transformation function that "refils" the contents of an iterator with one or
more new iterators. i.e.m once the input we're transforming is exhausted, we
ask for a new one, which is swapped in, and the function stacks updated as
necessary. This all but eliminates the jumping around of control flow for the
outer execution of a query, although the inner execution (memtable, sstable)
still has some cleaning up to be done to make it easy to debug.
> Flatten Iterator call hierarchy with a shared Transformer
> -
>
> Key: CASSANDRA-9975
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9975
> Project: Cassandra
> Issue Type: Sub-task
> Components: Core
>Reporter: Benedict
>Assignee: Benedict
> Fix For: 3.0.x
>
>
> Stepping through a read response is made exceedingly difficult by the sheer
> depth of the call hierarchy, and how rapidly your context jumps around. This
> ticket intend to partially address that, by flattening one of the main causes
> of this: iterator transformations.
> I have a patch that attempts to mitigate (but not entirely eliminate) this,
> through the introduction of a {{RowTransformer}} class that all
> transformations are applied through. If a transformation has already been
> applied, the {{RowTransformer}} class does not wrap a new iterator, but
> instead returns a new {{RowTransformer}} that wraps the original underlying
> (untransformed) iterator and both transformations. This can accumulate an
> arbitrary number of transformations and, quite importantly, can apply the
> filtration step {{Unfiltered -> Row}} in the same instance as well. The
> intention being that a majority of control flow happens inside this
> {{RowTransformer}}, so there is far less context jumping to cope with.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
