samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris Riccomini" <criccom...@apache.org>
Subject Re: Review Request 32006: SAMZA-597
Date Fri, 13 Mar 2015 20:39:10 GMT


> On March 13, 2015, 5:31 p.m., Yi Pan (Data Infrastructure) wrote:
> > samza-log4j/src/main/java/org/apache/samza/config/Log4jSystemConfig.java, line 54
> > <https://reviews.apache.org/r/32006/diff/1/?file=892561#file892561line54>
> >
> >     It seems that the default is set to false. Just curious: don't we always want
to see the file location info when using StreamAppender? Or is it included somewhere else?

I am following the lead of logstash's appender. I also think it makes sense to disable by
default. It increases message payload by 10%-20%. We could definitely default to true, but
in practice, I haven't seen this used much.


> On March 13, 2015, 5:31 p.m., Yi Pan (Data Infrastructure) wrote:
> > samza-log4j/src/main/java/org/apache/samza/config/Log4jSystemConfig.java, line 102
> > <https://reviews.apache.org/r/32006/diff/1/?file=892561#file892561line102>
> >
> >     It seems that the default return value of this method has changed and no longer
default the return value to LoggingEventStringSerdeFactory. It would be better to remove this
Java doc description here.

Done.


> On March 13, 2015, 5:31 p.m., Yi Pan (Data Infrastructure) wrote:
> > samza-log4j/src/main/java/org/apache/samza/logging/log4j/serializers/LoggingEventJsonSerde.java,
line 53
> > <https://reviews.apache.org/r/32006/diff/1/?file=892563#file892563line53>
> >
> >     nit: Serde<Object>

Done.


- Chris


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32006/#review76381
-----------------------------------------------------------


On March 13, 2015, 12:57 a.m., Chris Riccomini wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32006/
> -----------------------------------------------------------
> 
> (Updated March 13, 2015, 12:57 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> -------
> 
> add docs
> 
> 
> fail if containerName is not set.
> 
> 
> update tests
> 
> 
> move location enabled config to log4j config file
> 
> 
> fix MDC link in docs
> 
> 
> add a logging event json serde for log4j, and set it as default.
> 
> 
> default to log4j string serde for now
> 
> 
> Diffs
> -----
> 
>   build.gradle 08583e07f1c0bda88433bacb59bc2fd9ef6ce310 
>   docs/learn/documentation/versioned/jobs/configuration-table.html ec1287418042b95df73ff7c36a684d3123c46372

>   docs/learn/documentation/versioned/jobs/logging.md af2fd0ea6929230cdc6bc3c51d9ae62adacb55fa

>   samza-log4j/src/main/java/org/apache/samza/config/Log4jSystemConfig.java 107ddf0c3d4e0f584a2f68a23debbada5f68dcb8

>   samza-log4j/src/main/java/org/apache/samza/logging/log4j/StreamAppender.java 4ef3551f470e77e27bd156e81ce96486f25c21bf

>   samza-log4j/src/main/java/org/apache/samza/logging/log4j/serializers/LoggingEventJsonSerde.java
PRE-CREATION 
>   samza-log4j/src/main/java/org/apache/samza/logging/log4j/serializers/LoggingEventJsonSerdeFactory.java
PRE-CREATION 
>   samza-log4j/src/test/java/org/apache/samza/config/TestLog4jSystemConfig.java 16ccb459892f62245648235eb65f53b26e8ecb87

>   samza-log4j/src/test/java/org/apache/samza/logging/log4j/TestStreamAppender.java 3e4ddc9c72868e22f993f60015224cd3a153266c

> 
> Diff: https://reviews.apache.org/r/32006/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Chris Riccomini
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message