Skip to content

ReentrantLock.getOwner() to help clean up scan sessions#3644

Merged
DomGarguilo merged 2 commits intoapache:2.1from
DomGarguilo:cleanUpScanSessions
Jul 26, 2023
Merged

ReentrantLock.getOwner() to help clean up scan sessions#3644
DomGarguilo merged 2 commits intoapache:2.1from
DomGarguilo:cleanUpScanSessions

Conversation

@DomGarguilo
Copy link
Member

Closes #3579

This PR replaces the semaphore with a ReentrantLock in Scanner. A new private class, InterruptibleLock was created to expose the protected getOwner() method.

I am not too sure how to test these changes so have not added any tests yet. Ideas/suggestions are welcome.

Copy link
Contributor

@dlmarion dlmarion left a comment

Choose a reason for hiding this comment

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

LGTM, I kicked off full IT build for this PR.

@EdColeman
Copy link
Contributor

EdColeman commented Jul 24, 2023

@keith-turner - would running your new scan consistency test for an extended period also stress these lock changes?

@dlmarion
Copy link
Contributor

Full IT build passed successfully

@keith-turner
Copy link
Contributor

@keith-turner - would running your new scan consistency test for an extended period also stress these lock changes?

The test will definitely stress the read method modified in this PR. However I do not think it will stress the close method.

I think to stress this the test would need to do the following

  1. Start a scan over range x to +inf
  2. Do not read all data, leaving a scan session on the tserver to be cleaned up

I don't think the test does this, but it should if it does not. Some of its scans should randomly be open ended and not read all the data. On a long running test this would trigger the session clean up that calls close on an idle session.

Co-authored-by: Keith Turner <kturner@apache.org>
@keith-turner
Copy link
Contributor

@EdColeman I created #3656 to add an open ended scan to the test. However I am not sure if will leave a scan session open on the tserver. A scan may have to not read all data from a scanner and not close the scanner to leave a scan session open. I am not sure though. Wonder if the test should randomly not close scanners also.

@DomGarguilo DomGarguilo merged commit 5adc92b into apache:2.1 Jul 26, 2023
@DomGarguilo DomGarguilo deleted the cleanUpScanSessions branch July 26, 2023 15:15
@ctubbsii ctubbsii added this to the 2.1.2 milestone Jul 12, 2024
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.

Follow-on from #3569 - use ReentrantLock.getOwner() to help clean up scan sessions

5 participants