accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ke...@deenlo.com
Subject Re: Review Request: Patch for ACCUMULO-409
Date Mon, 02 Jul 2012 15:51:58 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message