Re: CASSANDRA-8527

[Aleksey Yeshchenko]

Sorry, don’t have strong feelings or suggestions regarding naming in 
logs/warnings.

But I agree with this behaviour.

—
AY

On 5 January 2018 at 19:31:58, Alexander Dejanovski (a...@thelastpickle.com) 
wrote:

Right, I think naming it "deleted rows" is misleading because it suggests  
that 100 rows shadowed by one RT would be counted as 100 tombstones, which  
is not the case here (those get merged away before getting counted).  

The patch counts row tombstones, where (after merge) one tombstone shadows  
a single row.  
Renaming "deleted rows" to "tombstone rows" or something similar would make  
more sense then?  

Le ven. 5 janv. 2018 à 20:24, Aleksey Yeshchenko  a  
écrit :  

> Counting the number of range tombstones - sure, that is reasonable.  
>  
> Counting cells/rows shadowed by range tombstones toward the limit, as if  
> they were regular tombstones - arguably not so much.  
>  
> —  
> AY  
>  
> On 5 January 2018 at 18:53:45, Jon Haddad (j...@jonhaddad.com) wrote:  
>  
> I think it’s reasonable to count the number of range tombstones towards  
> the total tombstone count / threshold.  
>  
> I agree the number of rows shadowed by the tombstones should be tracked  
> separately, and we should probably think a bit more about how to add  
> configuration / limits around this without burdening the user with a bunch  
> of new flags they have to think about. I would prefer to avoid any more  
> configuration settings as complex as back_pressure_strategy going forward.  
>  
> > On Jan 5, 2018, at 9:36 AM, Alexander Dejanovski   
> wrote:  
> >  
> > Hi Aleksey,  
> >  
> > ok we'll split the work and only deal with row level tombstones in  
> > CASSANDRA-8527   
> and  
> > create a follow up ticket to work on the separate counts you're  
> suggesting.  
> > My understanding of what you say is that you would not include range  
> > tombstones in the warn/fail threshold, but row level tombstones have  
> > somewhat an impact that is similar to cell tombstones. They will be  
> > retained in memory and will be sent to replicas.  
> > If we don't count them in the thresholds (at least for warnings), people  
> > will miss the fact that they may be reading a lot of tombstones.  
> > Are you ok with including those row tombstones as part of the thresholds  
> ?  
> > This was the original intent for creating this ticket, which was a  
> > follow-up to CASSANDRA-8477  
> > .  
> >  
> > For the follow up ticket, we may want to move the discussion in JIRA once  
> > we've create the ticket, but a merge listener seems like the right way to  
> > detect rows shadowed by range tombstones. That would force to change the  
> > UnfilteredRowIterator interface to include the tombstones/rows/cells  
> stats  
> > as this is what is returned from the lower levels of the read path.  
> > Is there any easier way to achieve this that you can think of, as that  
> > interface is used in many parts of the code ?  
> >  
> > On Wed, Dec 27, 2017 at 1:35 PM Aleksey Yeshchenko   
> > wrote:  
> >  
> >> As Jeff says, the number of actual tombstones is no less relevant than  
> the  
> >> number of  
> >> cells and rows shadowed by them. And the way row and cell tombstones  
> >> affect a read  
> >> query can be very different from the way a big range tombstone might:  
> you  
> >> potentially  
> >> have to retain every (regular) tombstone in memory and have replicas  
> ship  
> >> them to  
> >> a coordinator, while you can discard everything shadowed by a big RT and  
> >> only serialize  
> >> a tiny bit of the data between the replicas and the coordinator.  
> >>  
> >> So a mixed metric that just mixes up rows and cells shadowed by all  
> three  
> >> kinds of tombstones  
> >> without any differentiation, while better than not having that  
> visibility  
> >> at all, is worse than having  
> >> a detailed rundown. If possible, I’d love to see proper separate counts  
> >> tracked: range, row, and single-cell tombstones encountered, and # of  
> rows  
> >> or cells obsoleted by each type.  
> >>  
> >> I know that this is a non-trivial change, however, but hey, it doesn’t  
> >> have to be a trivial patch if it’s going into 4.0.  
> >>  
> >> In the meantime I think it’d be helpful to report that single count.  
> But I  
> >> don’t like the idea of redefining what  
> >> tombstone_warn_threshold and tombstone_failure_threshold mean, even in a  
> >> major release, as RTs are  
> >> qualitatively different from other tombstones, and have a much lower  
> >> impact per dead row.  
> >>  
> >> —  
> >> AY  
> >>  
> >> On 22 December 2017 at 03:53:47, kurt greaves (k...@instaclustr.com)  
> >> wrote:  
> >>  
> >> I think that's a good idea for 4.x, but not so for current branches. I  
> >> think as long as we document it well for 4.0 

Re: CASSANDRA-8527

[Aleksey Yeshchenko]

Counting the number of range tombstones - sure, that is reasonable.

Counting cells/rows shadowed by range tombstones toward the limit, as if they 
were regular tombstones - arguably not so much.

—
AY

On 5 January 2018 at 18:53:45, Jon Haddad (j...@jonhaddad.com) wrote:

I think it’s reasonable to count the number of range tombstones towards the 
total tombstone count / threshold.  

I agree the number of rows shadowed by the tombstones should be tracked 
separately, and we should probably think a bit more about how to add 
configuration / limits around this without burdening the user with a bunch of 
new flags they have to think about. I would prefer to avoid any more 
configuration settings as complex as back_pressure_strategy going forward.  

> On Jan 5, 2018, at 9:36 AM, Alexander Dejanovski  
> wrote:  
>  
> Hi Aleksey,  
>  
> ok we'll split the work and only deal with row level tombstones in  
> CASSANDRA-8527  and  
> create a follow up ticket to work on the separate counts you're suggesting.  
> My understanding of what you say is that you would not include range  
> tombstones in the warn/fail threshold, but row level tombstones have  
> somewhat an impact that is similar to cell tombstones. They will be  
> retained in memory and will be sent to replicas.  
> If we don't count them in the thresholds (at least for warnings), people  
> will miss the fact that they may be reading a lot of tombstones.  
> Are you ok with including those row tombstones as part of the thresholds ?  
> This was the original intent for creating this ticket, which was a  
> follow-up to CASSANDRA-8477  
> .  
>  
> For the follow up ticket, we may want to move the discussion in JIRA once  
> we've create the ticket, but a merge listener seems like the right way to  
> detect rows shadowed by range tombstones. That would force to change the  
> UnfilteredRowIterator interface to include the tombstones/rows/cells stats  
> as this is what is returned from the lower levels of the read path.  
> Is there any easier way to achieve this that you can think of, as that  
> interface is used in many parts of the code ?  
>  
> On Wed, Dec 27, 2017 at 1:35 PM Aleksey Yeshchenko   
> wrote:  
>  
>> As Jeff says, the number of actual tombstones is no less relevant than the  
>> number of  
>> cells and rows shadowed by them. And the way row and cell tombstones  
>> affect a read  
>> query can be very different from the way a big range tombstone might: you  
>> potentially  
>> have to retain every (regular) tombstone in memory and have replicas ship  
>> them to  
>> a coordinator, while you can discard everything shadowed by a big RT and  
>> only serialize  
>> a tiny bit of the data between the replicas and the coordinator.  
>>  
>> So a mixed metric that just mixes up rows and cells shadowed by all three  
>> kinds of tombstones  
>> without any differentiation, while better than not having that visibility  
>> at all, is worse than having  
>> a detailed rundown. If possible, I’d love to see proper separate counts  
>> tracked: range, row, and single-cell tombstones encountered, and # of rows  
>> or cells obsoleted by each type.  
>>  
>> I know that this is a non-trivial change, however, but hey, it doesn’t  
>> have to be a trivial patch if it’s going into 4.0.  
>>  
>> In the meantime I think it’d be helpful to report that single count. But I  
>> don’t like the idea of redefining what  
>> tombstone_warn_threshold and tombstone_failure_threshold mean, even in a  
>> major release, as RTs are  
>> qualitatively different from other tombstones, and have a much lower  
>> impact per dead row.  
>>  
>> —  
>> AY  
>>  
>> On 22 December 2017 at 03:53:47, kurt greaves (k...@instaclustr.com)  
>> wrote:  
>>  
>> I think that's a good idea for 4.x, but not so for current branches. I  
>> think as long as we document it well for 4.0 upgrades it's not so much of a  
>> problem. Obviously there will be cases of queries failing that were  
>> previously succeeding but we can already use  
>> tombstone_failure|warn_threshold to tune around that already. I don't think  
>> we need another yaml setting to enable/disable counting deleted rows for  
>> these thresholds, especially because it's going into 4.0. It *might* be a  
>> good idea to bump the tombstone failure threshold default as a safety net  
>> though (as well as put something in the NEWS.txt).  
>>  
>> On 21 December 2017 at 20:11, Jon Haddad  wrote:  
>>  
>>> I had suggested to Alex we kick this discussion over to the ML because  
>> the  
>>> change will have a significant impact on the behavior of Cassandra when  
>>> doing reads with range tombstones that cover a lot of rows. The behavior  
>>> now is a little weird, a single tombstone could shadow hundreds of  
>>> thousands or even 

Re: CASSANDRA-8527

[Jon Haddad]

I think it’s reasonable to count the number of range tombstones towards the 
total tombstone count / threshold.  

I agree the number of rows shadowed by the tombstones should be tracked 
separately, and we should probably think a bit more about how to add 
configuration / limits around this without burdening the user with a bunch of 
new flags they have to think about.  I would prefer to avoid any more 
configuration settings as complex as back_pressure_strategy going forward.

> On Jan 5, 2018, at 9:36 AM, Alexander Dejanovski  
> wrote:
> 
> Hi Aleksey,
> 
> ok we'll split the work and only deal with row level tombstones in
> CASSANDRA-8527  and
> create a follow up ticket to work on the separate counts you're suggesting.
> My understanding of what you say is that you would not include range
> tombstones in the warn/fail threshold, but row level tombstones have
> somewhat an impact that is similar to cell tombstones. They will be
> retained in memory and will be sent to replicas.
> If we don't count them in the thresholds (at least for warnings), people
> will miss the fact that they may be reading a lot of tombstones.
> Are you ok with including those row tombstones as part of the thresholds ?
> This was the original intent for creating this ticket, which was a
> follow-up to CASSANDRA-8477
> .
> 
> For the follow up ticket, we may want to move the discussion in JIRA once
> we've create the ticket, but a merge listener seems like the right way to
> detect rows shadowed by range tombstones. That would force to change the
> UnfilteredRowIterator interface to include the tombstones/rows/cells stats
> as this is what is returned from the lower levels of the read path.
> Is there any easier way to achieve this that you can think of, as that
> interface is used in many parts of the code ?
> 
> On Wed, Dec 27, 2017 at 1:35 PM Aleksey Yeshchenko 
> wrote:
> 
>> As Jeff says, the number of actual tombstones is no less relevant than the
>> number of
>> cells and rows shadowed by them. And the way row and cell tombstones
>> affect a read
>> query can be very different from the way a big range tombstone might: you
>> potentially
>> have to retain every (regular) tombstone in memory and have replicas ship
>> them to
>> a coordinator, while you can discard everything shadowed by a big RT and
>> only serialize
>> a tiny bit of the data between the replicas and the coordinator.
>> 
>> So a mixed metric that just mixes up rows and cells shadowed by all three
>> kinds of tombstones
>> without any differentiation, while better than not having that visibility
>> at all, is worse than having
>> a detailed rundown. If possible, I’d love to see proper separate counts
>> tracked: range, row, and single-cell tombstones encountered, and # of rows
>> or cells obsoleted by each type.
>> 
>> I know that this is a non-trivial change, however, but hey, it doesn’t
>> have to be a trivial patch if it’s going into 4.0.
>> 
>> In the meantime I think it’d be helpful to report that single count. But I
>> don’t like the idea of redefining what
>> tombstone_warn_threshold and tombstone_failure_threshold mean, even in a
>> major release, as RTs are
>> qualitatively different from other tombstones, and have a much lower
>> impact per dead row.
>> 
>> —
>> AY
>> 
>> On 22 December 2017 at 03:53:47, kurt greaves (k...@instaclustr.com)
>> wrote:
>> 
>> I think that's a good idea for 4.x, but not so for current branches. I
>> think as long as we document it well for 4.0 upgrades it's not so much of a
>> problem. Obviously there will be cases of queries failing that were
>> previously succeeding but we can already use
>> tombstone_failure|warn_threshold to tune around that already. I don't think
>> we need another yaml setting to enable/disable counting deleted rows for
>> these thresholds, especially because it's going into 4.0. It *might* be a
>> good idea to bump the tombstone failure threshold default as a safety net
>> though (as well as put something in the NEWS.txt).
>> 
>> On 21 December 2017 at 20:11, Jon Haddad  wrote:
>> 
>>> I had suggested to Alex we kick this discussion over to the ML because
>> the
>>> change will have a significant impact on the behavior of Cassandra when
>>> doing reads with range tombstones that cover a lot of rows. The behavior
>>> now is a little weird, a single tombstone could shadow hundreds of
>>> thousands or even millions of rows, and the query would probably just
>> time
>>> out. Personally, I’m in favor of the change in behavior of this patch but
>>> I wanted to get some more feedback before committing to it. Are there any
>>> objections to what Alex described?
>>> 
>>> Regarding Mockito, I’ve been meaning to bring this up for a while, and
>> I’m
>>> a solid +1 on introducing it to help with testing. In an ideal world we’d
>>> have no 

Re: CASSANDRA-8527

[Aleksey Yeshchenko]

As Jeff says, the number of actual tombstones is no less relevant than the 
number of
cells and rows shadowed by them. And the way row and cell tombstones affect a 
read
query can be very different from the way a big range tombstone might: you 
potentially
have to retain every (regular) tombstone in memory and have replicas ship them 
to
a coordinator, while you can discard everything shadowed by a big RT and only 
serialize
a tiny bit of the data between the replicas and the coordinator. 

So a mixed metric that just mixes up rows and cells shadowed by all three kinds 
of tombstones
without any differentiation, while better than not having that visibility at 
all, is worse than having
a detailed rundown. If possible, I’d love to see proper separate counts
tracked: range, row, and single-cell tombstones encountered, and # of rows or 
cells obsoleted by each type.

I know that this is a non-trivial change, however, but hey, it doesn’t have to 
be a trivial patch if it’s going into 4.0.

In the meantime I think it’d be helpful to report that single count. But I 
don’t like the idea of redefining what 
tombstone_warn_threshold and tombstone_failure_threshold mean, even in a major 
release, as RTs are
qualitatively different from other tombstones, and have a much lower impact per 
dead row.

—
AY

On 22 December 2017 at 03:53:47, kurt greaves (k...@instaclustr.com) wrote:

I think that's a good idea for 4.x, but not so for current branches. I  
think as long as we document it well for 4.0 upgrades it's not so much of a  
problem. Obviously there will be cases of queries failing that were  
previously succeeding but we can already use  
tombstone_failure|warn_threshold to tune around that already. I don't think  
we need another yaml setting to enable/disable counting deleted rows for  
these thresholds, especially because it's going into 4.0. It *might* be a  
good idea to bump the tombstone failure threshold default as a safety net  
though (as well as put something in the NEWS.txt).  

On 21 December 2017 at 20:11, Jon Haddad  wrote:  

> I had suggested to Alex we kick this discussion over to the ML because the  
> change will have a significant impact on the behavior of Cassandra when  
> doing reads with range tombstones that cover a lot of rows. The behavior  
> now is a little weird, a single tombstone could shadow hundreds of  
> thousands or even millions of rows, and the query would probably just time  
> out. Personally, I’m in favor of the change in behavior of this patch but  
> I wanted to get some more feedback before committing to it. Are there any  
> objections to what Alex described?  
>  
> Regarding Mockito, I’ve been meaning to bring this up for a while, and I’m  
> a solid +1 on introducing it to help with testing. In an ideal world we’d  
> have no singletons and could test everything in isolation, but  
> realistically that’s a multi year process and we just aren’t there.  
>  
>  
> > On Dec 19, 2017, at 11:07 PM, Alexander Dejanovski <  
> a...@thelastpickle.com> wrote:  
> >  
> > Hi folks,  
> >  
> > I'm working on CASSANDRA-8527  
> >  and would need to  
> > discuss a few things.  
> >  
> > The ticket makes it visible in tracing and metrics that rows shadowed by  
> > range tombstones were scanned during reads.  
> > Currently, scanned range tombstones aren't reported anywhere which hides  
> > the cause of performance issues during reads when the users perform  
> primary  
> > key deletes.  
> > As such, they do not count in the warn and failure thresholds.  
> >  
> > While the number of live rows and tombstone cells is counted in the  
> > ReadCommand class, it is currently not possible to count the number of  
> > range tombstones there as they are merged with the rows they shadow  
> before  
> > reaching the class.  
> > Instead, we can count the number of deleted rows that were read , which  
> > already improves diagnosis and show that range tombstones were scanned :  
> >  
> > if (row.hasLiveData(ReadCommand.this.nowInSec(), enforceStrictLiveness))  
> > ++liveRows;  
> > else if (!row.primaryKeyLivenessInfo().isLive(ReadCommand.this.  
> nowInSec()))  
> > {  
> > // We want to detect primary key deletions only.  
> > // If all cells have expired they will count as tombstones.  
> > ++deletedRows;  
> > }  
> >  
> > Deleted rows would be part of the warning threshold so that we can spot  
> the  
> > range tombstone scans in the logs and tracing would look like this :  
> >  
> > 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..  
> >  
> >  
> > Are there any caveats to that approach ?  
> > Should we include the number of deleted rows in the failure threshold or  
> > make it optional, knowing that it could make some queries fail while they  
> > were passing before ?  
> >  
> > On a side 

Re: CASSANDRA-8527

[Alexander Dejanovski]

Hi folks,

thanks for the feedback so far.

@Jeff, there are two distinct cases here :

   1. The range tombstones created on a partial primary key (that doesn't
   include the last column of the PK for example) : a single tombstone can
   shadow many rows
   2. The range tombstones created on the full PK : a single tombstone can
   shadow a single row only

In the first case, the range tombstones are (almost) correctly counted
(after merge since that happens really early in the code). We cannot know
how many shadowed rows/cells were read because they get merged early with
the tombstones.

In the second case (full PK delete), there is no tombstone counted and we
only get a count of the live rows after merge.

I'll illustrate this with the example below :

CREATE TABLE users.test (id int, clust1 text, clust2 text, val1 text, val2
text, PRIMARY KEY(id, clust1, clust2));
insert into users.test(id , clust1, clust2 , val1 , val2)
values(1,'c1','cc1', 'v1','v2');
insert into users.test(id , clust1, clust2 , val1 , val2)
values(1,'c1','cc2', 'v1','v2');
insert into users.test(id , clust1, clust2 , val1 , val2)
values(1,'c1','cc3', 'v1','v2');
insert into users.test(id , clust1, clust2 , val1 , val2)
values(1,'c2','cc1', 'v1','v2');
insert into users.test(id , clust1, clust2 , val1 , val2)
values(1,'c2','cc2', 'v1','v2');
insert into users.test(id , clust1, clust2 , val1 , val2)
values(1,'c2','cc3', 'v1','v2');

cqlsh> select * from users.test
   ... ;

 id | clust1 | clust2 | val1 | val2
+++--+--
  1 | c1 |cc1 |   v1 |   v2
  1 | c1 |cc2 |   v1 |   v2
  1 | c1 |cc3 |   v1 |   v2
  1 | c2 |cc1 |   v1 |   v2
  1 | c2 |cc2 |   v1 |   v2
  1 | c2 |cc3 |   v1 |   v2


Tracing session: 4c9804c0-e73a-11e7-931f-517010c60cf9

 activity
   | timestamp
| source| source_elapsed | client
--++---++---
...
Read 6 live rows, 0 deleted rows and 0 tombstone cells [ReadStage-1] |
2017-12-22 18:12:55.954000 | 127.0.0.1 |   5567 | 127.0.0.2

Then I issue a range tombstone on id = 1 and clust1 =  'c1' :

cqlsh> delete from users.test where id = 1 and clust1 = 'c1';
cqlsh> select * from users.test;

 id | clust1 | clust2 | val1 | val2
+++--+--
  1 | c2 |cc1 |   v1 |   v2
  1 | c2 |cc2 |   v1 |   v2
  1 | c2 |cc3 |   v1 |   v2

Tracing session: 8597e320-e73b-11e7-931f-517010c60cf9

 activity
| timestamp
  | source| source_elapsed | client
---++---++---
...
Read 3 live rows, 0 deleted rows and 2 tombstone cells [ReadStage-1] |
2017-12-22 18:14:08.855000 | 127.0.0.1 |   2878 | 127.0.0.2

Each range tombstone apparently counts for 2 tombstone cells (probably due
the the start bound marker and the end bound marker ?).

Then if I just delete a single row :

cqlsh> delete from users.test where id = 1 and clust1 = 'c2' and clust2 =
'cc1';
cqlsh> select * from users.test;

 id | clust1 | clust2 | val1 | val2
+++--+--
  1 | c2 |cc2 |   v1 |   v2
  1 | c2 |cc3 |   v1 |   v2

  Tracing session: b43d9170-e73b-11e7-931f-517010c60cf9

 activity
| timestamp
  | source| source_elapsed | client
---++---++---
...
  Read 2 live rows, 1
deleted rows and 2 tombstone cells [ReadStage-1] | 2017-12-22
18:15:27.117000 | 127.0.0.1 |   4487 | 127.0.0.2
...

My patch is applied here so the deleted row appears in the new counter, but
the tombstone cell count is unchanged (I haven't touched the way they are
counted).

It seems like only PK tombstones are the ones that are currently impossible
to notice. Are they even considered as range tombstones ? I've failed to
identify them as such while debugging the code.

We still cannot know how many tombstones or non live cells we're really
reading from disk due to early merging. What we get in the ReadCommand
class is expectedly pretty different from what Cassandra reads from
disk/memory.

@Kurt : I'd be in favor of not adding a new setting in the yaml file. I
think it's better for folks to realize early that they're reading a lot of
deleted rows. Makes sense to do this only in 4.x.

@DuyHai : what would you consider a non wise use of 

Re: CASSANDRA-8527

[kurt greaves]

I think that's a good idea for 4.x, but not so for current branches. I
think as long as we document it well for 4.0 upgrades it's not so much of a
problem. Obviously there will be cases of queries failing that were
previously succeeding but we can already use
tombstone_failure|warn_threshold to tune around that already. I don't think
we need another yaml setting to enable/disable counting deleted rows for
these thresholds, especially because it's going into 4.0. It *might* be a
good idea to bump the tombstone failure threshold default as a safety net
though (as well as put something in the NEWS.txt).

On 21 December 2017 at 20:11, Jon Haddad  wrote:

> I had suggested to Alex we kick this discussion over to the ML because the
> change will have a significant impact on the behavior of Cassandra when
> doing reads with range tombstones that cover a lot of rows.  The behavior
> now is a little weird, a single tombstone could shadow hundreds of
> thousands or even millions of rows, and the query would probably just time
> out.  Personally, I’m in favor of the change in behavior of this patch but
> I wanted to get some more feedback before committing to it.  Are there any
> objections to what Alex described?
>
> Regarding Mockito, I’ve been meaning to bring this up for a while, and I’m
> a solid +1 on introducing it to help with testing.  In an ideal world we’d
> have no singletons and could test everything in isolation, but
> realistically that’s a multi year process and we just aren’t there.
>
>
> > On Dec 19, 2017, at 11:07 PM, Alexander Dejanovski <
> a...@thelastpickle.com> wrote:
> >
> > Hi folks,
> >
> > I'm working on CASSANDRA-8527
> >  and would need to
> > discuss a few things.
> >
> > The ticket makes it visible in tracing and metrics that rows shadowed by
> > range tombstones were scanned during reads.
> > Currently, scanned range tombstones aren't reported anywhere which hides
> > the cause of performance issues during reads when the users perform
> primary
> > key deletes.
> > As such, they do not count in the warn and failure thresholds.
> >
> > While the number of live rows and tombstone cells is counted in the
> > ReadCommand class, it is currently not possible to count the number of
> > range tombstones there as they are merged with the rows they shadow
> before
> > reaching the class.
> > Instead, we can count the number of deleted rows that were read , which
> > already improves diagnosis and show that range tombstones were scanned :
> >
> > if (row.hasLiveData(ReadCommand.this.nowInSec(), enforceStrictLiveness))
> >++liveRows;
> > else if (!row.primaryKeyLivenessInfo().isLive(ReadCommand.this.
> nowInSec()))
> > {
> >// We want to detect primary key deletions only.
> >// If all cells have expired they will count as tombstones.
> >   ++deletedRows;
> > }
> >
> > Deleted rows would be part of the warning threshold so that we can spot
> the
> > range tombstone scans in the logs and tracing would look like this :
> >
> > 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..
> >
> >
> > Are there any caveats to that approach ?
> > Should we include the number of deleted rows in the failure threshold or
> > make it optional, knowing that it could make some queries fail while they
> > were passing before ?
> >
> > On a side note, is it ok to bring in Mockito in order to make it easier
> > writing tests ? I would like to use a Spy in order to write some tests
> > without starting the whole stack.
> >
> > Thanks,
> >
> >
> > --
> > -
> > Alexander Dejanovski
> > France
> > @alexanderdeja
> >
> > Consultant
> > Apache Cassandra Consulting
> > http://www.thelastpickle.com
>
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@cassandra.apache.org
> For additional commands, e-mail: dev-h...@cassandra.apache.org
>
>

Re: CASSANDRA-8527

[Qingcun Zhou]

Maybe I should rephrase my words but I think you got that thread. Singleton
is one big thing and possibly we can use powermock, or totally get rid of
it. But without mock, I'm frustrated to work on so-called unit test cases.

On Thu, Dec 21, 2017 at 2:28 PM, Jeff Jirsa  wrote:

> On Thu, Dec 21, 2017 at 12:33 PM, Qingcun Zhou 
> wrote:
>
> > I brought up Mockito topic couple of months ago but some committers don't
> > think we should introduce more dependencies.
> >
> >
> I don't see any indication of this in the dev@ archives. There was a
> comment from a contributor who said they weren't huge fans, but it wasn't a
> binding -1, and the biggest issue has always been the singletons.
>
> It's possible I missed it with my search, or there may be a few people who
> aren't anxious to add new things, but the singletons are the more glaring
> issue. Not everything CAN be mocked right now, and the conversation seems
> to have steered in that direction last time this happened.
>
> I don't personally care if mockito gets incorporated, but I see no vote
> that indicates we've decided as a project to stop developers from using
> it.
>



-- 
Thank you & Best Regards,
--Simon (Qingcun) Zhou

Re: CASSANDRA-8527

[Jeff Jirsa]

On Thu, Dec 21, 2017 at 12:33 PM, Qingcun Zhou 
wrote:

> I brought up Mockito topic couple of months ago but some committers don't
> think we should introduce more dependencies.
>
>
I don't see any indication of this in the dev@ archives. There was a
comment from a contributor who said they weren't huge fans, but it wasn't a
binding -1, and the biggest issue has always been the singletons.

It's possible I missed it with my search, or there may be a few people who
aren't anxious to add new things, but the singletons are the more glaring
issue. Not everything CAN be mocked right now, and the conversation seems
to have steered in that direction last time this happened.

I don't personally care if mockito gets incorporated, but I see no vote
that indicates we've decided as a project to stop developers from using
it.

Re: CASSANDRA-8527

[Jeff Jirsa]

On Tue, Dec 19, 2017 at 11:07 PM, Alexander Dejanovski <
a...@thelastpickle.com> wrote:

> Hi folks,
>
> I'm working on CASSANDRA-8527
>  and would need to
> discuss a few things.
>
> The ticket makes it visible in tracing and metrics that rows shadowed by
> range tombstones were scanned during reads.
> Currently, scanned range tombstones aren't reported anywhere which hides
> the cause of performance issues during reads when the users perform primary
> key deletes.
> As such, they do not count in the warn and failure thresholds.
>
> While the number of live rows and tombstone cells is counted in the
> ReadCommand class, it is currently not possible to count the number of
> range tombstones there as they are merged with the rows they shadow before
> reaching the class.
> Instead, we can count the number of deleted rows that were read , which
> already improves diagnosis and show that range tombstones were scanned :
>
> if (row.hasLiveData(ReadCommand.this.nowInSec(), enforceStrictLiveness))
> ++liveRows;
> else if (!row.primaryKeyLivenessInfo().isLive(ReadCommand.this.
> nowInSec()))
> {
> // We want to detect primary key deletions only.
> // If all cells have expired they will count as tombstones.
>++deletedRows;
> }
>
> Deleted rows would be part of the warning threshold so that we can spot the
> range tombstone scans in the logs and tracing would look like this :
>
> 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..
>
>
> Are there any caveats to that approach ?
> Should we include the number of deleted rows in the failure threshold or
> make it optional, knowing that it could make some queries fail while they
> were passing before ?
>
>

Thanks for sending the email. Unfortunately, it's a horrible time of year
to get much feedback. This is the type of question we should give folks a
chance to answer before rushing to merge.

I'm not personally sure of the answer without re-reading that code, and
it's the week before Christmas, so it's not going to happen until the new
year.

I wouldn't mind seeing the deleted rows in the log, but I suspect the
number of tombstones matters more than the number of deleted rows - 1 RT
that shadows a thousand deleted rows is very different than a thousand RTs
shadowing a thousand rows. If we end up with something only marginally
useful, we should make sure the changes to the hot path justify that
marginal gain.

Re: CASSANDRA-8527

[Jon Haddad]

The question isn’t so much about reporting them (we should), it’s about the 
behavior of tombstone_warn_threshold and tombstone_failure_threshold.  The 
patch changes the behavior to include the number of rows that are passed over 
due to the range tombstones.  We’re interested in feedback on if it makes sense 
to change the current behavior.  I’m a +.5 on the change, it makes sense to me, 
but am wondering if there’s a case we haven’t considered.  At the very least 
we’d need a NEWS entry since it is a behavior change.


> On Dec 21, 2017, at 12:33 PM, DuyHai Doan  wrote:
> 
> +1 to report range tombstones. This one is quite tricky indeed to track
> 
> +1 to Mockito too, with the reserve that it should be used wisely
> 
> On Thu, Dec 21, 2017 at 9:11 PM, Jon Haddad  wrote:
> 
>> I had suggested to Alex we kick this discussion over to the ML because the
>> change will have a significant impact on the behavior of Cassandra when
>> doing reads with range tombstones that cover a lot of rows.  The behavior
>> now is a little weird, a single tombstone could shadow hundreds of
>> thousands or even millions of rows, and the query would probably just time
>> out.  Personally, I’m in favor of the change in behavior of this patch but
>> I wanted to get some more feedback before committing to it.  Are there any
>> objections to what Alex described?
>> 
>> Regarding Mockito, I’ve been meaning to bring this up for a while, and I’m
>> a solid +1 on introducing it to help with testing.  In an ideal world we’d
>> have no singletons and could test everything in isolation, but
>> realistically that’s a multi year process and we just aren’t there.
>> 
>> 
>>> On Dec 19, 2017, at 11:07 PM, Alexander Dejanovski <
>> a...@thelastpickle.com> wrote:
>>> 
>>> Hi folks,
>>> 
>>> I'm working on CASSANDRA-8527
>>>  and would need to
>>> discuss a few things.
>>> 
>>> The ticket makes it visible in tracing and metrics that rows shadowed by
>>> range tombstones were scanned during reads.
>>> Currently, scanned range tombstones aren't reported anywhere which hides
>>> the cause of performance issues during reads when the users perform
>> primary
>>> key deletes.
>>> As such, they do not count in the warn and failure thresholds.
>>> 
>>> While the number of live rows and tombstone cells is counted in the
>>> ReadCommand class, it is currently not possible to count the number of
>>> range tombstones there as they are merged with the rows they shadow
>> before
>>> reaching the class.
>>> Instead, we can count the number of deleted rows that were read , which
>>> already improves diagnosis and show that range tombstones were scanned :
>>> 
>>> if (row.hasLiveData(ReadCommand.this.nowInSec(), enforceStrictLiveness))
>>>   ++liveRows;
>>> else if (!row.primaryKeyLivenessInfo().isLive(ReadCommand.this.
>> nowInSec()))
>>> {
>>>   // We want to detect primary key deletions only.
>>>   // If all cells have expired they will count as tombstones.
>>>  ++deletedRows;
>>> }
>>> 
>>> Deleted rows would be part of the warning threshold so that we can spot
>> the
>>> range tombstone scans in the logs and tracing would look like this :
>>> 
>>> 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..
>>> 
>>> 
>>> Are there any caveats to that approach ?
>>> Should we include the number of deleted rows in the failure threshold or
>>> make it optional, knowing that it could make some queries fail while they
>>> were passing before ?
>>> 
>>> On a side note, is it ok to bring in Mockito in order to make it easier
>>> writing tests ? I would like to use a Spy in order to write some tests
>>> without starting the whole stack.
>>> 
>>> Thanks,
>>> 
>>> 
>>> --
>>> -
>>> Alexander Dejanovski
>>> France
>>> @alexanderdeja
>>> 
>>> Consultant
>>> Apache Cassandra Consulting
>>> http://www.thelastpickle.com
>> 
>> 
>> -
>> To unsubscribe, e-mail: dev-unsubscr...@cassandra.apache.org
>> For additional commands, e-mail: dev-h...@cassandra.apache.org
>> 
>> 


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

Re: CASSANDRA-8527

[DuyHai Doan]

+1 to report range tombstones. This one is quite tricky indeed to track

+1 to Mockito too, with the reserve that it should be used wisely

On Thu, Dec 21, 2017 at 9:11 PM, Jon Haddad  wrote:

> I had suggested to Alex we kick this discussion over to the ML because the
> change will have a significant impact on the behavior of Cassandra when
> doing reads with range tombstones that cover a lot of rows.  The behavior
> now is a little weird, a single tombstone could shadow hundreds of
> thousands or even millions of rows, and the query would probably just time
> out.  Personally, I’m in favor of the change in behavior of this patch but
> I wanted to get some more feedback before committing to it.  Are there any
> objections to what Alex described?
>
> Regarding Mockito, I’ve been meaning to bring this up for a while, and I’m
> a solid +1 on introducing it to help with testing.  In an ideal world we’d
> have no singletons and could test everything in isolation, but
> realistically that’s a multi year process and we just aren’t there.
>
>
> > On Dec 19, 2017, at 11:07 PM, Alexander Dejanovski <
> a...@thelastpickle.com> wrote:
> >
> > Hi folks,
> >
> > I'm working on CASSANDRA-8527
> >  and would need to
> > discuss a few things.
> >
> > The ticket makes it visible in tracing and metrics that rows shadowed by
> > range tombstones were scanned during reads.
> > Currently, scanned range tombstones aren't reported anywhere which hides
> > the cause of performance issues during reads when the users perform
> primary
> > key deletes.
> > As such, they do not count in the warn and failure thresholds.
> >
> > While the number of live rows and tombstone cells is counted in the
> > ReadCommand class, it is currently not possible to count the number of
> > range tombstones there as they are merged with the rows they shadow
> before
> > reaching the class.
> > Instead, we can count the number of deleted rows that were read , which
> > already improves diagnosis and show that range tombstones were scanned :
> >
> > if (row.hasLiveData(ReadCommand.this.nowInSec(), enforceStrictLiveness))
> >++liveRows;
> > else if (!row.primaryKeyLivenessInfo().isLive(ReadCommand.this.
> nowInSec()))
> > {
> >// We want to detect primary key deletions only.
> >// If all cells have expired they will count as tombstones.
> >   ++deletedRows;
> > }
> >
> > Deleted rows would be part of the warning threshold so that we can spot
> the
> > range tombstone scans in the logs and tracing would look like this :
> >
> > 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..
> >
> >
> > Are there any caveats to that approach ?
> > Should we include the number of deleted rows in the failure threshold or
> > make it optional, knowing that it could make some queries fail while they
> > were passing before ?
> >
> > On a side note, is it ok to bring in Mockito in order to make it easier
> > writing tests ? I would like to use a Spy in order to write some tests
> > without starting the whole stack.
> >
> > Thanks,
> >
> >
> > --
> > -
> > Alexander Dejanovski
> > France
> > @alexanderdeja
> >
> > Consultant
> > Apache Cassandra Consulting
> > http://www.thelastpickle.com
>
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@cassandra.apache.org
> For additional commands, e-mail: dev-h...@cassandra.apache.org
>
>

Re: CASSANDRA-8527

[Jon Haddad]

I had suggested to Alex we kick this discussion over to the ML because the 
change will have a significant impact on the behavior of Cassandra when doing 
reads with range tombstones that cover a lot of rows.  The behavior now is a 
little weird, a single tombstone could shadow hundreds of thousands or even 
millions of rows, and the query would probably just time out.  Personally, I’m 
in favor of the change in behavior of this patch but I wanted to get some more 
feedback before committing to it.  Are there any objections to what Alex 
described?  

Regarding Mockito, I’ve been meaning to bring this up for a while, and I’m a 
solid +1 on introducing it to help with testing.  In an ideal world we’d have 
no singletons and could test everything in isolation, but realistically that’s 
a multi year process and we just aren’t there.  


> On Dec 19, 2017, at 11:07 PM, Alexander Dejanovski  
> wrote:
> 
> Hi folks,
> 
> I'm working on CASSANDRA-8527
>  and would need to
> discuss a few things.
> 
> The ticket makes it visible in tracing and metrics that rows shadowed by
> range tombstones were scanned during reads.
> Currently, scanned range tombstones aren't reported anywhere which hides
> the cause of performance issues during reads when the users perform primary
> key deletes.
> As such, they do not count in the warn and failure thresholds.
> 
> While the number of live rows and tombstone cells is counted in the
> ReadCommand class, it is currently not possible to count the number of
> range tombstones there as they are merged with the rows they shadow before
> reaching the class.
> Instead, we can count the number of deleted rows that were read , which
> already improves diagnosis and show that range tombstones were scanned :
> 
> if (row.hasLiveData(ReadCommand.this.nowInSec(), enforceStrictLiveness))
>++liveRows;
> else if (!row.primaryKeyLivenessInfo().isLive(ReadCommand.this.nowInSec()))
> {
>// We want to detect primary key deletions only.
>// If all cells have expired they will count as tombstones.
>   ++deletedRows;
> }
> 
> Deleted rows would be part of the warning threshold so that we can spot the
> range tombstone scans in the logs and tracing would look like this :
> 
> 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..
> 
> 
> Are there any caveats to that approach ?
> Should we include the number of deleted rows in the failure threshold or
> make it optional, knowing that it could make some queries fail while they
> were passing before ?
> 
> On a side note, is it ok to bring in Mockito in order to make it easier
> writing tests ? I would like to use a Spy in order to write some tests
> without starting the whole stack.
> 
> Thanks,
> 
> 
> -- 
> -
> Alexander Dejanovski
> France
> @alexanderdeja
> 
> Consultant
> Apache Cassandra Consulting
> http://www.thelastpickle.com


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