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/02/16 08:42:40 UTC
Review Request: Log4jAppender using AvroSourceProtocol
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3924/
-----------------------------------------------------------
Review request for Flume.
Summary
-------
A class which takes in a LoggingEvent and sends it to a predefined AvroSource at a given host and port.
This addresses bug FLUME-886.
https://issues.apache.org/jira/browse/FLUME-886
Diffs
-----
branches/flume-728/flume-ng-log4jappender/pom.xml PRE-CREATION
branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAppender.java PRE-CREATION
branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAvroHeaders.java PRE-CREATION
branches/flume-728/flume-ng-log4jappender/src/test/java/org/apache/flume/log4jappender/TestLog4jAppender.java PRE-CREATION
branches/flume-728/flume-ng-log4jappender/src/test/resources/log4j.properties PRE-CREATION
branches/flume-728/pom.xml 1244858
Diff: https://reviews.apache.org/r/3924/diff
Testing
-------
Added unit tests.
Thanks,
Hari
Re: Review Request: Log4jAppender using AvroSourceProtocol
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3924/#review5165
-----------------------------------------------------------
Ship it!
- Brock
On 2012-02-16 18:50:02, Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3924/
> -----------------------------------------------------------
>
> (Updated 2012-02-16 18:50:02)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> A class which takes in a LoggingEvent and sends it to a predefined AvroSource at a given host and port.
>
>
> This addresses bug FLUME-886.
> https://issues.apache.org/jira/browse/FLUME-886
>
>
> Diffs
> -----
>
> branches/flume-728/flume-ng-log4jappender/pom.xml PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAvroHeaders.java PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/test/java/org/apache/flume/log4jappender/TestLog4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/test/resources/log4j.properties PRE-CREATION
> branches/flume-728/pom.xml 1244858
>
> Diff: https://reviews.apache.org/r/3924/diff
>
>
> Testing
> -------
>
> Added unit tests.
>
>
> Thanks,
>
> Hari
>
>
Re: Review Request: Log4jAppender using AvroSourceProtocol
Posted by Hari Shreedharan <hs...@cloudera.com>.
Thanks Ralph. Now I understand what you were mentioning in the previous comment. I incorporate the changes in my patch, and thanks for the code as well, really helpful.
Thanks!
Hari
--
Hari Shreedharan
On Sunday, February 19, 2012 at 2:06 PM, Ralph Goers wrote:
> Nice - I didn't realize that your email was generated by Jira. I'll repost my comment there.
>
> Ralph
>
>
> On Feb 19, 2012, at 2:02 PM, Ralph Goers wrote:
>
> >
> > On Feb 19, 2012, at 11:20 AM, Hari Shreedharan wrote:
> >
> > >
> > >
> > > > On 2012-02-19 16:30:08, Ralph Goers wrote:
> > > > > I have a suspicion that if you actually try to use this from an application via a configuration file you won't be able to. I don't see any setter methods for the host and port.
> > > >
> > > >
> > >
> > >
> > > The reason I didn't put setters for host and port are that the AvroClient object can be created only if know the hostname and port. So the constructor needs these arguments. If the hostname and port change because the config changes, the application can simply create a new object of the Log4jAppender(resetting the port and hostname using setters will have the same effect - since internally we would be doing the same thing, by replacing the protocolClient). For example this would be how the setter would look:
> > >
> > > public synchronized void setHostnameAndPort(String hostname, int port){
> > > this.hostname = hostname;
> > > this.port = port
> > > protocolClient = null; //This basically to make sure a new AvroClient is created, considering the new setting.
> > > }
> > >
> >
> >
> >
> > I think you are missing the point. Have you tried creating a log4j.xml or log4j.properties and declaring the FlumeAvroAppender in it to see if it will work? The vast majority of Log4j users do not programmatically declare their appenders. To require that makes the Appender almost useless.
> >
> > FWIW, I've written a Flume appender (for OG) for Logback for my employer but I haven't published it anywhere. I've also written a FlumeAvroAppender for Log4j 2 for both OG and NG. See http://people.apache.org/~rgoers/log4j2/manual/appenders.html#FlumeAvroAppender. Feel free to review the code - the NG source is at https://svn.apache.org/repos/asf/logging/log4j/log4j2/trunk/log4j2-flume-ng/. Any suggestions you have would be most welcome.
> >
> > Ralph
Re: Review Request: Log4jAppender using AvroSourceProtocol
Posted by Ralph Goers <ra...@dslextreme.com>.
Nice - I didn't realize that your email was generated by Jira. I'll repost my comment there.
Ralph
On Feb 19, 2012, at 2:02 PM, Ralph Goers wrote:
>
> On Feb 19, 2012, at 11:20 AM, Hari Shreedharan wrote:
>
>>
>>
>>> On 2012-02-19 16:30:08, Ralph Goers wrote:
>>>> I have a suspicion that if you actually try to use this from an application via a configuration file you won't be able to. I don't see any setter methods for the host and port.
>>
>> The reason I didn't put setters for host and port are that the AvroClient object can be created only if know the hostname and port. So the constructor needs these arguments. If the hostname and port change because the config changes, the application can simply create a new object of the Log4jAppender(resetting the port and hostname using setters will have the same effect - since internally we would be doing the same thing, by replacing the protocolClient). For example this would be how the setter would look:
>>
>> public synchronized void setHostnameAndPort(String hostname, int port){
>> this.hostname = hostname;
>> this.port = port
>> protocolClient = null; //This basically to make sure a new AvroClient is created, considering the new setting.
>> }
>
>
> I think you are missing the point. Have you tried creating a log4j.xml or log4j.properties and declaring the FlumeAvroAppender in it to see if it will work? The vast majority of Log4j users do not programmatically declare their appenders. To require that makes the Appender almost useless.
>
> FWIW, I've written a Flume appender (for OG) for Logback for my employer but I haven't published it anywhere. I've also written a FlumeAvroAppender for Log4j 2 for both OG and NG. See http://people.apache.org/~rgoers/log4j2/manual/appenders.html#FlumeAvroAppender. Feel free to review the code - the NG source is at https://svn.apache.org/repos/asf/logging/log4j/log4j2/trunk/log4j2-flume-ng/. Any suggestions you have would be most welcome.
>
> Ralph
Re: Review Request: Log4jAppender using AvroSourceProtocol
Posted by Ralph Goers <rg...@apache.org>.
On Feb 19, 2012, at 11:20 AM, Hari Shreedharan wrote:
>
>
>> On 2012-02-19 16:30:08, Ralph Goers wrote:
>>> I have a suspicion that if you actually try to use this from an application via a configuration file you won't be able to. I don't see any setter methods for the host and port.
>
> The reason I didn't put setters for host and port are that the AvroClient object can be created only if know the hostname and port. So the constructor needs these arguments. If the hostname and port change because the config changes, the application can simply create a new object of the Log4jAppender(resetting the port and hostname using setters will have the same effect - since internally we would be doing the same thing, by replacing the protocolClient). For example this would be how the setter would look:
>
> public synchronized void setHostnameAndPort(String hostname, int port){
> this.hostname = hostname;
> this.port = port
> protocolClient = null; //This basically to make sure a new AvroClient is created, considering the new setting.
> }
I think you are missing the point. Have you tried creating a log4j.xml or log4j.properties and declaring the FlumeAvroAppender in it to see if it will work? The vast majority of Log4j users do not programmatically declare their appenders. To require that makes the Appender almost useless.
FWIW, I've written a Flume appender (for OG) for Logback for my employer but I haven't published it anywhere. I've also written a FlumeAvroAppender for Log4j 2 for both OG and NG. See http://people.apache.org/~rgoers/log4j2/manual/appenders.html#FlumeAvroAppender. Feel free to review the code - the NG source is at https://svn.apache.org/repos/asf/logging/log4j/log4j2/trunk/log4j2-flume-ng/. Any suggestions you have would be most welcome.
Ralph
Re: Review Request: Log4jAppender using AvroSourceProtocol
Posted by Hari Shreedharan <hs...@cloudera.com>.
> On 2012-02-19 16:30:08, Ralph Goers wrote:
> > I have a suspicion that if you actually try to use this from an application via a configuration file you won't be able to. I don't see any setter methods for the host and port.
The reason I didn't put setters for host and port are that the AvroClient object can be created only if know the hostname and port. So the constructor needs these arguments. If the hostname and port change because the config changes, the application can simply create a new object of the Log4jAppender(resetting the port and hostname using setters will have the same effect - since internally we would be doing the same thing, by replacing the protocolClient). For example this would be how the setter would look:
public synchronized void setHostnameAndPort(String hostname, int port){
this.hostname = hostname;
this.port = port
protocolClient = null; //This basically to make sure a new AvroClient is created, considering the new setting.
}
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3924/#review5211
-----------------------------------------------------------
On 2012-02-16 18:50:02, Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3924/
> -----------------------------------------------------------
>
> (Updated 2012-02-16 18:50:02)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> A class which takes in a LoggingEvent and sends it to a predefined AvroSource at a given host and port.
>
>
> This addresses bug FLUME-886.
> https://issues.apache.org/jira/browse/FLUME-886
>
>
> Diffs
> -----
>
> branches/flume-728/flume-ng-log4jappender/pom.xml PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAvroHeaders.java PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/test/java/org/apache/flume/log4jappender/TestLog4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/test/resources/log4j.properties PRE-CREATION
> branches/flume-728/pom.xml 1244858
>
> Diff: https://reviews.apache.org/r/3924/diff
>
>
> Testing
> -------
>
> Added unit tests.
>
>
> Thanks,
>
> Hari
>
>
Re: Review Request: Log4jAppender using AvroSourceProtocol
Posted by Ralph Goers <rg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3924/#review5211
-----------------------------------------------------------
I have a suspicion that if you actually try to use this from an application via a configuration file you won't be able to. I don't see any setter methods for the host and port.
- Ralph
On 2012-02-16 18:50:02, Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3924/
> -----------------------------------------------------------
>
> (Updated 2012-02-16 18:50:02)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> A class which takes in a LoggingEvent and sends it to a predefined AvroSource at a given host and port.
>
>
> This addresses bug FLUME-886.
> https://issues.apache.org/jira/browse/FLUME-886
>
>
> Diffs
> -----
>
> branches/flume-728/flume-ng-log4jappender/pom.xml PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAvroHeaders.java PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/test/java/org/apache/flume/log4jappender/TestLog4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/test/resources/log4j.properties PRE-CREATION
> branches/flume-728/pom.xml 1244858
>
> Diff: https://reviews.apache.org/r/3924/diff
>
>
> Testing
> -------
>
> Added unit tests.
>
>
> Thanks,
>
> Hari
>
>
Re: Review Request: Log4jAppender using AvroSourceProtocol
Posted by Hari Shreedharan <hs...@cloudera.com>.
> On 2012-02-21 00:29:14, Arvind Prabhakar wrote:
> > branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/main/java/org/apache/flume/clients/log4jappender/Log4jAppender.java, lines 106-109
> > <https://reviews.apache.org/r/3924/diff/5/?file=76163#file76163line106>
> >
> > Not sure if this is a valid assumption since the first line of the append() method raises an exception if the protocol client is null.
> >
> > Also, if we must dispose the protocol client, the underlying transceiver should be closed as well in order to ensure there are no resource leaks.
Hi Arvind,
Thanks for the feedback. If close() is called on the appender, the protocolClient is set to null, which is what I meant to do. This is to ensure that a future append cannot happen on a closed appender. I think the comment was outdated, and a call to append no longer creates a new client, but throws an exception. Sorry about that. If append is called on the appender after close(), it throws an Exception which I think is a valid case. I think it is also valid to throw an exception, if append is called before the activeOptions() function is called(in which case the protocolClient is null). Both these cases exist only when the appender is used programatically, not through a config file.
I will close the transceiver in the close function, to make sure the resources are released immediately once the close function is called
Just a note: even without this the resources would be cleared off by Java garbage collector since the protocolClient reference is no longer valid, hence all internal members of the client will be cleared, since no references to them exist anywhere outside.
> On 2012-02-21 00:29:14, Arvind Prabhakar wrote:
> > branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/main/java/org/apache/flume/clients/log4jappender/Log4jAppender.java, line 40
> > <https://reviews.apache.org/r/3924/diff/5/?file=76163#file76163line40>
> >
> > A brief javadoc will be good that describes this implementation.
Sure.
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3924/#review5226
-----------------------------------------------------------
On 2012-02-20 22:54:11, Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3924/
> -----------------------------------------------------------
>
> (Updated 2012-02-20 22:54:11)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> A class which takes in a LoggingEvent and sends it to a predefined AvroSource at a given host and port.
>
>
> This addresses bug FLUME-886.
> https://issues.apache.org/jira/browse/FLUME-886
>
>
> Diffs
> -----
>
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/pom.xml PRE-CREATION
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/main/java/org/apache/flume/clients/log4jappender/Log4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/main/java/org/apache/flume/clients/log4jappender/Log4jAvroHeaders.java PRE-CREATION
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/test/java/org/apache/flume/clients/log4jappender/TestLog4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/test/resources/flume-log4jtest.properties PRE-CREATION
> branches/flume-728/flume-ng-clients/pom.xml PRE-CREATION
> branches/flume-728/pom.xml 1244858
>
> Diff: https://reviews.apache.org/r/3924/diff
>
>
> Testing
> -------
>
> Added unit tests.
>
>
> Thanks,
>
> Hari
>
>
Re: Review Request: Log4jAppender using AvroSourceProtocol
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3924/#review5226
-----------------------------------------------------------
Thanks for incorporating the feedback Hari. Change looks good, a couple of minor suggestions follow.
branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/main/java/org/apache/flume/clients/log4jappender/Log4jAppender.java
<https://reviews.apache.org/r/3924/#comment11424>
A brief javadoc will be good that describes this implementation.
branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/main/java/org/apache/flume/clients/log4jappender/Log4jAppender.java
<https://reviews.apache.org/r/3924/#comment11423>
Not sure if this is a valid assumption since the first line of the append() method raises an exception if the protocol client is null.
Also, if we must dispose the protocol client, the underlying transceiver should be closed as well in order to ensure there are no resource leaks.
- Arvind
On 2012-02-20 22:54:11, Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3924/
> -----------------------------------------------------------
>
> (Updated 2012-02-20 22:54:11)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> A class which takes in a LoggingEvent and sends it to a predefined AvroSource at a given host and port.
>
>
> This addresses bug FLUME-886.
> https://issues.apache.org/jira/browse/FLUME-886
>
>
> Diffs
> -----
>
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/pom.xml PRE-CREATION
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/main/java/org/apache/flume/clients/log4jappender/Log4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/main/java/org/apache/flume/clients/log4jappender/Log4jAvroHeaders.java PRE-CREATION
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/test/java/org/apache/flume/clients/log4jappender/TestLog4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/test/resources/flume-log4jtest.properties PRE-CREATION
> branches/flume-728/flume-ng-clients/pom.xml PRE-CREATION
> branches/flume-728/pom.xml 1244858
>
> Diff: https://reviews.apache.org/r/3924/diff
>
>
> Testing
> -------
>
> Added unit tests.
>
>
> Thanks,
>
> Hari
>
>
Re: Review Request: Log4jAppender using AvroSourceProtocol
Posted by Ralph Goers <rg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3924/#review5233
-----------------------------------------------------------
Ship it!
Looks good to me.
- Ralph
On 2012-02-21 02:53:41, Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3924/
> -----------------------------------------------------------
>
> (Updated 2012-02-21 02:53:41)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> A class which takes in a LoggingEvent and sends it to a predefined AvroSource at a given host and port.
>
>
> This addresses bug FLUME-886.
> https://issues.apache.org/jira/browse/FLUME-886
>
>
> Diffs
> -----
>
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/pom.xml PRE-CREATION
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/main/java/org/apache/flume/clients/log4jappender/Log4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/main/java/org/apache/flume/clients/log4jappender/Log4jAvroHeaders.java PRE-CREATION
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/test/java/org/apache/flume/clients/log4jappender/TestLog4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/test/resources/flume-log4jtest.properties PRE-CREATION
> branches/flume-728/flume-ng-clients/pom.xml PRE-CREATION
> branches/flume-728/pom.xml 1244858
>
> Diff: https://reviews.apache.org/r/3924/diff
>
>
> Testing
> -------
>
> Added unit tests.
>
>
> Thanks,
>
> Hari
>
>
Re: Review Request: Log4jAppender using AvroSourceProtocol
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3924/#review5231
-----------------------------------------------------------
Ship it!
+1
- Arvind
On 2012-02-21 02:53:41, Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3924/
> -----------------------------------------------------------
>
> (Updated 2012-02-21 02:53:41)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> A class which takes in a LoggingEvent and sends it to a predefined AvroSource at a given host and port.
>
>
> This addresses bug FLUME-886.
> https://issues.apache.org/jira/browse/FLUME-886
>
>
> Diffs
> -----
>
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/pom.xml PRE-CREATION
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/main/java/org/apache/flume/clients/log4jappender/Log4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/main/java/org/apache/flume/clients/log4jappender/Log4jAvroHeaders.java PRE-CREATION
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/test/java/org/apache/flume/clients/log4jappender/TestLog4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/test/resources/flume-log4jtest.properties PRE-CREATION
> branches/flume-728/flume-ng-clients/pom.xml PRE-CREATION
> branches/flume-728/pom.xml 1244858
>
> Diff: https://reviews.apache.org/r/3924/diff
>
>
> Testing
> -------
>
> Added unit tests.
>
>
> Thanks,
>
> Hari
>
>
Re: Review Request: Log4jAppender using AvroSourceProtocol
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3924/
-----------------------------------------------------------
(Updated 2012-02-21 02:53:41.556678)
Review request for Flume.
Changes
-------
Indentation correction.
Summary
-------
A class which takes in a LoggingEvent and sends it to a predefined AvroSource at a given host and port.
This addresses bug FLUME-886.
https://issues.apache.org/jira/browse/FLUME-886
Diffs (updated)
-----
branches/flume-728/flume-ng-clients/flume-ng-log4jappender/pom.xml PRE-CREATION
branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/main/java/org/apache/flume/clients/log4jappender/Log4jAppender.java PRE-CREATION
branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/main/java/org/apache/flume/clients/log4jappender/Log4jAvroHeaders.java PRE-CREATION
branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/test/java/org/apache/flume/clients/log4jappender/TestLog4jAppender.java PRE-CREATION
branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/test/resources/flume-log4jtest.properties PRE-CREATION
branches/flume-728/flume-ng-clients/pom.xml PRE-CREATION
branches/flume-728/pom.xml 1244858
Diff: https://reviews.apache.org/r/3924/diff
Testing
-------
Added unit tests.
Thanks,
Hari
Re: Review Request: Log4jAppender using AvroSourceProtocol
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3924/
-----------------------------------------------------------
(Updated 2012-02-21 02:51:54.629181)
Review request for Flume.
Changes
-------
Javadocs added. Close off transceiver in close function.
Summary
-------
A class which takes in a LoggingEvent and sends it to a predefined AvroSource at a given host and port.
This addresses bug FLUME-886.
https://issues.apache.org/jira/browse/FLUME-886
Diffs (updated)
-----
branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/test/java/org/apache/flume/clients/log4jappender/TestLog4jAppender.java PRE-CREATION
branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/main/java/org/apache/flume/clients/log4jappender/Log4jAvroHeaders.java PRE-CREATION
branches/flume-728/flume-ng-clients/flume-ng-log4jappender/pom.xml PRE-CREATION
branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/main/java/org/apache/flume/clients/log4jappender/Log4jAppender.java PRE-CREATION
branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/test/resources/flume-log4jtest.properties PRE-CREATION
branches/flume-728/flume-ng-clients/pom.xml PRE-CREATION
branches/flume-728/pom.xml 1244858
Diff: https://reviews.apache.org/r/3924/diff
Testing
-------
Added unit tests.
Thanks,
Hari
Re: Review Request: Log4jAppender using AvroSourceProtocol
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3924/
-----------------------------------------------------------
(Updated 2012-02-20 22:54:11.791741)
Review request for Flume.
Changes
-------
Incorporated feedback from Arvind and Ralph.
Summary
-------
A class which takes in a LoggingEvent and sends it to a predefined AvroSource at a given host and port.
This addresses bug FLUME-886.
https://issues.apache.org/jira/browse/FLUME-886
Diffs (updated)
-----
branches/flume-728/flume-ng-clients/flume-ng-log4jappender/pom.xml PRE-CREATION
branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/main/java/org/apache/flume/clients/log4jappender/Log4jAppender.java PRE-CREATION
branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/main/java/org/apache/flume/clients/log4jappender/Log4jAvroHeaders.java PRE-CREATION
branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/test/java/org/apache/flume/clients/log4jappender/TestLog4jAppender.java PRE-CREATION
branches/flume-728/flume-ng-clients/flume-ng-log4jappender/src/test/resources/flume-log4jtest.properties PRE-CREATION
branches/flume-728/flume-ng-clients/pom.xml PRE-CREATION
branches/flume-728/pom.xml 1244858
Diff: https://reviews.apache.org/r/3924/diff
Testing
-------
Added unit tests.
Thanks,
Hari
Re: Review Request: Log4jAppender using AvroSourceProtocol
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3924/#review5213
-----------------------------------------------------------
Hari - thanks for the patch. Some feedback follows:
* Please do not use tabs anywhere. Instead, please use two spaces for indent where you are otherwise using a tab.
* Please remove any trailing white spaces from the sources. These are highlighted with red color in the review board diff view.
* Since this is one of the many clients that we would like to build, I suggest that you move it under a top-level module called flume-ng-clients. That way the root directory will not end up containing a lot of client modules.
branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAppender.java
<https://reviews.apache.org/r/3924/#comment11406>
This initialization should ideally happen in the overridden activateOptions() method of the appender. Also, as pointed out by Ralph, the configuration will need to be injected via setter methods, so please add them.
One thing that I am not too sure about is how the NettyTransceiver is supposed to be used in a multi-threaded environment. It will be good to find that out before making the assumption that it is safe for such use.
branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAvroHeaders.java
<https://reviews.apache.org/r/3924/#comment11405>
Since this appender is part of Flume distribution, the header names should be in flume's private namespace. For example - LOGGER_NAME should be "flume.client.log4j.logger.name".
You can do this by having providing these standard names as part of the enum state. Also, it is customary to have the first enum be OTHER which ensures that the ordinal for unknown headers will always remain 0.
- Arvind
On 2012-02-16 18:50:02, Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3924/
> -----------------------------------------------------------
>
> (Updated 2012-02-16 18:50:02)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> A class which takes in a LoggingEvent and sends it to a predefined AvroSource at a given host and port.
>
>
> This addresses bug FLUME-886.
> https://issues.apache.org/jira/browse/FLUME-886
>
>
> Diffs
> -----
>
> branches/flume-728/flume-ng-log4jappender/pom.xml PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAvroHeaders.java PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/test/java/org/apache/flume/log4jappender/TestLog4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/test/resources/log4j.properties PRE-CREATION
> branches/flume-728/pom.xml 1244858
>
> Diff: https://reviews.apache.org/r/3924/diff
>
>
> Testing
> -------
>
> Added unit tests.
>
>
> Thanks,
>
> Hari
>
>
Re: Review Request: Log4jAppender using AvroSourceProtocol
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3924/
-----------------------------------------------------------
(Updated 2012-02-16 18:50:02.817429)
Review request for Flume.
Changes
-------
Passing exception down.
Summary
-------
A class which takes in a LoggingEvent and sends it to a predefined AvroSource at a given host and port.
This addresses bug FLUME-886.
https://issues.apache.org/jira/browse/FLUME-886
Diffs (updated)
-----
branches/flume-728/flume-ng-log4jappender/pom.xml PRE-CREATION
branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAppender.java PRE-CREATION
branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAvroHeaders.java PRE-CREATION
branches/flume-728/flume-ng-log4jappender/src/test/java/org/apache/flume/log4jappender/TestLog4jAppender.java PRE-CREATION
branches/flume-728/flume-ng-log4jappender/src/test/resources/log4j.properties PRE-CREATION
branches/flume-728/pom.xml 1244858
Diff: https://reviews.apache.org/r/3924/diff
Testing
-------
Added unit tests.
Thanks,
Hari
Re: Review Request: Log4jAppender using AvroSourceProtocol
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3924/#review5164
-----------------------------------------------------------
branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAppender.java
<https://reviews.apache.org/r/3924/#comment11312>
Excellent error messages! The only comment I have is, should we pass the exception along so that people can see the stacktrace:
throw new FlumeException(errormsg, e);
- Brock
On 2012-02-16 17:47:02, Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3924/
> -----------------------------------------------------------
>
> (Updated 2012-02-16 17:47:02)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> A class which takes in a LoggingEvent and sends it to a predefined AvroSource at a given host and port.
>
>
> This addresses bug FLUME-886.
> https://issues.apache.org/jira/browse/FLUME-886
>
>
> Diffs
> -----
>
> branches/flume-728/flume-ng-log4jappender/pom.xml PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAvroHeaders.java PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/test/java/org/apache/flume/log4jappender/TestLog4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/test/resources/log4j.properties PRE-CREATION
> branches/flume-728/pom.xml 1244858
>
> Diff: https://reviews.apache.org/r/3924/diff
>
>
> Testing
> -------
>
> Added unit tests.
>
>
> Thanks,
>
> Hari
>
>
Re: Review Request: Log4jAppender using AvroSourceProtocol
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3924/
-----------------------------------------------------------
(Updated 2012-02-16 17:47:02.183272)
Review request for Flume.
Changes
-------
UPdated error messages. Kill client object if close function is called, forcing new client object if append is called.
Summary
-------
A class which takes in a LoggingEvent and sends it to a predefined AvroSource at a given host and port.
This addresses bug FLUME-886.
https://issues.apache.org/jira/browse/FLUME-886
Diffs (updated)
-----
branches/flume-728/flume-ng-log4jappender/pom.xml PRE-CREATION
branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAppender.java PRE-CREATION
branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAvroHeaders.java PRE-CREATION
branches/flume-728/flume-ng-log4jappender/src/test/java/org/apache/flume/log4jappender/TestLog4jAppender.java PRE-CREATION
branches/flume-728/flume-ng-log4jappender/src/test/resources/log4j.properties PRE-CREATION
branches/flume-728/pom.xml 1244858
Diff: https://reviews.apache.org/r/3924/diff
Testing
-------
Added unit tests.
Thanks,
Hari
Re: Review Request: Log4jAppender using AvroSourceProtocol
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3924/#review5160
-----------------------------------------------------------
A few comments below, otherwise looks good!
branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAppender.java
<https://reviews.apache.org/r/3924/#comment11283>
Perhaps we could have a better error message since these two messages will go up to users?
branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAppender.java
<https://reviews.apache.org/r/3924/#comment11284>
Better error messages on all these errors?
branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAppender.java
<https://reviews.apache.org/r/3924/#comment11285>
We don't want to shutdown the Transceiver here? Maybe not, I am not familiar with appenders.
- Brock
On 2012-02-16 07:42:40, Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3924/
> -----------------------------------------------------------
>
> (Updated 2012-02-16 07:42:40)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> A class which takes in a LoggingEvent and sends it to a predefined AvroSource at a given host and port.
>
>
> This addresses bug FLUME-886.
> https://issues.apache.org/jira/browse/FLUME-886
>
>
> Diffs
> -----
>
> branches/flume-728/flume-ng-log4jappender/pom.xml PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/main/java/org/apache/flume/log4jappender/Log4jAvroHeaders.java PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/test/java/org/apache/flume/log4jappender/TestLog4jAppender.java PRE-CREATION
> branches/flume-728/flume-ng-log4jappender/src/test/resources/log4j.properties PRE-CREATION
> branches/flume-728/pom.xml 1244858
>
> Diff: https://reviews.apache.org/r/3924/diff
>
>
> Testing
> -------
>
> Added unit tests.
>
>
> Thanks,
>
> Hari
>
>