[jira] [Commented] (CASSANDRA-9985) Introduce our own AbstractIterator
[
https://issues.apache.org/jira/browse/CASSANDRA-9985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14661392#comment-14661392
]
Benedict commented on CASSANDRA-9985:
-
Right, it seems exceedingly difficult to get into, and is a worse misuse of the
iterator than the iterator can handle.
My main reason to not want to address it, though, is that we will be stepping
through this {{AbstractIterator.hasNext()}} often, and I'd like it to be easy
and attractive to do so. I had a quick go at making it remain easy to read and,
hopefully, step through, while still catching this edge case. I've pushed a
version that I think meets the criteria.
Introduce our own AbstractIterator
--
Key: CASSANDRA-9985
URL: https://issues.apache.org/jira/browse/CASSANDRA-9985
Project: Cassandra
Issue Type: Sub-task
Components: Core
Reporter: Benedict
Assignee: Benedict
Priority: Trivial
Fix For: 3.0.0 rc1
The Guava AbstractIterator not only has unnecessary method call depth, it is
difficult to debug without attaching source. Since it's absolutely trivial to
write our own, and it's used widely within the codebase, I think we should do
so.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9985) Introduce our own AbstractIterator
[ https://issues.apache.org/jira/browse/CASSANDRA-9985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14662002#comment-14662002 ] Ariel Weisberg commented on CASSANDRA-9985: --- I am +1. Looks like we got to have our cake and eat it to which is nice. Introduce our own AbstractIterator -- Key: CASSANDRA-9985 URL: https://issues.apache.org/jira/browse/CASSANDRA-9985 Project: Cassandra Issue Type: Sub-task Components: Core Reporter: Benedict Assignee: Benedict Priority: Trivial Fix For: 3.0.0 rc1 The Guava AbstractIterator not only has unnecessary method call depth, it is difficult to debug without attaching source. Since it's absolutely trivial to write our own, and it's used widely within the codebase, I think we should do so. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9985) Introduce our own AbstractIterator
[
https://issues.apache.org/jira/browse/CASSANDRA-9985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14659823#comment-14659823
]
Benedict commented on CASSANDRA-9985:
-
bq. Can I bring up the icache boogyman?
You can. Since I'm it's main champion, it's a powerful weapon against me.
However in this case a little analysis shows that Guava doesn't even use its
own {{AbstractIterator}} (either of them!) very much at all. In fact nowhere
that we do, from what I can tell. I believe CASSANDRA-9471 uses
{{Iterables.mergeSorted}} which does - but we can perform a more efficient
merge since it's only 2-way, and otherwise should prefer our own Mergeiterator,
which has a more efficient heap implementation. So I will update it to avoid
using this function.
Which is a relief, since I am not in favour of overriding external packages if
we can avoid it, as it plays havoc with anyone wanting to use signed jars.
The only other place we use Guava {{AbstractIterator}} is in OHC. Since this is
the HotKey iterator, it is used infrequently and probably not to be troubled
over.
Of course, we do run the risk of in future introducing the occasional usage,
but those risks are small given how narrowly it is deployed in Guava. We have a
lot of minor issues around duplication, and we should probably file a ticket to
specifically track and fix it en masse. Especially regarding duplicate
functionality between Guava and java.util.function, but no harm setting up some
analysis for everything. It would be great if we had some automated analysis to
tell us of duplicate code candidates, in general. I'm certain such tools exist.
I'll take a look at purloining any Guava tests.
Introduce our own AbstractIterator
--
Key: CASSANDRA-9985
URL: https://issues.apache.org/jira/browse/CASSANDRA-9985
Project: Cassandra
Issue Type: Sub-task
Components: Core
Reporter: Benedict
Assignee: Benedict
Priority: Trivial
Fix For: 3.0.0 rc1
The Guava AbstractIterator not only has unnecessary method call depth, it is
difficult to debug without attaching source. Since it's absolutely trivial to
write our own, and it's used widely within the codebase, I think we should do
so.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9985) Introduce our own AbstractIterator
[
https://issues.apache.org/jira/browse/CASSANDRA-9985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14659837#comment-14659837
]
Benedict commented on CASSANDRA-9985:
-
So, I've pushed a version with Guava's test case included, but with two failing
tests removed because IMO they aren't things we should worry about:
1) Reentrancy in {{hasNext()}}, i.e. if you create a {{computeNext()}} method
that recursively calls {{hasNext()}} then it will throw ISE
2) Continuing to use an iterator that threw an exception during
{{computeNext()}}, which will also throw ISE
My view is that neither check buys us much, and I prefer the code clarity of
the current implementation. However support is easily added at the expense of a
bit of ugliness.
Introduce our own AbstractIterator
--
Key: CASSANDRA-9985
URL: https://issues.apache.org/jira/browse/CASSANDRA-9985
Project: Cassandra
Issue Type: Sub-task
Components: Core
Reporter: Benedict
Assignee: Benedict
Priority: Trivial
Fix For: 3.0.0 rc1
The Guava AbstractIterator not only has unnecessary method call depth, it is
difficult to debug without attaching source. Since it's absolutely trivial to
write our own, and it's used widely within the codebase, I think we should do
so.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9985) Introduce our own AbstractIterator
[
https://issues.apache.org/jira/browse/CASSANDRA-9985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14660488#comment-14660488
]
Ariel Weisberg commented on CASSANDRA-9985:
---
#1 seems less important. #2 seems important. My thinking is that this is
library code we'll never touch again so ugly isn't a great reason. Performance
would be, but that would be trading performance for problems down the road. If
performance were the reason then maybe tying it to assertions?
WRT to #2. In practice computeNext is definitely going to throw. Now what are
the odds the exception handling keeps the iterator in scope, and then comes
back to try and use it? Not so bad maybe?
Iterator foo = bar.iterator();
while (foo.hasNext())
{
try {
//stuff
}
catch (Exception e)
{
//Log or whatever
}
}
If we do #2 is #1 still a lot of code?
Introduce our own AbstractIterator
--
Key: CASSANDRA-9985
URL: https://issues.apache.org/jira/browse/CASSANDRA-9985
Project: Cassandra
Issue Type: Sub-task
Components: Core
Reporter: Benedict
Assignee: Benedict
Priority: Trivial
Fix For: 3.0.0 rc1
The Guava AbstractIterator not only has unnecessary method call depth, it is
difficult to debug without attaching source. Since it's absolutely trivial to
write our own, and it's used widely within the codebase, I think we should do
so.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9985) Introduce our own AbstractIterator
[ https://issues.apache.org/jira/browse/CASSANDRA-9985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14655039#comment-14655039 ] Benedict commented on CASSANDRA-9985: - Trivial patch available [here|https://github.com/belliottsmith/cassandra/tree/abstract-iterator] I realise this may (or may not) be contentious, so it is just a suggestion. Opinions solicited. Introduce our own AbstractIterator -- Key: CASSANDRA-9985 URL: https://issues.apache.org/jira/browse/CASSANDRA-9985 Project: Cassandra Issue Type: Sub-task Components: Core Reporter: Benedict Assignee: Benedict Priority: Trivial Fix For: 3.0.0 rc1 The Guava AbstractIterator not only has unnecessary method call depth, it is difficult to debug without attaching source. Since it's absolutely trivial to write our own, and it's used widely within the codebase, I think we should do so. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9985) Introduce our own AbstractIterator
[ https://issues.apache.org/jira/browse/CASSANDRA-9985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14658264#comment-14658264 ] Jason Brown commented on CASSANDRA-9985: Rereading through the guava AbstractIterator code, I didn't feel it to be too overbearing (there's the state machine and a Precondition call). TBH, I'm kinda mixed on this - I appreciate one less external dependency, and the patch is easy enough. If others are fine with the change, I will be, as well. Nit: - in your AbstractIterator.remove(), it's a no-op. In the guava version, they throw a UnsupportedOperationException as it derives from UnmodifiableIterator Introduce our own AbstractIterator -- Key: CASSANDRA-9985 URL: https://issues.apache.org/jira/browse/CASSANDRA-9985 Project: Cassandra Issue Type: Sub-task Components: Core Reporter: Benedict Assignee: Benedict Priority: Trivial Fix For: 3.0.0 rc1 The Guava AbstractIterator not only has unnecessary method call depth, it is difficult to debug without attaching source. Since it's absolutely trivial to write our own, and it's used widely within the codebase, I think we should do so. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9985) Introduce our own AbstractIterator
[ https://issues.apache.org/jira/browse/CASSANDRA-9985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14658273#comment-14658273 ] Benedict commented on CASSANDRA-9985: - I wouldn't care if it weren't for the sheer number of these calls that we chain together now, so that 75% of your code stepping is through a linked jar. With multiple versions it is hard to link the source, and so the majority of your debug stepping is through a morass of unknown. This patch also removes one method call from that chain, which shrinks the height of the call tree significantly when they're added together. But, as I say, I figured this little patch might split opinion so I only propose it for inclusion at the consensus of the community. It;s worth noting that CASSANDRA-9975 also mitigates the problem, however it leaves a lot of these iterators in the call tree. Introduce our own AbstractIterator -- Key: CASSANDRA-9985 URL: https://issues.apache.org/jira/browse/CASSANDRA-9985 Project: Cassandra Issue Type: Sub-task Components: Core Reporter: Benedict Assignee: Benedict Priority: Trivial Fix For: 3.0.0 rc1 The Guava AbstractIterator not only has unnecessary method call depth, it is difficult to debug without attaching source. Since it's absolutely trivial to write our own, and it's used widely within the codebase, I think we should do so. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9985) Introduce our own AbstractIterator
[ https://issues.apache.org/jira/browse/CASSANDRA-9985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14658310#comment-14658310 ] Benedict commented on CASSANDRA-9985: - FTR, I've had to update the patch to closer to Guava because we do in fact use null returns (I had thought we did not). To be honest, I would be totally comfortable just porting the Guava code wholesale (or almost wholesale; the removal of tryComputeNext is still nice). Introduce our own AbstractIterator -- Key: CASSANDRA-9985 URL: https://issues.apache.org/jira/browse/CASSANDRA-9985 Project: Cassandra Issue Type: Sub-task Components: Core Reporter: Benedict Assignee: Benedict Priority: Trivial Fix For: 3.0.0 rc1 The Guava AbstractIterator not only has unnecessary method call depth, it is difficult to debug without attaching source. Since it's absolutely trivial to write our own, and it's used widely within the codebase, I think we should do so. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9985) Introduce our own AbstractIterator
[ https://issues.apache.org/jira/browse/CASSANDRA-9985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14658893#comment-14658893 ] Ariel Weisberg commented on CASSANDRA-9985: --- Can you use the AbstractIterator test from Guava on this? It's a chance to check for any gotchas they may have come across and quietly engineered around. Can I bring up the icache boogyman? Guava appears to have two AbstractIterator implementations. One in base and one in collect? Maybe what we want is to replace Guava's abstract iterator(s) by putting it in their package? Introduce our own AbstractIterator -- Key: CASSANDRA-9985 URL: https://issues.apache.org/jira/browse/CASSANDRA-9985 Project: Cassandra Issue Type: Sub-task Components: Core Reporter: Benedict Assignee: Benedict Priority: Trivial Fix For: 3.0.0 rc1 The Guava AbstractIterator not only has unnecessary method call depth, it is difficult to debug without attaching source. Since it's absolutely trivial to write our own, and it's used widely within the codebase, I think we should do so. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
