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 upgrades it's not so much  
> of a  
> >> problem. Obviously there w

Re: CASSANDRA-8527

[kurt greaves]

I think tombstone rows is more appropriate. Calling them deleted rows
initially confused me and it took a few reads of the problem for me to work
it out. Especially when you put it in context and "tombstone cells"
follows.​

Re: [Patch Available for Review!] CASSANDRA-14134: Migrate dtests to use pytest and python3

[Michael Kjellman]

hi!

a few of us have been continuously iterating on the dtest-on-pytest branch now 
since the 2nd and we’ve run the dtests close to 600 times in ci. ariel has been 
working his way thru a formal review (three cheers for ariel!)

flaky tests are a real thing and despite a few dozen totally green test runs, 
the vast majority of runs are still reliably hitting roughly 1-3 test failures. 
in a world where we can now run the dtests in 20 minutes instead of 13 hours 
it’s now at least possible to keep finding these flaky tests and fixing them 
one by one...

i haven’t gotten a huge amount of feedback overall and i really want to hear 
it! ultimately this work is driven by the desire to 1) have *all* our tests run 
on *every* commit; 2) be able to trust the results; 3) make our testing story 
so amazing that even the most casual weekend warrior who wants to work on the 
project can (and will want to!) use it.

i’m *not* a python guy (although lucky i know and work with many who are). 
thankfully i’ve been able to defer to them for much of this largely python 
based effort i’m sure there are a few more people working on the project 
who do consider themselves python experts and i’d especially appreciate your 
feedback!

finally, a lot of my effort was focused around improving the end users 
experience (getting bootstrapped, running the tests, improving the debugability 
story, etc). i’d really appreciate it if people could try running the pytest 
branch and following the install instructions to figure out what could be 
improved on. any existing behavior i’ve inadvertently now removed that’s going 
to make someone’s life miserable? 😅

thanks! looking forward to hearing any and all feedback from the community!

best,
kjellman



On Jan 3, 2018, at 8:08 AM, Michael Kjellman 
mailto:mkjell...@internalcircle.com>> wrote:

no, i’m not. i just figured i should target python 3.6 if i was doing this work 
in the first place. the current Ubuntu LTS was pulling in a pretty old version. 
any concerns with using 3.6?

On Jan 3, 2018, at 1:51 AM, Stefan Podkowinski 
mailto:s...@apache.org>> wrote:

The latest updates to your branch fixed the logging issue, thanks! Tests
now seem to execute fine locally using pytest.

I was looking at the dockerfile and noticed that you explicitly use
python 3.6 there. Are you aware of any issues with older python3
versions, e.g. 3.5? Do I have to use 3.6 as well locally and do we have
to do the same for jenkins?


On 02.01.2018 22:42, Michael Kjellman wrote:
I reproduced the NOTSET log issue locally... got a fix.. i'll push a commit up 
in a moment.

On Jan 2, 2018, at 11:24 AM, Michael Kjellman 
mailto:mkjell...@internalcircle.com>> wrote:

Comments Inline: Thanks for giving this a go!!

On Jan 2, 2018, at 6:10 AM, Stefan Podkowinski 
mailto:s...@apache.org>> wrote:

I was giving this a try today with some mixed results. First of all,
running pytest locally would fail with an "ccmlib.common.ArgumentError:
Unknown log level NOTSET" error for each test. Although I created a new
virtualenv for that as described in the readme (thanks for updating!)
and use both of your dtest and cassandra branches. But I haven't patched
ccm as described in the ticket, maybe that's why? Can you publish a
patched ccm branch to gh?

99% sure this is an issue parsing the logging level passed to pytest to the 
python logger... could you paste the exact command you're using to invoke 
pytest? should be a small change - i'm sure i just missed a invocation case.


The updated circle.yml is now using docker, which seems to be a good
idea to reduce clutter in the yaml file and gives us more control over
the test environment. Can you add the Dockerfile to the .circleci
directory as well? I couldn't find it when I was trying to solve the
pytest error mentioned above.

This is already tracked in a separate repo: 
https://github.com/mkjellman/cassandra-test-docker/blob/master/Dockerfile

Next thing I did was to push your trunk_circle branch to my gh repo to
start a circleCI run. Finishing all dtests in 15 minutes sounds
exciting, but requires a paid tier plan to get that kind of
parallelization. Looks like the dtests have even been deliberately
disabled for non-paid accounts, so I couldn't test this any further.

the plan of action (i already already mentioned this in previous emails) is to 
get dtests working for the free circieci oss accounts as well. part of this 
work (already included in this pytest effort) is to have fixtures that look at 
the system resources and dynamically include tests as possible.


Running dtests from the pytest branch on 
builds.apache.org did not work
either. At least the run_dtests.py arguments will need to be updated in
cassandra-builds. We currently only use a single cassandra-dtest.sh
script for all builds. Maybe we should create a new job template that
would use an updated script with the wip-pytest dtest branch, to make
this work and testable in parallel.

ye

Re: [Patch Available for Review!] CASSANDRA-14134: Migrate dtests to use pytest and python3

[Nate McCall]

Making these tests more accessible and reliable is super huge. There
are a lot of folks in our community who are not well versed with
python (myself included). I wholly support *any* efforts we can make
for the dtest process to be easy.

Thanks a bunch for taking this on. I think it will pay off quickly.

On Wed, Jan 10, 2018 at 4:55 PM, Michael Kjellman  wrote:
> hi!
>
> a few of us have been continuously iterating on the dtest-on-pytest branch 
> now since the 2nd and we’ve run the dtests close to 600 times in ci. ariel 
> has been working his way thru a formal review (three cheers for ariel!)
>
> flaky tests are a real thing and despite a few dozen totally green test runs, 
> the vast majority of runs are still reliably hitting roughly 1-3 test 
> failures. in a world where we can now run the dtests in 20 minutes instead of 
> 13 hours it’s now at least possible to keep finding these flaky tests and 
> fixing them one by one...
>
> i haven’t gotten a huge amount of feedback overall and i really want to hear 
> it! ultimately this work is driven by the desire to 1) have *all* our tests 
> run on *every* commit; 2) be able to trust the results; 3) make our testing 
> story so amazing that even the most casual weekend warrior who wants to work 
> on the project can (and will want to!) use it.
>
> i’m *not* a python guy (although lucky i know and work with many who are). 
> thankfully i’ve been able to defer to them for much of this largely python 
> based effort i’m sure there are a few more people working on the project 
> who do consider themselves python experts and i’d especially appreciate your 
> feedback!
>
> finally, a lot of my effort was focused around improving the end users 
> experience (getting bootstrapped, running the tests, improving the 
> debugability story, etc). i’d really appreciate it if people could try 
> running the pytest branch and following the install instructions to figure 
> out what could be improved on. any existing behavior i’ve inadvertently now 
> removed that’s going to make someone’s life miserable? 😅
>
> thanks! looking forward to hearing any and all feedback from the community!
>
> best,
> kjellman
>
>
>
> On Jan 3, 2018, at 8:08 AM, Michael Kjellman 
> mailto:mkjell...@internalcircle.com>> wrote:
>
> no, i’m not. i just figured i should target python 3.6 if i was doing this 
> work in the first place. the current Ubuntu LTS was pulling in a pretty old 
> version. any concerns with using 3.6?
>
> On Jan 3, 2018, at 1:51 AM, Stefan Podkowinski 
> mailto:s...@apache.org>> wrote:
>
> The latest updates to your branch fixed the logging issue, thanks! Tests
> now seem to execute fine locally using pytest.
>
> I was looking at the dockerfile and noticed that you explicitly use
> python 3.6 there. Are you aware of any issues with older python3
> versions, e.g. 3.5? Do I have to use 3.6 as well locally and do we have
> to do the same for jenkins?
>
>
> On 02.01.2018 22:42, Michael Kjellman wrote:
> I reproduced the NOTSET log issue locally... got a fix.. i'll push a commit 
> up in a moment.
>
> On Jan 2, 2018, at 11:24 AM, Michael Kjellman 
> mailto:mkjell...@internalcircle.com>> wrote:
>
> Comments Inline: Thanks for giving this a go!!
>
> On Jan 2, 2018, at 6:10 AM, Stefan Podkowinski 
> mailto:s...@apache.org>> wrote:
>
> I was giving this a try today with some mixed results. First of all,
> running pytest locally would fail with an "ccmlib.common.ArgumentError:
> Unknown log level NOTSET" error for each test. Although I created a new
> virtualenv for that as described in the readme (thanks for updating!)
> and use both of your dtest and cassandra branches. But I haven't patched
> ccm as described in the ticket, maybe that's why? Can you publish a
> patched ccm branch to gh?
>
> 99% sure this is an issue parsing the logging level passed to pytest to the 
> python logger... could you paste the exact command you're using to invoke 
> pytest? should be a small change - i'm sure i just missed a invocation case.
>
>
> The updated circle.yml is now using docker, which seems to be a good
> idea to reduce clutter in the yaml file and gives us more control over
> the test environment. Can you add the Dockerfile to the .circleci
> directory as well? I couldn't find it when I was trying to solve the
> pytest error mentioned above.
>
> This is already tracked in a separate repo: 
> https://github.com/mkjellman/cassandra-test-docker/blob/master/Dockerfile
>
> Next thing I did was to push your trunk_circle branch to my gh repo to
> start a circleCI run. Finishing all dtests in 15 minutes sounds
> exciting, but requires a paid tier plan to get that kind of
> parallelization. Looks like the dtests have even been deliberately
> disabled for non-paid accounts, so I couldn't test this any further.
>
> the plan of action (i already already mentioned this in previous emails) is 
> to get dtests working for the free circieci oss accounts as well. part of 
> this work (already included in