[jira] [Commented] (CASSANDRA-8527) Account for range tombstones wherever we account for tombstones
[ https://issues.apache.org/jira/browse/CASSANDRA-8527?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16351355#comment-16351355 ] Alexander Dejanovski commented on CASSANDRA-8527: - [~rustyrazorblade], I've made a patch for 3.11 as well. Here are the patches and CircleCI builds : ||branch||tests|| |[trunk\|[https://github.com/apache/cassandra/compare/trunk]...thelastpickle:CASSANDRA-8527-trace-range-tombstones?expand=1]|[tests\|https://circleci.com/gh/thelastpickle/cassandra/105]| |[3.11\|[https://github.com/apache/cassandra/compare/cassandra-3.11]...thelastpickle:CASSANDRA-8527-3.11]|[tests\|https://circleci.com/gh/thelastpickle/cassandra/106]| > 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 >Priority: Major > 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 (v7.6.3#76005) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-8527) Account for range tombstones wherever we account for tombstones
[ https://issues.apache.org/jira/browse/CASSANDRA-8527?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16325621#comment-16325621 ] Alexander Dejanovski commented on CASSANDRA-8527: - Hi [~rustyrazorblade], I've made the requested changes and pushed it here : https://github.com/apache/cassandra/compare/trunk...thelastpickle:CASSANDRA-8527-trace-range-tombstones I added comments in both the code and the tests, and refactored the tests a bit to remove code duplication. Here's the latest CircleCI report for unit tests : https://circleci.com/gh/thelastpickle/cassandra/93 It fails on the same tests than trunk does : https://circleci.com/gh/thelastpickle/cassandra/85 Let me know if that looks good to you. > 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
[jira] [Commented] (CASSANDRA-8527) Account for range tombstones wherever we account for tombstones
[
https://issues.apache.org/jira/browse/CASSANDRA-8527?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=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
[jira] [Commented] (CASSANDRA-8527) Account for range tombstones wherever we account for tombstones
[ https://issues.apache.org/jira/browse/CASSANDRA-8527?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16321328#comment-16321328 ] Jon Haddad commented on CASSANDRA-8527: --- Thanks [~adejanovski] I'll look at look at the patch this week, thanks for the revision. > 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
[jira] [Commented] (CASSANDRA-8527) Account for range tombstones wherever we account for tombstones
[ https://issues.apache.org/jira/browse/CASSANDRA-8527?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16315931#comment-16315931 ] Alexander Dejanovski commented on CASSANDRA-8527: - [~rustyrazorblade], I've simplified the patch a lot following the discussions on the dev ML. Counting row tombstones as "deleted rows" creates a confusion that rows shadowed by multi row range tombstones would be counted too. Since it's not the case (rows shadowed by RT get merged away before the ReadCommand class and only 2 tombstone cells get counted for each RT), I've moved back to counting row tombstones as part of the current tombstone cell counter. Here's the updated code : https://github.com/apache/cassandra/compare/trunk...thelastpickle:CASSANDRA-8527-trace-range-tombstones We will deal with shadowed rows in CASSANDRA-14149. > 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 > > > 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
[jira] [Commented] (CASSANDRA-8527) Account for range tombstones wherever we account for tombstones
[ https://issues.apache.org/jira/browse/CASSANDRA-8527?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16313532#comment-16313532 ] Alexander Dejanovski commented on CASSANDRA-8527: - I created a follow up ticket under CASSANDRA-14149 to handle specific counts of tombstones by types and shadowed cells/rows. > 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.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
[jira] [Commented] (CASSANDRA-8527) Account for range tombstones wherever we account for tombstones
[
https://issues.apache.org/jira/browse/CASSANDRA-8527?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16297976#comment-16297976
]
Alexander Dejanovski commented on CASSANDRA-8527:
-
I pushed a small fix in my branch to avoid counting some rows in both
{{deletedRows}} and {{tombstones}}.
If all cells in a row have expired then each will get counted as a tombstone
but it will also make {{row.hasLiveData() }} return false.
I've changed the test to :
{code:java}
if (row.hasLiveData(ReadCommand.this.nowInSec(), enforceStrictLiveness))
++liveRows;
else if (!row.primaryKeyLivenessInfo().isLive(ReadCommand.this.nowInSec()))
{
++deletedRows;
}
{code}
Also started the discussion on the dev ML today.
> 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.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
[jira] [Commented] (CASSANDRA-8527) Account for range tombstones wherever we account for tombstones
[
https://issues.apache.org/jira/browse/CASSANDRA-8527?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16297189#comment-16297189
]
Jon Haddad commented on CASSANDRA-8527:
---
{quote}
The way the code is architected, I don't see a way of returning the number of
range tombstones without pretty deep changes.
Counting the number of deleted rows is already very interesting and would not
require dangerous modifications.
{quote}
OK, we can do it as a follow up.
{quote}
But if we can't do this in a new major version, then when is the appropriate
time ?
A first step could be to introduce a flag and an additional warning.
It would be something like :
{quote}
I'm not saying we shouldn't do it in this version. I think we should kick this
discussion into the dev ML to get some more eyes on it. As far as I can tell,
the options are
1. Keep the old behavior
2. Keep new
3. Config for picking
4. Separate config options for both.
{quote}
Do you think adding Mockito to the list of dependencies is reasonable/wanted ?
{quote}
I have no objections, and I think it would help make the codebase more
testable.
> 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.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
[jira] [Commented] (CASSANDRA-8527) Account for range tombstones wherever we account for tombstones
[
https://issues.apache.org/jira/browse/CASSANDRA-8527?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16297043#comment-16297043
]
Alexander Dejanovski commented on CASSANDRA-8527:
-
[~rustyrazorblade], as much as I would like to count the range tombstones, my
understanding of the code shows that they are already merged with the rows
before being returned to the ReadCommand subclass in charge.
You can see here that this is done on purpose because the code path responsible
for reading a partition from memtables and sstables can be used from different
places (like compaction for example) :
https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/db/SinglePartitionReadCommand.java#L563-L578
The way the code is architected, I don't see a way of returning the number of
range tombstones without pretty deep changes.
Counting the number of deleted rows is already very interesting and would not
require dangerous modifications.
Wether or not to count such rows in the failure threshold is clearly debatable
as it would indeed break existing (but probably poorly performing) queries.
But if we can't do this in a new major version, then when is the appropriate
time ?
A first step could be to introduce a flag and an additional warning.
It would be something like :
{noformat}
include_deleted_rows_in_tombstone_failure_threshold: false
{noformat}
If set to false (which would be the default at first) then we would issue a
warning that the query would probably fail in future versions or if the flag
was set to true.
If set to true, then the count of deleted rows would be summed to the tombstone
count to fail queries.
Then we could change this (or not) to false in future releases. I seem to
recall that an similar thing was done for cross_node_timeout and it is still
set to false by default to avoid timeouts when nodes aren't properly in sync on
time.
I've taken a look at how I could add unit tests for that code, and I would need
to hook on the table metrics of the ColumnFamilyStore object. The simplest way
of doing this without running the whole stack would be to use a Mockito spy.
Do you think adding Mockito to the list of dependencies is reasonable/wanted ?
> 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.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
[jira] [Commented] (CASSANDRA-8527) Account for range tombstones wherever we account for tombstones
[
https://issues.apache.org/jira/browse/CASSANDRA-8527?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16295773#comment-16295773
]
Jon Haddad commented on CASSANDRA-8527:
---
It's a little odd that we'd still report back "0 tombstones" when in fact there
are tombstones. I'd love to see the number of range tombstones separately
tracked and reported as part of this patch.
For discussion's sake, I'd like to point out that this patch changes the
behavior of the tombstone warning and failure thresholds:
{code}
-if (tombstones > failureThreshold &&
respectTombstoneThresholds)
+if (tombstones + deletedRows > failureThreshold &&
respectTombstoneThresholds)
{code}
I haven't given this much thought yet about if it's the "right way", but it's
important to note as it can significantly change the behavior of when queries
warn and fail.
> 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.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
[jira] [Commented] (CASSANDRA-8527) Account for range tombstones wherever we account for tombstones
[
https://issues.apache.org/jira/browse/CASSANDRA-8527?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16295331#comment-16295331
]
Alexander Dejanovski commented on CASSANDRA-8527:
-
The branch containing the patch is here :
https://github.com/thelastpickle/cassandra/tree/CASSANDRA-8527-trace-range-tombstones
Instead of counting range tombstones, the plan here is to count all rows that
are "not live" and account for them in the tombstone threshold.
Tracing has been adjusted as follows :
{noformat}
WARN [ReadStage-2] 2017-12-18 18:22:31,352 ReadCommand.java:491 - Read 2086
live rows, 2440 deleted rows and 0 tombstone cells for query...
{noformat}
And {{nodetool tablestats}} reports them as follows :
{noformat}
Average live cells per slice (last five minutes): 2299.0
Maximum live cells per slice (last five minutes): 2299
Average deleted cells per slice (last five minutes): 2759.0
Maximum deleted cells per slice (last five minutes): 2759
Average tombstones per slice (last five minutes): 1.0
Maximum tombstones per slice (last five minutes): 1
{noformat}
This patch would be a good thing to contribute to 3.11.x since we can run into
major read performance issues due to range tombstones, but no metric to help
diagnosing.
Not sure it can pass as anything else than a new feature though.
> 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
>
>
> 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
[jira] [Commented] (CASSANDRA-8527) Account for range tombstones wherever we account for tombstones
[ https://issues.apache.org/jira/browse/CASSANDRA-8527?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16295313#comment-16295313 ] Jon Haddad commented on CASSANDRA-8527: --- I've got someone that wants to contribute this to 4.0 so I'm going to unassign and fix the version number. > 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 > Fix For: 2.2.x > > > 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
[jira] [Commented] (CASSANDRA-8527) Account for range tombstones wherever we account for tombstones
[ https://issues.apache.org/jira/browse/CASSANDRA-8527?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15172122#comment-15172122 ] Sylvain Lebresne commented on CASSANDRA-8527: - There is a bunch of places where we don't properly account for range tombstone, not just for the thresholds. Those include tracing, statistics etc... We should fix all those while we're at it so expanding the title slightly. > 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 > Fix For: 2.2.x > > > 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.3.4#6332)
