[jira] [Commented] (CASSANDRA-9985) Introduce our own AbstractIterator
[ https://issues.apache.org/jira/browse/CASSANDRA-9985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=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-tabpanel&focusedCommentId=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-tabpanel&focusedCommentId=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-tabpanel&focusedCommentId=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-tabpanel&focusedCommentId=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-tabpanel&focusedCommentId=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)
[jira] [Commented] (CASSANDRA-9985) Introduce our own AbstractIterator
[ https://issues.apache.org/jira/browse/CASSANDRA-9985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=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-tabpanel&focusedCommentId=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-tabpanel&focusedCommentId=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-tabpanel&focusedCommentId=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)