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

storage/concurrency: test and bugfix for clearing the locks when the #45080

Merged
merged 1 commit into from
Feb 13, 2020

Conversation

sumeerbhola
Copy link
Collaborator

size limit of the lockTable is exceeded

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 5 of 5 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @sumeerbhola)


pkg/storage/concurrency/lock_table.go, line 1497 at r1 (raw file):

		tree := &t.locks[i]
		tree.mu.Lock()
		var locksToClear []*lockState

Heh, I noticed this over in 3f987df as well.


pkg/storage/concurrency/testdata/lock_table/size_limit_exceeded, line 52 at r1 (raw file):

local: num=0

acquire r=req1 k=c durability=r

Add a comment that we don't expect this to add a lock to the table and that we should remove this step entirely when we eventually immediately add replicated locks to the lock table.


pkg/storage/concurrency/testdata/lock_table/size_limit_exceeded, line 87 at r1 (raw file):

----
start-waiting: true

Could you add a print right here?


pkg/storage/concurrency/testdata/lock_table/size_limit_exceeded, line 190 at r1 (raw file):

start-waiting: false

acquire r=req8 k=e durability=u

Add a comment that this is whether the lock table hits its size limit and evicts everything except the discovered lock without a reservation.

size limit of the lockTable is exceeded

Release note: None
Copy link
Collaborator Author

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten and @sumeerbhola)


pkg/storage/concurrency/testdata/lock_table/size_limit_exceeded, line 52 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Add a comment that we don't expect this to add a lock to the table and that we should remove this step entirely when we eventually immediately add replicated locks to the lock table.

Added a slightly different comment. We won't need the unreplicated lock acquisition.


pkg/storage/concurrency/testdata/lock_table/size_limit_exceeded, line 87 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Could you add a print right here?

Done


pkg/storage/concurrency/testdata/lock_table/size_limit_exceeded, line 190 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Add a comment that this is whether the lock table hits its size limit and evicts everything except the discovered lock without a reservation.

Done

Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @sumeerbhola)

@sumeerbhola
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 13, 2020

Build failed

@sumeerbhola
Copy link
Collaborator Author

bors r+

craig bot pushed a commit that referenced this pull request Feb 13, 2020
45039: colexec: exhaust input in sorter benchmarks r=yuzefovich a=asubiotto

Some benchmarks were only calling `Next` the expected number of times and
failing if a zero batch was encountered. However, operators perform cleanup
when a zero batch is returned, so this cleanup step was being elided. It's
also more in line with other benchmarks to exhaust the operator before
finishing a benchmark.

Release note: None (testing code cleanup)

45080: storage/concurrency: test and bugfix for clearing the locks when the r=sumeerbhola a=sumeerbhola

size limit of the lockTable is exceeded

Release note: None

Co-authored-by: Alfonso Subiotto Marques <[email protected]>
Co-authored-by: sumeerbhola <[email protected]>
@craig
Copy link
Contributor

craig bot commented Feb 13, 2020

Build succeeded

@craig craig bot merged commit 9564274 into cockroachdb:master Feb 13, 2020
@sumeerbhola sumeerbhola deleted the ltmem branch February 14, 2020 15:25
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