[ 
https://issues.apache.org/jira/browse/CASSANDRA-8527?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16322908#comment-16322908
 ] 

Jon Haddad commented on CASSANDRA-8527:
---------------------------------------

Overall, nice patch, I've got a few nits though:

* The complexity of {{UnfilteredPartitionIterator.applyToRow}} has increased 
enough where it would be really helpful to add some comments explaining what is 
going on.  Please describe the _intent_ of the code, not what it does.
* Please reorder this:

{code}
else if (!row.primaryKeyLivenessInfo().isLive(ReadCommand.this.nowInSec())
  && row.hasDeletion(ReadCommand.this.nowInSec())
  && !hasTombstones)
{code}

to this:

{code}
else if ( !hasTombstones && 
!row.primaryKeyLivenessInfo().isLive(ReadCommand.this.nowInSec())
                        && row.hasDeletion(ReadCommand.this.nowInSec()))
{code}

The check on the bool is cheaper than the other checks and if that fails we can 
short circuit the other checks.

* Please add some comments on the tests you've created.  There's about 150 
lines of code and no comments to explain what's going on or what's being tested.

When you've revised the patch, could you link a CircleCI build result?  See 
https://cassandra.apache.org/doc/latest/development/testing.html

Thanks,
Jon

> Account for range tombstones wherever we account for tombstones
> ---------------------------------------------------------------
>
>                 Key: CASSANDRA-8527
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-8527
>             Project: Cassandra
>          Issue Type: Improvement
>            Reporter: Sylvain Lebresne
>            Assignee: Alexander Dejanovski
>             Fix For: 4.x
>
>         Attachments: CASSANDRA-8527-4.x.diff
>
>
> As discussed in CASSANDRA-8477, we should make sure the tombstone thresholds 
> also apply to range tombstones, since they poses the same problems than cell 
> tombstones.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org

Reply via email to