You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by phrocker <gi...@git.apache.org> on 2016/01/01 02:51:17 UTC
[GitHub] accumulo pull request: ACCUMULO-3509: Allow cleanup state to be ke...
GitHub user phrocker opened a pull request:
https://github.com/apache/accumulo/pull/60
ACCUMULO-3509: Allow cleanup state to be kept to avoid blocking tserv…
…er session sweep
By enabling state ( true/false) from the cleanup method, the change will avoid blocking
on a scan session being swept. if the session cleanup blocks because a ScanSession is
still being read, we may block until the ScanBatch returns for that ScanSession.
The change uses a simple semaphore ( purely because I like the word ) to attempt acquisition.
If that fails, we return false from the cleanup and reintroduce that Session back into
the queue to clean up.
Added unit test that in the error condition will show the missing scan sessions ( which are missing due to the cleanup being blocked)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/phrocker/accumulo-1 master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/60.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #60
----
commit bbd2257a001d654a87b433bb387ad9fce402dbb8
Author: phrocker <ma...@gmail.com>
Date: 2015-12-31T15:16:49Z
ACCUMULO-3509: Allow cleanup state to be kept to avoid blocking tserver session sweep
By enabling state ( true/false) from the cleanup method, the change will avoid blocking
on a scan session being swept. if the session cleanup blocks because a ScanSession is
still being read, we may block until the ScanBatch returns for that ScanSession.
The change uses a simple semaphore ( purely because I like the word ) to attempt acquisition.
If that fails, we return false from the cleanup and reintroduce that Session back into
the queue to clean up.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-3509: Allow cleanup state to be ke...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/60#discussion_r48752641
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Scanner.java ---
@@ -38,100 +40,126 @@
private ScanDataSource isolatedDataSource;
private boolean sawException = false;
private boolean scanClosed = false;
+ protected Semaphore scannerSemaphore;
Scanner(Tablet tablet, Range range, ScanOptions options) {
this.tablet = tablet;
this.range = range;
this.options = options;
+ scannerSemaphore = new Semaphore(1, true);
--- End diff --
This semaphore replaces a synchronized block with a fair semaphore. AFAICT from searching around on the web, synchronized blocks are not fair. Do we need fairness here? The javadocs say its more expensive. If its not needed, not sure should set it.
I was trying to determine the difference between the semaphore and a lock for this case. It seems like the only difference is re-entrance, which it does not seem is needed in this case.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-3509: Allow cleanup state to be ke...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/60#discussion_r48753936
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Scanner.java ---
@@ -38,100 +40,126 @@
private ScanDataSource isolatedDataSource;
private boolean sawException = false;
private boolean scanClosed = false;
+ protected Semaphore scannerSemaphore;
Scanner(Tablet tablet, Range range, ScanOptions options) {
this.tablet = tablet;
this.range = range;
this.options = options;
+ scannerSemaphore = new Semaphore(1, true);
--- End diff --
The expense of fairness may not be a problem. I suspect the expense of fairness is an issue for semaphores that are frequently accessed by many threads, which is not the case here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-3509: Allow cleanup state to be ke...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker closed the pull request at:
https://github.com/apache/accumulo/pull/60
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-3509: Allow cleanup state to be ke...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/60#discussion_r48749879
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Scanner.java ---
@@ -38,100 +40,126 @@
private ScanDataSource isolatedDataSource;
private boolean sawException = false;
private boolean scanClosed = false;
+ protected Semaphore scannerSemaphore;
Scanner(Tablet tablet, Range range, ScanOptions options) {
this.tablet = tablet;
this.range = range;
this.options = options;
+ scannerSemaphore = new Semaphore(1, true);
}
- public synchronized ScanBatch read() throws IOException, TabletClosedException {
-
- if (sawException)
- throw new IllegalStateException("Tried to use scanner after exception occurred.");
+ public ScanBatch read() throws IOException, TabletClosedException {
if (scanClosed)
--- End diff --
Below is from the semaphore javadoc. Reading that is sounds like there is no guarantee of seeing a change to `scanClosed` until after `acquire()`.
```
Memory consistency effects: Actions in a thread prior to calling a "release" method such as release() happen-before actions following a successful "acquire" method such as acquire() in another thread.
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-3509: Allow cleanup state to be ke...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/60#discussion_r48753512
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Scanner.java ---
@@ -38,100 +40,126 @@
private ScanDataSource isolatedDataSource;
private boolean sawException = false;
private boolean scanClosed = false;
+ protected Semaphore scannerSemaphore;
Scanner(Tablet tablet, Range range, ScanOptions options) {
this.tablet = tablet;
this.range = range;
this.options = options;
+ scannerSemaphore = new Semaphore(1, true);
}
- public synchronized ScanBatch read() throws IOException, TabletClosedException {
-
- if (sawException)
- throw new IllegalStateException("Tried to use scanner after exception occurred.");
+ public ScanBatch read() throws IOException, TabletClosedException {
if (scanClosed)
--- End diff --
I made a typo when writing the patch from my other machine. I made the boolean volatile there but in talking with you I will move the scanClosed below the acquisition to avoid the issue entirely and ensure cache coherency.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-3509: Allow cleanup state to be ke...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/60#discussion_r48753072
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Scanner.java ---
@@ -38,100 +40,126 @@
private ScanDataSource isolatedDataSource;
private boolean sawException = false;
private boolean scanClosed = false;
+ protected Semaphore scannerSemaphore;
Scanner(Tablet tablet, Range range, ScanOptions options) {
this.tablet = tablet;
this.range = range;
this.options = options;
+ scannerSemaphore = new Semaphore(1, true);
--- End diff --
My only reason for enforcing fairness was that we likely only have a single thread doing reads ( otherwise we'd have different sessions ); however, we will have another thread doing a close. I wanted to enforce order in the event that the read should occur first. In rethinking this I'm not terribly certain this a problem.
As we discussed I only used a semaphore based on the aforementioned assumption of a single thread doing reads within the scanner. I'm not tied to the idea.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-3509: Allow cleanup state to be ke...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on the pull request:
https://github.com/apache/accumulo/pull/60#issuecomment-168673706
I'm going to target 1.6.5 so it can be merged up.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---