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 2012/12/06 00:46:47 UTC
Review Request: FLUME-1726. HBase sink security support
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8365/
-----------------------------------------------------------
Review request for Flume.
Description
-------
Did some basic testing against secure and non-secure hbase. I will update this patch as I test. This is an initial patch only, changes are likely.
This addresses bug FLUME-1726.
https://issues.apache.org/jira/browse/FLUME-1726
Diffs
-----
flume-ng-sinks/flume-ng-hbase-sink/pom.xml 25422e1
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java 021ecd0
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 62f7097
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkSecurityManager.java PRE-CREATION
Diff: https://reviews.apache.org/r/8365/diff/
Testing
-------
Thanks,
Hari Shreedharan
Re: Review Request: FLUME-1726. HBase sink security support
Posted by Matteo Bertozzi <th...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8365/#review14091
-----------------------------------------------------------
Ship it!
Ship It!
- Matteo Bertozzi
On Dec. 6, 2012, 7:48 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8365/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2012, 7:48 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Did some basic testing against secure and non-secure hbase. I will update this patch as I test. This is an initial patch only, changes are likely.
>
>
> This addresses bug FLUME-1726.
> https://issues.apache.org/jira/browse/FLUME-1726
>
>
> Diffs
> -----
>
> flume-ng-sinks/flume-ng-hbase-sink/pom.xml 25422e1
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java 021ecd0
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 62f7097
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkSecurityManager.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8365/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hari Shreedharan
>
>
Re: Review Request: FLUME-1726. HBase sink security support
Posted by Hari Shreedharan <hs...@cloudera.com>.
> On Dec. 6, 2012, 7:34 p.m., Brock Noland wrote:
> > Looks good! A couple more comments/questions below. Also, it looks like we could unit test a fair amount of HBaseSinkSecurityManager even if we cannot unit test the actual kerberos login portion at this time? Do you think that is possible?
I don't see much value in adding unit testing in the HBaseSinkSecurityManager, since we cannot really test the kerberos part. All of the code in that method is already unit-tested by HBase. But please see https://issues.apache.org/jira/browse/FLUME-1737. If you have ideas on unit-testing without kerberos, please add to that jira.
> On Dec. 6, 2012, 7:34 p.m., Brock Noland wrote:
> > flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkSecurityManager.java, line 67
> > <https://reviews.apache.org/r/8365/diff/3/?file=234950#file234950line67>
> >
> > This method will only be called when security is enabled correct?
> >
> > Assuming that is true do we want to return null from this method because the subsequent call to hbase will fail with an error message anyway, correct?
We should throw instead of returning null so that the sink does not start up.
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8365/#review14101
-----------------------------------------------------------
On Dec. 6, 2012, 7:22 p.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8365/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2012, 7:22 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Did some basic testing against secure and non-secure hbase. I will update this patch as I test. This is an initial patch only, changes are likely.
>
>
> This addresses bug FLUME-1726.
> https://issues.apache.org/jira/browse/FLUME-1726
>
>
> Diffs
> -----
>
> flume-ng-sinks/flume-ng-hbase-sink/pom.xml 25422e1
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java 021ecd0
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 62f7097
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkSecurityManager.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8365/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hari Shreedharan
>
>
Re: Review Request: FLUME-1726. HBase sink security support
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8365/#review14101
-----------------------------------------------------------
Looks good! A couple more comments/questions below. Also, it looks like we could unit test a fair amount of HBaseSinkSecurityManager even if we cannot unit test the actual kerberos login portion at this time? Do you think that is possible?
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkSecurityManager.java
<https://reviews.apache.org/r/8365/#comment30172>
Nit: Empty javadoc comment
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkSecurityManager.java
<https://reviews.apache.org/r/8365/#comment30171>
Nit: Should we remove the return statement or fill it in?
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkSecurityManager.java
<https://reviews.apache.org/r/8365/#comment30169>
This method will only be called when security is enabled correct?
Assuming that is true do we want to return null from this method because the subsequent call to hbase will fail with an error message anyway, correct?
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkSecurityManager.java
<https://reviews.apache.org/r/8365/#comment30170>
Nit: Looks like there is a space missing in the error message.
- Brock Noland
On Dec. 6, 2012, 7:22 p.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8365/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2012, 7:22 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Did some basic testing against secure and non-secure hbase. I will update this patch as I test. This is an initial patch only, changes are likely.
>
>
> This addresses bug FLUME-1726.
> https://issues.apache.org/jira/browse/FLUME-1726
>
>
> Diffs
> -----
>
> flume-ng-sinks/flume-ng-hbase-sink/pom.xml 25422e1
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java 021ecd0
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 62f7097
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkSecurityManager.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8365/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hari Shreedharan
>
>
Re: Review Request: FLUME-1626. HBase sink security support
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8365/#review14139
-----------------------------------------------------------
Patch looks good, I will run tests as soon as git is back up.
- Brock Noland
On Dec. 6, 2012, 7:55 p.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8365/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2012, 7:55 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Did some basic testing against secure and non-secure hbase. I will update this patch as I test. This is an initial patch only, changes are likely.
>
>
> This addresses bug FLUME-1626.
> https://issues.apache.org/jira/browse/FLUME-1626
>
>
> Diffs
> -----
>
> flume-ng-sinks/flume-ng-hbase-sink/pom.xml 25422e1
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java 021ecd0
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 62f7097
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkSecurityManager.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8365/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hari Shreedharan
>
>
Re: Review Request: FLUME-1626. HBase sink security support
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8365/
-----------------------------------------------------------
(Updated Dec. 6, 2012, 7:55 p.m.)
Review request for Flume.
Changes
-------
Changing the jira this points to.
Summary (updated)
-----------------
FLUME-1626. HBase sink security support
Description
-------
Did some basic testing against secure and non-secure hbase. I will update this patch as I test. This is an initial patch only, changes are likely.
This addresses bug FLUME-1626.
https://issues.apache.org/jira/browse/FLUME-1626
Diffs
-----
flume-ng-sinks/flume-ng-hbase-sink/pom.xml 25422e1
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java 021ecd0
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 62f7097
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkSecurityManager.java PRE-CREATION
Diff: https://reviews.apache.org/r/8365/diff/
Testing
-------
Thanks,
Hari Shreedharan
Re: Review Request: FLUME-1726. HBase sink security support
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8365/
-----------------------------------------------------------
(Updated Dec. 6, 2012, 7:55 p.m.)
Review request for Flume.
Description
-------
Did some basic testing against secure and non-secure hbase. I will update this patch as I test. This is an initial patch only, changes are likely.
This addresses bug FLUME-1726.
https://issues.apache.org/jira/browse/FLUME-1726
Diffs (updated)
-----
flume-ng-sinks/flume-ng-hbase-sink/pom.xml 25422e1
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java 021ecd0
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 62f7097
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkSecurityManager.java PRE-CREATION
Diff: https://reviews.apache.org/r/8365/diff/
Testing
-------
Thanks,
Hari Shreedharan
Re: Review Request: FLUME-1726. HBase sink security support
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8365/
-----------------------------------------------------------
(Updated Dec. 6, 2012, 7:22 p.m.)
Review request for Flume.
Changes
-------
Incorporating Brock's feecback
Description
-------
Did some basic testing against secure and non-secure hbase. I will update this patch as I test. This is an initial patch only, changes are likely.
This addresses bug FLUME-1726.
https://issues.apache.org/jira/browse/FLUME-1726
Diffs (updated)
-----
flume-ng-sinks/flume-ng-hbase-sink/pom.xml 25422e1
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java 021ecd0
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 62f7097
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkSecurityManager.java PRE-CREATION
Diff: https://reviews.apache.org/r/8365/diff/
Testing
-------
Thanks,
Hari Shreedharan
Re: Review Request: FLUME-1726. HBase sink security support
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8365/#review14099
-----------------------------------------------------------
Looks quite good in general! A couple questions/comments below.
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java
<https://reviews.apache.org/r/8365/#comment30164>
unused variable
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java
<https://reviews.apache.org/r/8365/#comment30167>
Should the runPrivileged be outside the loop?
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java
<https://reviews.apache.org/r/8365/#comment30166>
Nit: This code will be somehwhat hot, debug statement could be done one when debug logging is enabled.
- Brock Noland
On Dec. 6, 2012, 7:48 a.m., Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8365/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2012, 7:48 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Did some basic testing against secure and non-secure hbase. I will update this patch as I test. This is an initial patch only, changes are likely.
>
>
> This addresses bug FLUME-1726.
> https://issues.apache.org/jira/browse/FLUME-1726
>
>
> Diffs
> -----
>
> flume-ng-sinks/flume-ng-hbase-sink/pom.xml 25422e1
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java 021ecd0
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 62f7097
> flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkSecurityManager.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/8365/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hari Shreedharan
>
>
Re: Review Request: FLUME-1726. HBase sink security support
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8365/
-----------------------------------------------------------
(Updated Dec. 6, 2012, 7:48 a.m.)
Review request for Flume.
Description
-------
Did some basic testing against secure and non-secure hbase. I will update this patch as I test. This is an initial patch only, changes are likely.
This addresses bug FLUME-1726.
https://issues.apache.org/jira/browse/FLUME-1726
Diffs (updated)
-----
flume-ng-sinks/flume-ng-hbase-sink/pom.xml 25422e1
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java 021ecd0
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkConfigurationConstants.java 62f7097
flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkSecurityManager.java PRE-CREATION
Diff: https://reviews.apache.org/r/8365/diff/
Testing
-------
Thanks,
Hari Shreedharan