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/22 09:56:09 UTC
Review Request 12803: Fix for Flume-2109,
Take 3. Incorporated Review comments and implementation in sysc with Avro
source
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12803/
-----------------------------------------------------------
Review request for Flume.
Bugs: FLUME-2109
https://issues.apache.org/jira/browse/FLUME-2109
Repository: flume-git
Description
-------
Fix for Flume-2109, to support HTTPS.
Incorporates Review comments from Alex
Implementation refined to be as close to Avro Source, like SSL port is not to be defined, the port value shall be used
Documentation updated to reflect the changes
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/12803/diff/
Testing
-------
Unit Test case written to test SSL functionality
Thanks,
Ashish Paliwal
Re: Review Request 12803: Fix for Flume-2109, Take 3. Incorporated
Review comments and implementation in sysc with Avro source
Posted by Ashish <pa...@gmail.com>.
Thanks !
On Fri, Aug 2, 2013 at 7:12 AM, Hari Shreedharan
<hs...@cloudera.com>wrote:
> Sounds good, I will commit this later today
>
>
> On Thu, Aug 1, 2013 at 6:40 PM, Ashish <pa...@gmail.com> wrote:
>
>> I shall create a JIRA to track it. This patch had too many iteration :)
>>
>> I shall look at the code coverage as well and that shall identify any
>> other missing paths.
>>
>> does it work?
>>
>>
>> On Fri, Aug 2, 2013 at 4:01 AM, Hari Shreedharan <
>> hshreedharan@cloudera.com> wrote:
>>
>>> This is an automatically generated e-mail. To reply, visit:
>>> https://reviews.apache.org/r/12803/
>>>
>>> Ashish,
>>>
>>> This looks ready to commit. I have a couple of requests - if you want me to commit this one, and complete the test in a follow-up jira, that is fine by me too.
>>>
>>>
>>>
>>> flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java<https://reviews.apache.org/r/12803/diff/1/?file=324467#file324467line116> (Diff
>>> revision 1)
>>>
>>> public class HTTPSource extends AbstractSource implements
>>>
>>> 115
>>>
>>> LOG.debug("SSL configuration enabled");
>>>
>>> We should probably log this at INFO level.
>>>
>>>
>>>
>>> flume-ng-core/src/test/java/org/apache/flume/source/http/TestHTTPSource.java<https://reviews.apache.org/r/12803/diff/1/?file=324469#file324469line300> (Diff
>>> revision 1)
>>>
>>> 296
>>>
>>> public void testHttps() throws Exception {
>>>
>>> Can you add one test where a non-SSL connection fails?
>>>
>>>
>>> - Hari Shreedharan
>>>
>>> On July 22nd, 2013, 7:56 a.m. UTC, Ashish Paliwal wrote:
>>> Review request for Flume.
>>> By Ashish Paliwal.
>>>
>>> *Updated July 22, 2013, 7:56 a.m.*
>>> *Bugs: * FLUME-2109 <https://issues.apache.org/jira/browse/FLUME-2109>
>>> *Repository: * flume-git
>>> Description
>>>
>>> Fix for Flume-2109, to support HTTPS.
>>> Incorporates Review comments from Alex
>>> Implementation refined to be as close to Avro Source, like SSL port is not to be defined, the port value shall be used
>>> Documentation updated to reflect the changes
>>>
>>> Testing
>>>
>>> Unit Test case written to test 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/12803/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 12803: Fix for Flume-2109, Take 3. Incorporated
Review comments and implementation in sysc with Avro source
Posted by Hari Shreedharan <hs...@cloudera.com>.
Sounds good, I will commit this later today
On Thu, Aug 1, 2013 at 6:40 PM, Ashish <pa...@gmail.com> wrote:
> I shall create a JIRA to track it. This patch had too many iteration :)
>
> I shall look at the code coverage as well and that shall identify any
> other missing paths.
>
> does it work?
>
>
> On Fri, Aug 2, 2013 at 4:01 AM, Hari Shreedharan <
> hshreedharan@cloudera.com> wrote:
>
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/12803/
>>
>> Ashish,
>>
>> This looks ready to commit. I have a couple of requests - if you want me to commit this one, and complete the test in a follow-up jira, that is fine by me too.
>>
>>
>>
>> flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java<https://reviews.apache.org/r/12803/diff/1/?file=324467#file324467line116> (Diff
>> revision 1)
>>
>> public class HTTPSource extends AbstractSource implements
>>
>> 115
>>
>> LOG.debug("SSL configuration enabled");
>>
>> We should probably log this at INFO level.
>>
>>
>>
>> flume-ng-core/src/test/java/org/apache/flume/source/http/TestHTTPSource.java<https://reviews.apache.org/r/12803/diff/1/?file=324469#file324469line300> (Diff
>> revision 1)
>>
>> 296
>>
>> public void testHttps() throws Exception {
>>
>> Can you add one test where a non-SSL connection fails?
>>
>>
>> - Hari Shreedharan
>>
>> On July 22nd, 2013, 7:56 a.m. UTC, Ashish Paliwal wrote:
>> Review request for Flume.
>> By Ashish Paliwal.
>>
>> *Updated July 22, 2013, 7:56 a.m.*
>> *Bugs: * FLUME-2109 <https://issues.apache.org/jira/browse/FLUME-2109>
>> *Repository: * flume-git
>> Description
>>
>> Fix for Flume-2109, to support HTTPS.
>> Incorporates Review comments from Alex
>> Implementation refined to be as close to Avro Source, like SSL port is not to be defined, the port value shall be used
>> Documentation updated to reflect the changes
>>
>> Testing
>>
>> Unit Test case written to test 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/12803/diff/>
>>
>
>
>
> --
> thanks
> ashish
>
> Blog: http://www.ashishpaliwal.com/blog
> My Photo Galleries: http://www.pbase.com/ashishpaliwal
>
Re: Review Request 12803: Fix for Flume-2109, Take 3. Incorporated
Review comments and implementation in sysc with Avro source
Posted by Ashish <pa...@gmail.com>.
I shall create a JIRA to track it. This patch had too many iteration :)
I shall look at the code coverage as well and that shall identify any other
missing paths.
does it work?
On Fri, Aug 2, 2013 at 4:01 AM, Hari Shreedharan
<hs...@cloudera.com>wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12803/
>
> Ashish,
>
> This looks ready to commit. I have a couple of requests - if you want me to commit this one, and complete the test in a follow-up jira, that is fine by me too.
>
>
>
> flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java<https://reviews.apache.org/r/12803/diff/1/?file=324467#file324467line116> (Diff
> revision 1)
>
> public class HTTPSource extends AbstractSource implements
>
> 115
>
> LOG.debug("SSL configuration enabled");
>
> We should probably log this at INFO level.
>
>
>
> flume-ng-core/src/test/java/org/apache/flume/source/http/TestHTTPSource.java<https://reviews.apache.org/r/12803/diff/1/?file=324469#file324469line300> (Diff
> revision 1)
>
> 296
>
> public void testHttps() throws Exception {
>
> Can you add one test where a non-SSL connection fails?
>
>
> - Hari Shreedharan
>
> On July 22nd, 2013, 7:56 a.m. UTC, Ashish Paliwal wrote:
> Review request for Flume.
> By Ashish Paliwal.
>
> *Updated July 22, 2013, 7:56 a.m.*
> *Bugs: * FLUME-2109 <https://issues.apache.org/jira/browse/FLUME-2109>
> *Repository: * flume-git
> Description
>
> Fix for Flume-2109, to support HTTPS.
> Incorporates Review comments from Alex
> Implementation refined to be as close to Avro Source, like SSL port is not to be defined, the port value shall be used
> Documentation updated to reflect the changes
>
> Testing
>
> Unit Test case written to test 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/12803/diff/>
>
--
thanks
ashish
Blog: http://www.ashishpaliwal.com/blog
My Photo Galleries: http://www.pbase.com/ashishpaliwal
Re: Review Request 12803: Fix for Flume-2109,
Take 3. Incorporated Review comments and implementation in sysc with Avro
source
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12803/#review24484
-----------------------------------------------------------
Ashish,
This looks ready to commit. I have a couple of requests - if you want me to commit this one, and complete the test in a follow-up jira, that is fine by me too.
flume-ng-core/src/main/java/org/apache/flume/source/http/HTTPSource.java
<https://reviews.apache.org/r/12803/#comment48424>
We should probably log this at INFO level.
flume-ng-core/src/test/java/org/apache/flume/source/http/TestHTTPSource.java
<https://reviews.apache.org/r/12803/#comment48428>
Can you add one test where a non-SSL connection fails?
- Hari Shreedharan
On July 22, 2013, 7:56 a.m., Ashish Paliwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12803/
> -----------------------------------------------------------
>
> (Updated July 22, 2013, 7:56 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2109
> https://issues.apache.org/jira/browse/FLUME-2109
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Fix for Flume-2109, to support HTTPS.
> Incorporates Review comments from Alex
> Implementation refined to be as close to Avro Source, like SSL port is not to be defined, the port value shall be used
> Documentation updated to reflect the changes
>
>
> 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/12803/diff/
>
>
> Testing
> -------
>
> Unit Test case written to test SSL functionality
>
>
> Thanks,
>
> Ashish Paliwal
>
>
Re: Review Request 12803: Fix for Flume-2109, Take 3. Incorporated
Review comments and implementation in sysc with Avro source
Posted by Ashish <pa...@gmail.com>.
On Fri, Aug 2, 2013 at 10:52 AM, Hari Shreedharan <hshreedharan@cloudera.com
> wrote:
> Sorry if I confused you. I meant the patch for FLUME-2109 which you
> already completed (the one from reviewboard - not the one for the jira I
> filed - that one is completely upto you) ;)
>
Got it. I missed attaching it, damn my memory. I was under the impression
that I attached it.
Thanks for catching. The patch is uploaded, also please take care of the
jettykeystore (attached to keystore)
>
>
> Thanks,
> Hari
>
> On Thursday, August 1, 2013 at 10:14 PM, Ashish wrote:
>
>
>
>
> On Fri, Aug 2, 2013 at 10:42 AM, Hari Shreedharan <
> hshreedharan@cloudera.com> wrote:
>
> Ashish,
>
> Please attach the patch to the jira. I will commit it.
>
> Hari
>
>
> Thanks,
> Hari
>
>
> Thanks Hari ! Shall do it. Just starting my day here, shall try to kill it
> during weekend.
>
> thanks
> ashish
>
>
>
--
thanks
ashish
Blog: http://www.ashishpaliwal.com/blog
My Photo Galleries: http://www.pbase.com/ashishpaliwal
Re: Review Request 12803: Fix for Flume-2109, Take 3.
Incorporated Review comments and implementation in sysc with Avro
source
Posted by Hari Shreedharan <hs...@cloudera.com>.
Sorry if I confused you. I meant the patch for FLUME-2109 which you already completed (the one from reviewboard - not the one for the jira I filed - that one is completely upto you) ;)
Thanks,
Hari
On Thursday, August 1, 2013 at 10:14 PM, Ashish wrote:
>
>
>
> On Fri, Aug 2, 2013 at 10:42 AM, Hari Shreedharan <hshreedharan@cloudera.com (mailto:hshreedharan@cloudera.com)> wrote:
> > Ashish,
> >
> > Please attach the patch to the jira. I will commit it.
> >
> > Hari
> >
> >
> > Thanks,
> > Hari
> >
>
> Thanks Hari ! Shall do it. Just starting my day here, shall try to kill it during weekend.
>
> thanks
> ashish
>
>
>
>
>
Re: Review Request 12803: Fix for Flume-2109, Take 3. Incorporated
Review comments and implementation in sysc with Avro source
Posted by Ashish <pa...@gmail.com>.
On Fri, Aug 2, 2013 at 10:42 AM, Hari Shreedharan <hshreedharan@cloudera.com
> wrote:
> Ashish,
>
> Please attach the patch to the jira. I will commit it.
>
> Hari
>
>
> Thanks,
> Hari
>
>
Thanks Hari ! Shall do it. Just starting my day here, shall try to kill it
during weekend.
thanks
ashish
Re: Review Request 12803: Fix for Flume-2109, Take 3.
Incorporated Review comments and implementation in sysc with Avro
source
Posted by Hari Shreedharan <hs...@cloudera.com>.
Ashish,
Please attach the patch to the jira. I will commit it.
Hari
Thanks,
Hari
On Thursday, August 1, 2013 at 7:05 PM, Hari Shreedharan wrote:
> This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12803/
>
>
>
>
> Ship it!
> As discussed, please file a follow up jira to add a new test. please update the log message to INFO and attach the patch to the jira.
>
> - Hari Shreedharan
> On July 22nd, 2013, 7:56 a.m. UTC, Ashish Paliwal wrote:
> Review request for Flume.
> By Ashish Paliwal.
>
> Updated July 22, 2013, 7:56 a.m.
>
> Bugs: FLUME-2109 (https://issues.apache.org/jira/browse/FLUME-2109)
> Repository: flume-git
> Description
> Fix for Flume-2109, to support HTTPS. Incorporates Review comments from Alex Implementation refined to be as close to Avro Source, like SSL port is not to be defined, the port value shall be used Documentation updated to reflect the changes
>
>
>
>
> Testing
> Unit Test case written to test 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/12803/diff/)
>
>
>
>
>
>
>
Re: Review Request 12803: Fix for Flume-2109,
Take 3. Incorporated Review comments and implementation in sysc with Avro
source
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12803/#review24501
-----------------------------------------------------------
Ship it!
As discussed, please file a follow up jira to add a new test. please update the log message to INFO and attach the patch to the jira.
- Hari Shreedharan
On July 22, 2013, 7:56 a.m., Ashish Paliwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12803/
> -----------------------------------------------------------
>
> (Updated July 22, 2013, 7:56 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2109
> https://issues.apache.org/jira/browse/FLUME-2109
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Fix for Flume-2109, to support HTTPS.
> Incorporates Review comments from Alex
> Implementation refined to be as close to Avro Source, like SSL port is not to be defined, the port value shall be used
> Documentation updated to reflect the changes
>
>
> 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/12803/diff/
>
>
> Testing
> -------
>
> Unit Test case written to test SSL functionality
>
>
> Thanks,
>
> Ashish Paliwal
>
>