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

storage: make TestRangeTransferLeaseExpirationBased fail more eargerly #31265

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@m-schneider
Contributor

m-schneider commented Oct 11, 2018

Before we were seeing test flakes from
TestRangeTransferLeaseExpirationBased. The reason for this is that
forceLeaseExtension method in client_replica_test.go was swallowing
every NotLeaseHolderError even though it was only supposed to swallow
them when the leaseholder and the proposed leaseholder where the same
replica.

Now we're only going to swallow the error if the proposed leaseholder
and current leaseholder are the same. This will allow tests that depend
on forceLeaseExtension to fail eargely instead of timing out while
waiting for an error.

Release note: None

@m-schneider m-schneider requested a review from tschottdorf Oct 11, 2018

@m-schneider m-schneider requested a review from cockroachdb/core-prs as a code owner Oct 11, 2018

@cockroach-teamcity

This comment has been minimized.

Show comment
Hide comment
@cockroach-teamcity

cockroach-teamcity Oct 11, 2018

Member

This change is Reviewable

Member

cockroach-teamcity commented Oct 11, 2018

This change is Reviewable

@tschottdorf

This comment has been minimized.

Show comment
Hide comment
@tschottdorf

tschottdorf Oct 11, 2018

Member

Wait a minute, that failed these tests immediately. Isn't that surprising?

Member

tschottdorf commented Oct 11, 2018

Wait a minute, that failed these tests immediately. Isn't that surprising?

@m-schneider

This comment has been minimized.

Show comment
Hide comment
@m-schneider

m-schneider Oct 11, 2018

Contributor

That is surprising, I ran it locally a couple of times and it didn't fail. What do you think?

Contributor

m-schneider commented Oct 11, 2018

That is surprising, I ran it locally a couple of times and it didn't fail. What do you think?

@m-schneider

This comment has been minimized.

Show comment
Hide comment
@m-schneider

m-schneider Oct 11, 2018

Contributor

Also just ran a regular make stress and it didn't fail for 10 minutes.

Contributor

m-schneider commented Oct 11, 2018

Also just ran a regular make stress and it didn't fail for 10 minutes.

storage: make TestRangeTransferLeaseExpirationBased fail more eargerly
Before we were seeing test flakes from
TestRangeTransferLeaseExpirationBased. The reason for this is that
forceLeaseExtension method in client_replica_test.go was swallowing
every NotLeaseHolderError even though it was only supposed to swallow
them when the leaseholder and the proposed leaseholder where the same
replica.

Now we're only going to swallow the error if the proposed leaseholder
and current leaseholder are the same. This will allow tests that depend
on forceLeaseExtension to fail eargely instead of timing out while
waiting for an error.

See #30869

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment