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 28035: SAMZA-310-1
Date Fri, 14 Nov 2014 19:05:11 GMT

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


Looks like this was coded against master. Are you planning to backport to 0.8.0 branch?

I think we should either keep this module scala-free, or should add scala as a first-class
citizen (i.e. implement the config code using real Scala). I'm not a big fan of bleeding Scala
APIs into Java (e.g. Option).

If we keep it scala free, we'll have to duplicate a couple of config names, but I think that's
an OK trade-off. SAMZA-40 is going to fix this stuff (Config with Java as a first class citizen)
anyway.


build.gradle
<https://reviews.apache.org/r/28035/#comment103120>

    If you keep scala, this will have to be a compile-time dependency, not testCompile. This
is because Option is imported in the config object.



samza-log4j/src/main/java/org/apache/samza/config/Log4jSystemConfig.java
<https://reviews.apache.org/r/28035/#comment103094>

    Javadocs.



samza-log4j/src/main/java/org/apache/samza/config/Log4jSystemConfig.java
<https://reviews.apache.org/r/28035/#comment103092>

    Add to configuration-table.html



samza-log4j/src/main/java/org/apache/samza/config/Log4jSystemConfig.java
<https://reviews.apache.org/r/28035/#comment103096>

    Should try and guess the right default. If there's only one system defined, then we shouldn't
require task.log4j.system to be set, we should just use the system.



samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java
<https://reviews.apache.org/r/28035/#comment103080>

    Javadocs.



samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java
<https://reviews.apache.org/r/28035/#comment103084>

    should probably private static final for all three of these.



samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java
<https://reviews.apache.org/r/28035/#comment103085>

    private * for all four of these.



samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java
<https://reviews.apache.org/r/28035/#comment103106>

    This is pretty confusing. Since we're using a string, can we just have it default to "application-master",
and use "samza-container-%d" for containers?



samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java
<https://reviews.apache.org/r/28035/#comment103088>

    Some logging in this method seems useful in cases where the appender can't properly instantiate
itself.



samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java
<https://reviews.apache.org/r/28035/#comment103089>

    need both job name and job id here to be fully unique. __samza-<job name>-<job
id>-logs. Also, should string replace "-" to "_" for job name and job id. See:
    
    private def getTopic(jobName: String, jobId: String) =
    
    In KafkaCheckpointManagerFactory for an example.
    
    Might want to make this configurable, as well. Should still provide a default, but I can
imagine use cases where folks might want to customize their stream name.



samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java
<https://reviews.apache.org/r/28035/#comment103112>

    Can we do "samza-container-%d" here, just to make things a big more explicit.



samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java
<https://reviews.apache.org/r/28035/#comment103116>

    Should call flushSystemProducer before stop. The SystemProducer implementations should
do this as well, but just to be safe.



samza-log4j/src/test/java/org/apache/samza/logging/log4j/TestSystemProducerAppender.java
<https://reviews.apache.org/r/28035/#comment103077>

    License.


- Chris Riccomini


On Nov. 14, 2014, 11:40 a.m., Yan Fang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28035/
> -----------------------------------------------------------
> 
> (Updated Nov. 14, 2014, 11:40 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Bugs: SAMZA-310
>     https://issues.apache.org/jira/browse/SAMZA-310
> 
> 
> Repository: samza
> 
> 
> Description
> -------
> 
> Added a log4j appender using SystemProducer
> Added a log4jConfig class to help get log4j specific config
> Unit test code
> 
> 
> Diffs
> -----
> 
>   build.gradle 828bce9 
>   samza-log4j/src/main/java/org/apache/samza/config/Log4jSystemConfig.java PRE-CREATION

>   samza-log4j/src/main/java/org/apache/samza/logging/log4j/SystemProducerAppender.java
PRE-CREATION 
>   samza-log4j/src/test/java/org/apache/samza/logging/log4j/TestSystemProducerAppender.java
PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/28035/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Yan Fang
> 
>


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