Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Flume, mail # dev - Review Request 16415: flume-ng accumulo sink


Copy link to this message
-
Re: Review Request 16415: flume-ng accumulo sink
Hari Shreedharan 2014-01-09, 21:52

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16415/#review31464
-----------------------------------------------------------
This looks pretty good!

I am not entirely sure of how the versions for accumulo work with hadoop 1 and 2. We should move the deps to the relevant profiles in the top level pom, so the correct accumulo jars are pulled in for each hadoop version.

Much of what I have is what Bruno already mentioned - several int/strings (the defaults and properties to be used with accumulo) can be made final etc.
flume-ng-sinks/flume-ng-accumulo-sink/src/main/java/org/apache/flume/sink/accumulo/AccumuloEventSerializer.java
<https://reviews.apache.org/r/16415/#comment59960>

    Nit: Extra whitespace

flume-ng-sinks/flume-ng-accumulo-sink/src/main/java/org/apache/flume/sink/accumulo/AccumuloEventSerializer.java
<https://reviews.apache.org/r/16415/#comment59961>

    Nit: Whitespace

flume-ng-sinks/flume-ng-accumulo-sink/src/main/java/org/apache/flume/sink/accumulo/AccumuloSink.java
<https://reviews.apache.org/r/16415/#comment59964>

    You should be throwing this exception out, so that the start failure is seen by the framework and it tries to restart it.

flume-ng-sinks/flume-ng-accumulo-sink/src/main/java/org/apache/flume/sink/accumulo/AccumuloSink.java
<https://reviews.apache.org/r/16415/#comment59962>

    Really, we should return backoff only if the batch is empty, not if we could not get a complete the batch

flume-ng-sinks/flume-ng-accumulo-sink/src/main/java/org/apache/flume/sink/accumulo/AccumuloSink.java
<https://reviews.apache.org/r/16415/#comment59963>

    txn.rollback() should also be in try catch. See the current HBaseSink code.
- Hari Shreedharan
On Dec. 27, 2013, 6 p.m., Matt Molek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16415/
> -----------------------------------------------------------
>
> (Updated Dec. 27, 2013, 6 p.m.)
>
>
> Review request for Flume.
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> This patch is identical to FLUME-2234-0.patch except I had to regenerate it with different git options to get review board to accept it.
>
>
> Diffs
> -----
>
>   flume-ng-sinks/flume-ng-accumulo-sink/pom.xml PRE-CREATION
>   flume-ng-sinks/flume-ng-accumulo-sink/src/main/java/org/apache/flume/sink/accumulo/AccumuloEventSerializer.java PRE-CREATION
>   flume-ng-sinks/flume-ng-accumulo-sink/src/main/java/org/apache/flume/sink/accumulo/AccumuloSink.java PRE-CREATION
>   flume-ng-sinks/flume-ng-accumulo-sink/src/main/java/org/apache/flume/sink/accumulo/AccumuloSinkConfigurationConstants.java PRE-CREATION
>   flume-ng-sinks/flume-ng-accumulo-sink/src/main/java/org/apache/flume/sink/accumulo/SimpleAccumuloEventSerializer.java PRE-CREATION
>   flume-ng-sinks/flume-ng-accumulo-sink/src/test/java/org/apache/flume/sink/accumulo/TestAccumuloSink.java PRE-CREATION
>   flume-ng-sinks/pom.xml d03576b
>
> Diff: https://reviews.apache.org/r/16415/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Matt Molek
>
>