You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "DomGarguilo (via GitHub)" <gi...@apache.org> on 2023/07/21 16:24:58 UTC
[GitHub] [accumulo] DomGarguilo opened a new pull request, #3644: ReentrantLock.getOwner() to help clean up scan sessions
DomGarguilo opened a new pull request, #3644:
URL: https://github.com/apache/accumulo/pull/3644
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3644: ReentrantLock.getOwner() to help clean up scan sessions
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3644:
URL: https://github.com/apache/accumulo/pull/3644#discussion_r1273792470
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Scanner.java:
##########
@@ -162,22 +162,38 @@ public ScanBatch read() throws IOException, TabletClosedException {
tablet.updateQueryStats(results.getResults().size(), results.getNumBytes());
}
} finally {
- scannerSemaphore.release();
+ lock.unlock();
Review Comment:
```suggestion
inRead=false;
lock.unlock();
```
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Scanner.java:
##########
@@ -72,7 +72,7 @@ public ScanBatch read() throws IOException, TabletClosedException {
try {
try {
- scannerSemaphore.acquire();
+ lock.lockInterruptibly();
Review Comment:
I think part of what the semaphore was doing was preventing recursive calls to this function. I don't the the current code ever does this, but if it ever did in the future it would leave the instance variable in this class in a bad state. I think a boolean along with the lock can preserve this behavior.
```suggestion
lock.lockInterruptibly();
Preconditions.checkState(!inRead);
// Simple check to ensure the same thread never calls this method recursively. This code would not handle that well.
inRead = true;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3644: ReentrantLock.getOwner() to help clean up scan sessions
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3644:
URL: https://github.com/apache/accumulo/pull/3644#issuecomment-1650218677
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3644: ReentrantLock.getOwner() to help clean up scan sessions
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3644:
URL: https://github.com/apache/accumulo/pull/3644#issuecomment-1650148155
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #3644: ReentrantLock.getOwner() to help clean up scan sessions
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #3644:
URL: https://github.com/apache/accumulo/pull/3644#discussion_r1273830581
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Scanner.java:
##########
@@ -72,7 +72,7 @@ public ScanBatch read() throws IOException, TabletClosedException {
try {
try {
- scannerSemaphore.acquire();
+ lock.lockInterruptibly();
Review Comment:
Added in baa37a9
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Scanner.java:
##########
@@ -162,22 +162,38 @@ public ScanBatch read() throws IOException, TabletClosedException {
tablet.updateQueryStats(results.getResults().size(), results.getNumBytes());
}
} finally {
- scannerSemaphore.release();
+ lock.unlock();
Review Comment:
Added in baa37a9
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #3644: ReentrantLock.getOwner() to help clean up scan sessions
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3644:
URL: https://github.com/apache/accumulo/pull/3644#issuecomment-1647964244
@keith-turner - would running your new test for an extended period also stress these lock changes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #3644: ReentrantLock.getOwner() to help clean up scan sessions
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3644:
URL: https://github.com/apache/accumulo/pull/3644#issuecomment-1648507684
Full IT build passed successfully
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo merged pull request #3644: ReentrantLock.getOwner() to help clean up scan sessions
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo merged PR #3644:
URL: https://github.com/apache/accumulo/pull/3644
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org