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 > <https://issues.apache.org/jira/browse/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
