You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Hari Shreedharan <hs...@cloudera.com> on 2013/01/07 21:35:17 UTC
Review Request: FLUME-1821. Support configuration of hbase instances to be
used in AsyncHBaseSink from flume config
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8864/
-----------------------------------------------------------
Review request for Flume.
Description
-------
AsyncHbasesink can now be configured to use zk quorum and zk root from configuration file.
This addresses bug FLUME-1821.
https://issues.apache.org/jira/browse/FLUME-1821
Diffs
-----
flume-ng-doc/sphinx/FlumeUserGuide.rst 63b8f9b
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java 1598f26
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 463c9c3
flume-ng-sinks/flume-ng-hbase-sink/src/test/java/org/apache/flume/sink/hbase/TestAsyncHBaseSink.java c835172
Diff: https://reviews.apache.org/r/8864/diff/
Testing
-------
All current tests pass. Added test to make sure new code works.
Thanks,
Hari Shreedharan
Re: Review Request: FLUME-1821. Support configuration of hbase instances to
be used in AsyncHBaseSink from flume config
Posted by Hari Shreedharan <hs...@cloudera.com>.
> On Jan. 10, 2013, 12:27 a.m., Brock Noland wrote:
> > flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java, line 112
> > <https://reviews.apache.org/r/8864/diff/5/?file=245497#file245497line112>
> >
> > Should this constructor delegate to the other constructor passing in a null?
I don't see this adding any value, all it would do is simply set conf to null, which it already is - right? I am just avoiding that one extra method call overhead.
> On Jan. 10, 2013, 12:27 a.m., Brock Noland wrote:
> > flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java, line 304
> > <https://reviews.apache.org/r/8864/diff/5/?file=245497#file245497line304>
> >
> > Good idea! How about doing same check for base dir and give both checks a good error message?
Actually if it is null, it perfectly ok - since we simply startup without it:
if(zkBaseDir != null){
client = new HBaseClient(zkQuorum, zkBaseDir);
} else {
client = new HBaseClient(zkQuorum);
}
Added an error message though.
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8864/#review15217
-----------------------------------------------------------
On Jan. 8, 2013, 12:14 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8864/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2013, 12:14 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> AsyncHbasesink can now be configured to use zk quorum and zk root from configuration file.
>
>
> This addresses bug FLUME-1821.
> https://issues.apache.org/jira/browse/FLUME-1821
>
>
> Diffs
> -----
>
> flume-ng-doc/sphinx/FlumeUserGuide.rst 63b8f9b
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java 1598f26
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 463c9c3
> flume-ng-sinks/flume-ng-hbase-sink/src/test/java/org/apache/flume/sink/hbase/TestAsyncHBaseSink.java c835172
>
> Diff: https://reviews.apache.org/r/8864/diff/
>
>
> Testing
> -------
>
> All current tests pass. Added test to make sure new code works.
>
>
> Thanks,
>
> Hari Shreedharan
>
>
Re: Review Request: FLUME-1821. Support configuration of hbase instances to
be used in AsyncHBaseSink from flume config
Posted by Brock Noland <br...@cloudera.com>.
> On Jan. 10, 2013, 12:27 a.m., Brock Noland wrote:
> > flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java, line 304
> > <https://reviews.apache.org/r/8864/diff/5/?file=245497#file245497line304>
> >
> > Good idea! How about doing same check for base dir and give both checks a good error message?
>
> Hari Shreedharan wrote:
> Actually if it is null, it perfectly ok - since we simply startup without it:
>
> if(zkBaseDir != null){
> client = new HBaseClient(zkQuorum, zkBaseDir);
> } else {
> client = new HBaseClient(zkQuorum);
> }
>
> Added an error message though.
Good call
> On Jan. 10, 2013, 12:27 a.m., Brock Noland wrote:
> > flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java, line 112
> > <https://reviews.apache.org/r/8864/diff/5/?file=245497#file245497line112>
> >
> > Should this constructor delegate to the other constructor passing in a null?
>
> Hari Shreedharan wrote:
> I don't see this adding any value, all it would do is simply set conf to null, which it already is - right? I am just avoiding that one extra method call overhead.
It's nice to have only one true constructor but this case is quite trivial. No need to update.
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8864/#review15217
-----------------------------------------------------------
On Jan. 10, 2013, 12:53 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8864/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2013, 12:53 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> AsyncHbasesink can now be configured to use zk quorum and zk root from configuration file.
>
>
> This addresses bug FLUME-1821.
> https://issues.apache.org/jira/browse/FLUME-1821
>
>
> Diffs
> -----
>
> flume-ng-doc/sphinx/FlumeUserGuide.rst 63b8f9b
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java 1598f26
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 463c9c3
> flume-ng-sinks/flume-ng-hbase-sink/src/test/java/org/apache/flume/sink/hbase/TestAsyncHBaseSink.java c835172
>
> Diff: https://reviews.apache.org/r/8864/diff/
>
>
> Testing
> -------
>
> All current tests pass. Added test to make sure new code works.
>
>
> Thanks,
>
> Hari Shreedharan
>
>
Re: Review Request: FLUME-1821. Support configuration of hbase instances to
be used in AsyncHBaseSink from flume config
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8864/#review15217
-----------------------------------------------------------
Hari, great patch! A few items below all minor.
flume-ng-doc/sphinx/FlumeUserGuide.rst
<https://reviews.apache.org/r/8864/#comment32823>
This coupled with the statement below about hbase-site.xml are slightly unclear to me. How about something like "Zookkeeper Quorum and parent node configuration may be specified in the flume configuration file, alternatively these configuration values are taken from the first hbase-site.xml file in the classpath.
flume-ng-doc/sphinx/FlumeUserGuide.rst
<https://reviews.apache.org/r/8864/#comment32824>
Great doc update! znodeParent has a default value above, do we need to re-state it's not mandatory?
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java
<https://reviews.apache.org/r/8864/#comment32826>
Should this constructor delegate to the other constructor passing in a null?
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java
<https://reviews.apache.org/r/8864/#comment32822>
How about passing in empty string as the default this would eliminate the null check. Also should we be calling .trim() on the return value of getString()?
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java
<https://reviews.apache.org/r/8864/#comment32820>
Good idea! How about doing same check for base dir and give both checks a good error message?
- Brock Noland
On Jan. 8, 2013, 12:14 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8864/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2013, 12:14 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> AsyncHbasesink can now be configured to use zk quorum and zk root from configuration file.
>
>
> This addresses bug FLUME-1821.
> https://issues.apache.org/jira/browse/FLUME-1821
>
>
> Diffs
> -----
>
> flume-ng-doc/sphinx/FlumeUserGuide.rst 63b8f9b
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java 1598f26
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 463c9c3
> flume-ng-sinks/flume-ng-hbase-sink/src/test/java/org/apache/flume/sink/hbase/TestAsyncHBaseSink.java c835172
>
> Diff: https://reviews.apache.org/r/8864/diff/
>
>
> Testing
> -------
>
> All current tests pass. Added test to make sure new code works.
>
>
> Thanks,
>
> Hari Shreedharan
>
>
Re: Review Request: FLUME-1821. Support configuration of hbase instances to
be used in AsyncHBaseSink from flume config
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8864/#review15242
-----------------------------------------------------------
Ship it!
Ship It!
- Brock Noland
On Jan. 10, 2013, 12:53 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8864/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2013, 12:53 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> AsyncHbasesink can now be configured to use zk quorum and zk root from configuration file.
>
>
> This addresses bug FLUME-1821.
> https://issues.apache.org/jira/browse/FLUME-1821
>
>
> Diffs
> -----
>
> flume-ng-doc/sphinx/FlumeUserGuide.rst 63b8f9b
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java 1598f26
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 463c9c3
> flume-ng-sinks/flume-ng-hbase-sink/src/test/java/org/apache/flume/sink/hbase/TestAsyncHBaseSink.java c835172
>
> Diff: https://reviews.apache.org/r/8864/diff/
>
>
> Testing
> -------
>
> All current tests pass. Added test to make sure new code works.
>
>
> Thanks,
>
> Hari Shreedharan
>
>
Re: Review Request: FLUME-1821. Support configuration of hbase instances to
be used in AsyncHBaseSink from flume config
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8864/
-----------------------------------------------------------
(Updated Jan. 10, 2013, 12:53 a.m.)
Review request for Flume.
Description
-------
AsyncHbasesink can now be configured to use zk quorum and zk root from configuration file.
This addresses bug FLUME-1821.
https://issues.apache.org/jira/browse/FLUME-1821
Diffs (updated)
-----
flume-ng-doc/sphinx/FlumeUserGuide.rst 63b8f9b
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java 1598f26
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 463c9c3
flume-ng-sinks/flume-ng-hbase-sink/src/test/java/org/apache/flume/sink/hbase/TestAsyncHBaseSink.java c835172
Diff: https://reviews.apache.org/r/8864/diff/
Testing
-------
All current tests pass. Added test to make sure new code works.
Thanks,
Hari Shreedharan
Re: Review Request: FLUME-1821. Support configuration of hbase instances to
be used in AsyncHBaseSink from flume config
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8864/
-----------------------------------------------------------
(Updated Jan. 8, 2013, 12:14 a.m.)
Review request for Flume.
Changes
-------
Minor test update.
Description
-------
AsyncHbasesink can now be configured to use zk quorum and zk root from configuration file.
This addresses bug FLUME-1821.
https://issues.apache.org/jira/browse/FLUME-1821
Diffs (updated)
-----
flume-ng-doc/sphinx/FlumeUserGuide.rst 63b8f9b
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java 1598f26
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 463c9c3
flume-ng-sinks/flume-ng-hbase-sink/src/test/java/org/apache/flume/sink/hbase/TestAsyncHBaseSink.java c835172
Diff: https://reviews.apache.org/r/8864/diff/
Testing
-------
All current tests pass. Added test to make sure new code works.
Thanks,
Hari Shreedharan
Re: Review Request: FLUME-1821. Support configuration of hbase instances to
be used in AsyncHBaseSink from flume config
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8864/
-----------------------------------------------------------
(Updated Jan. 7, 2013, 11:15 p.m.)
Review request for Flume.
Changes
-------
Updated tests.
Description
-------
AsyncHbasesink can now be configured to use zk quorum and zk root from configuration file.
This addresses bug FLUME-1821.
https://issues.apache.org/jira/browse/FLUME-1821
Diffs (updated)
-----
flume-ng-doc/sphinx/FlumeUserGuide.rst 63b8f9b
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java 1598f26
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 463c9c3
flume-ng-sinks/flume-ng-hbase-sink/src/test/java/org/apache/flume/sink/hbase/TestAsyncHBaseSink.java c835172
Diff: https://reviews.apache.org/r/8864/diff/
Testing
-------
All current tests pass. Added test to make sure new code works.
Thanks,
Hari Shreedharan
Re: Review Request: FLUME-1821. Support configuration of hbase instances to
be used in AsyncHBaseSink from flume config
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8864/
-----------------------------------------------------------
(Updated Jan. 7, 2013, 10:33 p.m.)
Review request for Flume.
Changes
-------
Further doc updates.
Description
-------
AsyncHbasesink can now be configured to use zk quorum and zk root from configuration file.
This addresses bug FLUME-1821.
https://issues.apache.org/jira/browse/FLUME-1821
Diffs (updated)
-----
flume-ng-doc/sphinx/FlumeUserGuide.rst 63b8f9b
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java 1598f26
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 463c9c3
flume-ng-sinks/flume-ng-hbase-sink/src/test/java/org/apache/flume/sink/hbase/TestAsyncHBaseSink.java c835172
Diff: https://reviews.apache.org/r/8864/diff/
Testing
-------
All current tests pass. Added test to make sure new code works.
Thanks,
Hari Shreedharan
Re: Review Request: FLUME-1821. Support configuration of hbase instances to
be used in AsyncHBaseSink from flume config
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8864/
-----------------------------------------------------------
(Updated Jan. 7, 2013, 10:30 p.m.)
Review request for Flume.
Changes
-------
Updating documentation to make it clearer.
Description
-------
AsyncHbasesink can now be configured to use zk quorum and zk root from configuration file.
This addresses bug FLUME-1821.
https://issues.apache.org/jira/browse/FLUME-1821
Diffs (updated)
-----
flume-ng-doc/sphinx/FlumeUserGuide.rst 63b8f9b
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/AsyncHBaseSink.java 1598f26
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 463c9c3
flume-ng-sinks/flume-ng-hbase-sink/src/test/java/org/apache/flume/sink/hbase/TestAsyncHBaseSink.java c835172
Diff: https://reviews.apache.org/r/8864/diff/
Testing
-------
All current tests pass. Added test to make sure new code works.
Thanks,
Hari Shreedharan