Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

roachtest: enable kv/gracefuldraining/nodes=3 #98720

Merged
merged 1 commit into from
May 9, 2023

Conversation

kvoli
Copy link
Collaborator

@kvoli kvoli commented Mar 15, 2023

This commit re-enables the kv/gracefuldraining/nodes=3 roachtest. The
test is still likely to fail occasionally however has produced
interesting findings just in testing to re-enable.

Informs: #59094

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@kvoli kvoli marked this pull request as ready for review March 15, 2023 23:45
@kvoli kvoli requested a review from a team as a code owner March 15, 2023 23:45
@kvoli kvoli requested review from smg260, renatolabs and andrewbaptist and removed request for a team March 15, 2023 23:45
@andrewbaptist
Copy link
Collaborator

I'm confused why you switched from the admin ts stats to Prometheus stats. I was hoping with https://grafana.testeng.crdb.io/ set up, we could stop launching separate Prometheus instances with each test, but this change makes that harder to do.

@kvoli kvoli marked this pull request as draft March 16, 2023 22:34
@kvoli kvoli force-pushed the 230315.roachtest-gracefuldrain branch from fb790c8 to 96745fb Compare March 17, 2023 01:20
@kvoli kvoli marked this pull request as ready for review March 17, 2023 01:21
@kvoli
Copy link
Collaborator Author

kvoli commented Mar 17, 2023

I'm confused why you switched from the admin ts stats to Prometheus stats. I was hoping with grafana.testeng.crdb.io set up, we could stop launching separate Prometheus instances with each test, but this change makes that harder to do.

Discussed offline. I updated this commit just to re-enable the test. I was originally very suspicious of the internal timeseries metrics possibly running into an erroneous windowing issue, which could be dealt with separately.

However, upon double checking the debug stuff I had, I realized the drop in QPS was legitimate and showed up in the workload runner (I looked at a different run initially).

I think we should re-enable the test as is. It has already produced useful findings.

Copy link
Collaborator

@andrewbaptist andrewbaptist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - We might have to disable again, but we will at least get a chance to see if things are better.

This commit re-enables the `kv/gracefuldraining/nodes=3` roachtest. The
test is still likely to fail occasionally however has produced
interesting findings just in testing to re-enable.

Informs: cockroachdb#59094

Release note: None
@kvoli kvoli force-pushed the 230315.roachtest-gracefuldrain branch from 96745fb to d0120ff Compare March 30, 2023 14:07
@kvoli
Copy link
Collaborator Author

kvoli commented Mar 30, 2023

@andrewbaptist do you think now's a good time to re-enable? It will likely flake in a couple/few weeks but could also find interesting regressions.

@kvoli kvoli self-assigned this Apr 5, 2023
@kvoli
Copy link
Collaborator Author

kvoli commented May 9, 2023

Going to re-enable this now, I'm anticipating some possible failures in the future but it will be good to start getting coverage again.

bors r=andrewbaptist

@craig
Copy link
Contributor

craig bot commented May 9, 2023

Build succeeded:

@craig craig bot merged commit a961c9d into cockroachdb:master May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants