You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Adam Fuchs <af...@apache.org> on 2012/07/02 17:13:13 UTC
Re: Review Request: Patch for ACCUMULO-409
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5684/#review8792
-----------------------------------------------------------
/branches/1.4/src/core/src/main/java/org/apache/accumulo/core/Constants.java
<https://reviews.apache.org/r/5684/#comment18613>
Is it obvious to the general developer who might be trying to read this code that BIF stands for bulk import failure? Maybe a better naming scheme for variables, constants, and directories is in order.
/branches/1.4/src/server/src/main/java/org/apache/accumulo/server/master/tableOps/BulkImport.java
<https://reviews.apache.org/r/5684/#comment18615>
Ack! Formatting!
/branches/1.4/src/server/src/main/java/org/apache/accumulo/server/master/tableOps/BulkImport.java
<https://reviews.apache.org/r/5684/#comment18616>
Also need to check whether a concurrent operation could be in progress that writes the load flag after we check for its existence.
/branches/1.4/src/server/src/main/java/org/apache/accumulo/server/tabletserver/TabletServer.java
<https://reviews.apache.org/r/5684/#comment18617>
Fixed-sized thread pool seems like a problem for scaling to different-sized machines. Also, what does 2 threads imply given the thread pool queue size checks in DistributedWorkQueue? Seems like if we generalize the thread pool size that would cause problems.
Another thought here: we do a bunch of stuff that is distributed through tablet servers. Does it make sense to have threads dedicated to this particular distrubted task, or should this be part of a generalized framework?
/branches/1.4/src/server/src/main/java/org/apache/accumulo/server/zookeeper/DistributedWorkQueue.java
<https://reviews.apache.org/r/5684/#comment18614>
What is special about a queue size of greater than one? Also, this code is repeated in several spots -- should it be turned into a method?
- Adam Fuchs
On June 30, 2012, 3:10 a.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5684/
> -----------------------------------------------------------
>
> (Updated June 30, 2012, 3:10 a.m.)
>
>
> Review request for accumulo.
>
>
> Description
> -------
>
> A patch that fixes the issue of failed bulk imports being copied through the master.
>
>
> This addresses bug ACCUMULO-409.
> https://issues.apache.org/jira/browse/ACCUMULO-409
>
>
> Diffs
> -----
>
> /branches/1.4/src/core/src/main/java/org/apache/accumulo/core/Constants.java 1355540
> /branches/1.4/src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooUtil.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/client/BulkImporter.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/master/tableOps/BulkImport.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/tabletserver/BIFCopyProcessor.java PRE-CREATION
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/tabletserver/TabletServer.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/zookeeper/DistributedWorkQueue.java PRE-CREATION
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/zookeeper/IZooReaderWriter.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/zookeeper/ZooReaderWriter.java 1355540
> /branches/1.4/test/system/test4/bulk_import_test.sh 1355540
>
> Diff: https://reviews.apache.org/r/5684/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kturner
>
>
Re: Review Request: Patch for ACCUMULO-409
Posted by ke...@deenlo.com.
> On July 2, 2012, 3:13 p.m., Adam Fuchs wrote:
> > /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/zookeeper/DistributedWorkQueue.java, line 52
> > <https://reviews.apache.org/r/5684/diff/1/?file=117637#file117637line52>
> >
> > What is special about a queue size of greater than one? Also, this code is repeated in several spots -- should it be turned into a method?
>
> kturner wrote:
> I am not sure if there is way to tell if all threads are busy in the pool other than the fact that at least one thing is queued. Basically if all threads are busy, then the tablet server should not reserve it because another tablet server my have processing capacity.
>
> Adam Fuchs wrote:
> The condition would be > 0 and not > 1, right?
>
> Seems like there's a potential race condition, where the first thread would just empty the queue and none of the other threads would make progress.
Could increment an atomic long before adding a task to the thread pool and then decrement the atomic long when a task finishes. Could check this atomic long against the thread pool size before adding to the pool. This count will include queued and running. Need to decrement in a finally block of the task.
- kturner
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5684/#review8792
-----------------------------------------------------------
On June 30, 2012, 3:10 a.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5684/
> -----------------------------------------------------------
>
> (Updated June 30, 2012, 3:10 a.m.)
>
>
> Review request for accumulo.
>
>
> Description
> -------
>
> A patch that fixes the issue of failed bulk imports being copied through the master.
>
>
> This addresses bug ACCUMULO-409.
> https://issues.apache.org/jira/browse/ACCUMULO-409
>
>
> Diffs
> -----
>
> /branches/1.4/src/core/src/main/java/org/apache/accumulo/core/Constants.java 1355540
> /branches/1.4/src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooUtil.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/client/BulkImporter.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/master/tableOps/BulkImport.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/tabletserver/BIFCopyProcessor.java PRE-CREATION
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/tabletserver/TabletServer.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/zookeeper/DistributedWorkQueue.java PRE-CREATION
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/zookeeper/IZooReaderWriter.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/zookeeper/ZooReaderWriter.java 1355540
> /branches/1.4/test/system/test4/bulk_import_test.sh 1355540
>
> Diff: https://reviews.apache.org/r/5684/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kturner
>
>
Re: Review Request: Patch for ACCUMULO-409
Posted by ke...@deenlo.com.
> On July 2, 2012, 3:13 p.m., Adam Fuchs wrote:
> > /branches/1.4/src/core/src/main/java/org/apache/accumulo/core/Constants.java, line 80
> > <https://reviews.apache.org/r/5684/diff/1/?file=117631#file117631line80>
> >
> > Is it obvious to the general developer who might be trying to read this code that BIF stands for bulk import failure? Maybe a better naming scheme for variables, constants, and directories is in order.
Its not obvious
> On July 2, 2012, 3:13 p.m., Adam Fuchs wrote:
> > /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/master/tableOps/BulkImport.java, line 394
> > <https://reviews.apache.org/r/5684/diff/1/?file=117634#file117634line394>
> >
> > Also need to check whether a concurrent operation could be in progress that writes the load flag after we check for its existence.
the code in CleanUpBulkImport.isReady() needs to happen before CopyFailed
> On July 2, 2012, 3:13 p.m., Adam Fuchs wrote:
> > /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/tabletserver/TabletServer.java, line 2687
> > <https://reviews.apache.org/r/5684/diff/1/?file=117636#file117636line2687>
> >
> > Fixed-sized thread pool seems like a problem for scaling to different-sized machines. Also, what does 2 threads imply given the thread pool queue size checks in DistributedWorkQueue? Seems like if we generalize the thread pool size that would cause problems.
> >
> > Another thought here: we do a bunch of stuff that is distributed through tablet servers. Does it make sense to have threads dedicated to this particular distrubted task, or should this be part of a generalized framework?
Probably should make it configurable, yet another knob.
The code is general, in 1.5 I plan to use the DistributedWorkQueue for write ahead log sorts. The code was taken from 1.5 log sorts and generalized along with a fixing a zookeeper locking bug. When this is merged to 1.5, the existing code can be modified to use it.
> On July 2, 2012, 3:13 p.m., Adam Fuchs wrote:
> > /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/zookeeper/DistributedWorkQueue.java, line 52
> > <https://reviews.apache.org/r/5684/diff/1/?file=117637#file117637line52>
> >
> > What is special about a queue size of greater than one? Also, this code is repeated in several spots -- should it be turned into a method?
I am not sure if there is way to tell if all threads are busy in the pool other than the fact that at least one thing is queued. Basically if all threads are busy, then the tablet server should not reserve it because another tablet server my have processing capacity.
- kturner
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5684/#review8792
-----------------------------------------------------------
On June 30, 2012, 3:10 a.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5684/
> -----------------------------------------------------------
>
> (Updated June 30, 2012, 3:10 a.m.)
>
>
> Review request for accumulo.
>
>
> Description
> -------
>
> A patch that fixes the issue of failed bulk imports being copied through the master.
>
>
> This addresses bug ACCUMULO-409.
> https://issues.apache.org/jira/browse/ACCUMULO-409
>
>
> Diffs
> -----
>
> /branches/1.4/src/core/src/main/java/org/apache/accumulo/core/Constants.java 1355540
> /branches/1.4/src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooUtil.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/client/BulkImporter.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/master/tableOps/BulkImport.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/tabletserver/BIFCopyProcessor.java PRE-CREATION
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/tabletserver/TabletServer.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/zookeeper/DistributedWorkQueue.java PRE-CREATION
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/zookeeper/IZooReaderWriter.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/zookeeper/ZooReaderWriter.java 1355540
> /branches/1.4/test/system/test4/bulk_import_test.sh 1355540
>
> Diff: https://reviews.apache.org/r/5684/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kturner
>
>
Re: Review Request: Patch for ACCUMULO-409
Posted by Adam Fuchs <af...@apache.org>.
> On July 2, 2012, 3:13 p.m., Adam Fuchs wrote:
> > /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/zookeeper/DistributedWorkQueue.java, line 52
> > <https://reviews.apache.org/r/5684/diff/1/?file=117637#file117637line52>
> >
> > What is special about a queue size of greater than one? Also, this code is repeated in several spots -- should it be turned into a method?
>
> kturner wrote:
> I am not sure if there is way to tell if all threads are busy in the pool other than the fact that at least one thing is queued. Basically if all threads are busy, then the tablet server should not reserve it because another tablet server my have processing capacity.
The condition would be > 0 and not > 1, right?
Seems like there's a potential race condition, where the first thread would just empty the queue and none of the other threads would make progress.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5684/#review8792
-----------------------------------------------------------
On June 30, 2012, 3:10 a.m., kturner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5684/
> -----------------------------------------------------------
>
> (Updated June 30, 2012, 3:10 a.m.)
>
>
> Review request for accumulo.
>
>
> Description
> -------
>
> A patch that fixes the issue of failed bulk imports being copied through the master.
>
>
> This addresses bug ACCUMULO-409.
> https://issues.apache.org/jira/browse/ACCUMULO-409
>
>
> Diffs
> -----
>
> /branches/1.4/src/core/src/main/java/org/apache/accumulo/core/Constants.java 1355540
> /branches/1.4/src/core/src/main/java/org/apache/accumulo/core/zookeeper/ZooUtil.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/client/BulkImporter.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/master/tableOps/BulkImport.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/tabletserver/BIFCopyProcessor.java PRE-CREATION
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/tabletserver/TabletServer.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/zookeeper/DistributedWorkQueue.java PRE-CREATION
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/zookeeper/IZooReaderWriter.java 1355540
> /branches/1.4/src/server/src/main/java/org/apache/accumulo/server/zookeeper/ZooReaderWriter.java 1355540
> /branches/1.4/test/system/test4/bulk_import_test.sh 1355540
>
> Diff: https://reviews.apache.org/r/5684/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kturner
>
>