[jira] [Commented] (CASSANDRA-9985) Introduce our own AbstractIterator

2015-08-07 Thread Ariel Weisberg (JIRA)

[ 
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

2015-08-06 Thread Benedict (JIRA)

[ 
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

2015-08-06 Thread Ariel Weisberg (JIRA)

[ 
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

2015-08-06 Thread Benedict (JIRA)

[ 
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

2015-08-06 Thread Benedict (JIRA)

[ 
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

2015-08-05 Thread Ariel Weisberg (JIRA)

[ 
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

2015-08-05 Thread Benedict (JIRA)

[ 
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

2015-08-05 Thread Benedict (JIRA)

[ 
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

2015-08-05 Thread Jason Brown (JIRA)

[ 
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

2015-08-05 Thread Benedict (JIRA)

[ 
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)