You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Ashish Paliwal <pa...@gmail.com> on 2013/07/10 09:40:07 UTC
Review Request 12440: Fix for Flume-2109, with review comments incorporated
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12440/
-----------------------------------------------------------
Review request for Flume.
Bugs: FLUME-2109
https://issues.apache.org/jira/browse/FLUME-2109
Repository: flume-git
Description
-------
Flume patch to support HTTPS support. Review comments incorporated. Now only one mode shall be enabled either normal HTTP or HTTPS.
Diffs
-----
flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java c90f067
flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSourceConfigurationConstants.java f547e0f
flume-ng-core/src/test/java/org/apache/flume/source/http/TestHTTPSource.java 8952db3
flume-ng-doc/sphinx/FlumeUserGuide.rst 63cad21
Diff: https://reviews.apache.org/r/12440/diff/
Testing
-------
Unit test case added to verify SSL functionality.
Thanks,
Ashish Paliwal
Re: Review Request 12440: Fix for Flume-2109, with review comments incorporated
Posted by Ashish <pa...@gmail.com>.
Thanks Alex ! Shall fix these and upload new patch.
On Wed, Jul 17, 2013 at 11:15 PM, Alexander Alten-Lorenz <
alexander@cloudera.com> wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12440/
>
> flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java<https://reviews.apache.org/r/12440/diff/1/?file=319796#file319796line124> (Diff
> revision 1)
>
> public class HTTPSource extends AbstractSource implements
>
> 124
>
> Preconditions.checkArgument(sslPort != null && sslPort > 0, "SSL Port cannot be null or less than 0" );
>
> Whats about SSL port can't be < 0 or empty
>
>
>
> flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java<https://reviews.apache.org/r/12440/diff/1/?file=319796#file319796line127> (Diff
> revision 1)
>
> public class HTTPSource extends AbstractSource implements
>
> 127
>
> "Keystore is required for SSL Conifguration" );
>
> Whats about
> A keystore is required ....
>
> A keystore password is ....
>
>
> flume-ng-doc/sphinx/FlumeUserGuide.rst<https://reviews.apache.org/r/12440/diff/1/?file=319799#file319799line1228> (Diff
> revision 1)
>
> 1228
>
> keystore Location of the keystore includng keystore file name
>
> Spelling issue:
>
> includng = including
>
>
> - Alexander Alten-Lorenz
>
> On July 10th, 2013, 7:40 a.m. UTC, Ashish Paliwal wrote:
> Review request for Flume.
> By Ashish Paliwal.
>
> *Updated July 10, 2013, 7:40 a.m.*
> *Bugs: * FLUME-2109 <https://issues.apache.org/jira/browse/FLUME-2109>
> *Repository: * flume-git
> Description
>
> Flume patch to support HTTPS support. Review comments incorporated. Now only one mode shall be enabled either normal HTTP or HTTPS.
>
> Testing
>
> Unit test case added to verify SSL functionality.
>
> Diffs
>
> - flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java
> (c90f067)
> - flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSourceConfigurationConstants.java
> (f547e0f)
> - flume-ng-core/src/test/java/org/apache/flume/source/http/TestHTTPSource.java
> (8952db3)
> - flume-ng-doc/sphinx/FlumeUserGuide.rst (63cad21)
>
> View Diff <https://reviews.apache.org/r/12440/diff/>
>
--
thanks
ashish
Blog: http://www.ashishpaliwal.com/blog
My Photo Galleries: http://www.pbase.com/ashishpaliwal
Re: Review Request 12440: Fix for Flume-2109,
with review comments incorporated
Posted by Alexander Alten-Lorenz <al...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12440/#review23306
-----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java
<https://reviews.apache.org/r/12440/#comment47190>
Whats about SSL port can't be < 0 or empty
flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java
<https://reviews.apache.org/r/12440/#comment47191>
Whats about
A keystore is required ....
A keystore password is ....
flume-ng-doc/sphinx/FlumeUserGuide.rst
<https://reviews.apache.org/r/12440/#comment47182>
Spelling issue:
includng = including
- Alexander Alten-Lorenz
On July 10, 2013, 7:40 a.m., Ashish Paliwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12440/
> -----------------------------------------------------------
>
> (Updated July 10, 2013, 7:40 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2109
> https://issues.apache.org/jira/browse/FLUME-2109
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Flume patch to support HTTPS support. Review comments incorporated. Now only one mode shall be enabled either normal HTTP or HTTPS.
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java c90f067
> flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSourceConfigurationConstants.java f547e0f
> flume-ng-core/src/test/java/org/apache/flume/source/http/TestHTTPSource.java 8952db3
> flume-ng-doc/sphinx/FlumeUserGuide.rst 63cad21
>
> Diff: https://reviews.apache.org/r/12440/diff/
>
>
> Testing
> -------
>
> Unit test case added to verify SSL functionality.
>
>
> Thanks,
>
> Ashish Paliwal
>
>
Re: Review Request 12440: Fix for Flume-2109, with review comments incorporated
Posted by Ashish <pa...@gmail.com>.
Thanks Hari !
I shall upload the patch (minus dir spaces fix) with docs fixes. Would
inline doc with Avro Source wordings.
Appreciate your help.
On Thu, Jul 18, 2013 at 11:13 AM, Hari Shreedharan <
hshreedharan@cloudera.com> wrote:
> Thanks Ashish for the update. I will review the patch sometime soon, maybe
> early next week - sorry, swamped with other stuff. I recommend keeping the
> semantics the same as Avro Source (we can fix the dirs with sources issue
> later for both if that issue pops up).
>
>
> Hari
>
>
> Thanks,
> Hari
>
>
> On Wednesday, July 17, 2013 at 10:35 PM, Ashish wrote:
>
> > Alex,
> >
> > Quick comment, I tried with directory name with spaces on Mac, it worked.
> > Don't have access to Windows box to test this.
> > Also, if this is the case, then Avro SSL implementation would also break.
> > It uses the same constructs. Do you want me to fix this?
> >
> > There is one more missing thing, keystore type is not supported. Avro
> > Source support this. I shall add support for this as well.
> >
> > Most of the stuff is ready, shall publish the patch again for review.
> >
> > Thanks for this review marathon :)
> >
> >
> > On Thu, Jul 18, 2013 at 12:37 AM, Alexander Alten-Lorenz <
> > alexander@cloudera.com (mailto:alexander@cloudera.com)> wrote:
> >
> > > This is an automatically generated e-mail. To reply, visit:
> > > https://reviews.apache.org/r/12440/
> > >
> > >
> flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java<
> https://reviews.apache.org/r/12440/diff/1/?file=319796#file319796line125>
> (Diff
> > > revision 1)
> > >
> > > public class HTTPSource extends AbstractSource implements
> > >
> > > 125
> > >
> > > keyStorePath =
> context.getString(HTTPSourceConfigurationConstants.SSL_KEYSTORE);
> > >
> > > I think this could break under some circumstances a windows
> integration, ex. when a path is like:
> > > C:\WINNT\PATH SOMEWHERE\DIRECTORY WITH SPACES\
> > >
> > > You should workaround this.
> > >
> > >
> > > - Alexander Alten-Lorenz
> > >
> > > On July 10th, 2013, 7:40 a.m. UTC, Ashish Paliwal wrote:
> > > Review request for Flume.
> > > By Ashish Paliwal.
> > >
> > > *Updated July 10, 2013, 7:40 a.m.*
> > > *Bugs: * FLUME-2109 <https://issues.apache.org/jira/browse/FLUME-2109>
> > > *Repository: * flume-git
> > > Description
> > >
> > > Flume patch to support HTTPS support. Review comments incorporated.
> Now only one mode shall be enabled either normal HTTP or HTTPS.
> > >
> > > Testing
> > >
> > > Unit test case added to verify SSL functionality.
> > >
> > > Diffs
> > >
> > > -
> flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java
> > > (c90f067)
> > > -
> flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSourceConfigurationConstants.java
> > > (f547e0f)
> > > -
> flume-ng-core/src/test/java/org/apache/flume/source/http/TestHTTPSource.java
> > > (8952db3)
> > > - flume-ng-doc/sphinx/FlumeUserGuide.rst (63cad21)
> > >
> > > View Diff <https://reviews.apache.org/r/12440/diff/>
> >
> >
> >
> > --
> > thanks
> > ashish
> >
> > Blog: http://www.ashishpaliwal.com/blog
> > My Photo Galleries: http://www.pbase.com/ashishpaliwal
> >
> >
>
>
>
--
thanks
ashish
Blog: http://www.ashishpaliwal.com/blog
My Photo Galleries: http://www.pbase.com/ashishpaliwal
Re: Review Request 12440: Fix for Flume-2109, with review
comments incorporated
Posted by Hari Shreedharan <hs...@cloudera.com>.
Thanks Ashish for the update. I will review the patch sometime soon, maybe early next week - sorry, swamped with other stuff. I recommend keeping the semantics the same as Avro Source (we can fix the dirs with sources issue later for both if that issue pops up).
Hari
Thanks,
Hari
On Wednesday, July 17, 2013 at 10:35 PM, Ashish wrote:
> Alex,
>
> Quick comment, I tried with directory name with spaces on Mac, it worked.
> Don't have access to Windows box to test this.
> Also, if this is the case, then Avro SSL implementation would also break.
> It uses the same constructs. Do you want me to fix this?
>
> There is one more missing thing, keystore type is not supported. Avro
> Source support this. I shall add support for this as well.
>
> Most of the stuff is ready, shall publish the patch again for review.
>
> Thanks for this review marathon :)
>
>
> On Thu, Jul 18, 2013 at 12:37 AM, Alexander Alten-Lorenz <
> alexander@cloudera.com (mailto:alexander@cloudera.com)> wrote:
>
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/12440/
> >
> > flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java<https://reviews.apache.org/r/12440/diff/1/?file=319796#file319796line125> (Diff
> > revision 1)
> >
> > public class HTTPSource extends AbstractSource implements
> >
> > 125
> >
> > keyStorePath = context.getString(HTTPSourceConfigurationConstants.SSL_KEYSTORE);
> >
> > I think this could break under some circumstances a windows integration, ex. when a path is like:
> > C:\WINNT\PATH SOMEWHERE\DIRECTORY WITH SPACES\
> >
> > You should workaround this.
> >
> >
> > - Alexander Alten-Lorenz
> >
> > On July 10th, 2013, 7:40 a.m. UTC, Ashish Paliwal wrote:
> > Review request for Flume.
> > By Ashish Paliwal.
> >
> > *Updated July 10, 2013, 7:40 a.m.*
> > *Bugs: * FLUME-2109 <https://issues.apache.org/jira/browse/FLUME-2109>
> > *Repository: * flume-git
> > Description
> >
> > Flume patch to support HTTPS support. Review comments incorporated. Now only one mode shall be enabled either normal HTTP or HTTPS.
> >
> > Testing
> >
> > Unit test case added to verify SSL functionality.
> >
> > Diffs
> >
> > - flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java
> > (c90f067)
> > - flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSourceConfigurationConstants.java
> > (f547e0f)
> > - flume-ng-core/src/test/java/org/apache/flume/source/http/TestHTTPSource.java
> > (8952db3)
> > - flume-ng-doc/sphinx/FlumeUserGuide.rst (63cad21)
> >
> > View Diff <https://reviews.apache.org/r/12440/diff/>
>
>
>
> --
> thanks
> ashish
>
> Blog: http://www.ashishpaliwal.com/blog
> My Photo Galleries: http://www.pbase.com/ashishpaliwal
>
>
Re: Review Request 12440: Fix for Flume-2109, with review comments incorporated
Posted by Ashish <pa...@gmail.com>.
Alex,
Quick comment, I tried with directory name with spaces on Mac, it worked.
Don't have access to Windows box to test this.
Also, if this is the case, then Avro SSL implementation would also break.
It uses the same constructs. Do you want me to fix this?
There is one more missing thing, keystore type is not supported. Avro
Source support this. I shall add support for this as well.
Most of the stuff is ready, shall publish the patch again for review.
Thanks for this review marathon :)
On Thu, Jul 18, 2013 at 12:37 AM, Alexander Alten-Lorenz <
alexander@cloudera.com> wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12440/
>
> flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java<https://reviews.apache.org/r/12440/diff/1/?file=319796#file319796line125> (Diff
> revision 1)
>
> public class HTTPSource extends AbstractSource implements
>
> 125
>
> keyStorePath = context.getString(HTTPSourceConfigurationConstants.SSL_KEYSTORE);
>
> I think this could break under some circumstances a windows integration, ex. when a path is like:
> C:\WINNT\PATH SOMEWHERE\DIRECTORY WITH SPACES\
>
> You should workaround this.
>
>
> - Alexander Alten-Lorenz
>
> On July 10th, 2013, 7:40 a.m. UTC, Ashish Paliwal wrote:
> Review request for Flume.
> By Ashish Paliwal.
>
> *Updated July 10, 2013, 7:40 a.m.*
> *Bugs: * FLUME-2109 <https://issues.apache.org/jira/browse/FLUME-2109>
> *Repository: * flume-git
> Description
>
> Flume patch to support HTTPS support. Review comments incorporated. Now only one mode shall be enabled either normal HTTP or HTTPS.
>
> Testing
>
> Unit test case added to verify SSL functionality.
>
> Diffs
>
> - flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java
> (c90f067)
> - flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSourceConfigurationConstants.java
> (f547e0f)
> - flume-ng-core/src/test/java/org/apache/flume/source/http/TestHTTPSource.java
> (8952db3)
> - flume-ng-doc/sphinx/FlumeUserGuide.rst (63cad21)
>
> View Diff <https://reviews.apache.org/r/12440/diff/>
>
--
thanks
ashish
Blog: http://www.ashishpaliwal.com/blog
My Photo Galleries: http://www.pbase.com/ashishpaliwal
Re: Review Request 12440: Fix for Flume-2109,
with review comments incorporated
Posted by Alexander Alten-Lorenz <al...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12440/#review23326
-----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java
<https://reviews.apache.org/r/12440/#comment47207>
I think this could break under some circumstances a windows integration, ex. when a path is like:
C:\WINNT\PATH SOMEWHERE\DIRECTORY WITH SPACES\
You should workaround this.
- Alexander Alten-Lorenz
On July 10, 2013, 7:40 a.m., Ashish Paliwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12440/
> -----------------------------------------------------------
>
> (Updated July 10, 2013, 7:40 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2109
> https://issues.apache.org/jira/browse/FLUME-2109
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Flume patch to support HTTPS support. Review comments incorporated. Now only one mode shall be enabled either normal HTTP or HTTPS.
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java c90f067
> flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSourceConfigurationConstants.java f547e0f
> flume-ng-core/src/test/java/org/apache/flume/source/http/TestHTTPSource.java 8952db3
> flume-ng-doc/sphinx/FlumeUserGuide.rst 63cad21
>
> Diff: https://reviews.apache.org/r/12440/diff/
>
>
> Testing
> -------
>
> Unit test case added to verify SSL functionality.
>
>
> Thanks,
>
> Ashish Paliwal
>
>