Re: Review Request 40313: SAMZA-785

[VENKATA KRISHNA NANNAPANENI]

> On Nov. 16, 2015, 7:34 p.m., Boris Shkolnik wrote:
> >
> 
> VENKATA KRISHNA NANNAPANENI wrote:
> Is there anything I need to do like commit or create pull request? I 
> wasn't aware of the process from here.
> 
> Navina Ramesh wrote:
> Sorry for the delay, Venkata Krishna Nannapaneni. We are currently 
> closing up the JIRAs related to 0.10.0 release. Will review and commit your 
> code once that is done. Thanks!

Hi, Just wanted follow up on this. JIRA is been assigned to me. Is there 
anything I can do to get his closed? 
(https://issues.apache.org/jira/browse/SAMZA-785).


- VENKATA KRISHNA


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


On Nov. 13, 2015, 10:51 p.m., VENKATA KRISHNA NANNAPANENI wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40313/
> ---
> 
> (Updated Nov. 13, 2015, 10:51 p.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> check for system serde name for stream appender.
> 
> 
> Diffs
> -
> 
>   samza-log4j/src/main/java/org/apache/samza/config/Log4jSystemConfig.java 
> 59015a90acaf822469b0b24164e2d4c4a8000bce 
>   
> samza-log4j/src/main/java/org/apache/samza/logging/log4j/StreamAppender.java 
> 0c6329ede9b3df4dc05125729b5b44ba2c98803a 
>   
> samza-log4j/src/test/java/org/apache/samza/logging/log4j/TestStreamAppender.java
>  e2e17a06159a35234270f342e8ae1c81031d971b 
> 
> Diff: https://reviews.apache.org/r/40313/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> VENKATA KRISHNA NANNAPANENI
> 
>

Re: Review Request 40313: SAMZA-785

[VENKATA KRISHNA NANNAPANENI]

> On Nov. 18, 2015, 9:31 p.m., Navina Ramesh wrote:
> > samza-log4j/src/main/java/org/apache/samza/logging/log4j/StreamAppender.java,
> >  line 242
> > 
> >
> > What happens if serdeName is not defined at stream or system level? You 
> > should throw an exception where a serde is not defined.
> > 
> > Can you please change the multiple if statments to a more logic if-else 
> > statement?

I don't think it is can be an if else here (Cause of the scenarios where it has 
to set both). Let me know if you still think otherwise. Thanks.


- VENKATA KRISHNA


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


On Nov. 13, 2015, 10:51 p.m., VENKATA KRISHNA NANNAPANENI wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40313/
> ---
> 
> (Updated Nov. 13, 2015, 10:51 p.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> check for system serde name for stream appender.
> 
> 
> Diffs
> -
> 
>   samza-log4j/src/main/java/org/apache/samza/config/Log4jSystemConfig.java 
> 59015a90acaf822469b0b24164e2d4c4a8000bce 
>   
> samza-log4j/src/main/java/org/apache/samza/logging/log4j/StreamAppender.java 
> 0c6329ede9b3df4dc05125729b5b44ba2c98803a 
>   
> samza-log4j/src/test/java/org/apache/samza/logging/log4j/TestStreamAppender.java
>  e2e17a06159a35234270f342e8ae1c81031d971b 
> 
> Diff: https://reviews.apache.org/r/40313/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> VENKATA KRISHNA NANNAPANENI
> 
>

Re: Review Request 40313: SAMZA-785

[VENKATA KRISHNA NANNAPANENI]

> On Nov. 18, 2015, 9:31 p.m., Navina Ramesh wrote:
> >

Sorry, I hope this is not too late. It does throw an exception if it can't find 
the serde.


- VENKATA KRISHNA


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


On Nov. 13, 2015, 10:51 p.m., VENKATA KRISHNA NANNAPANENI wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40313/
> ---
> 
> (Updated Nov. 13, 2015, 10:51 p.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> check for system serde name for stream appender.
> 
> 
> Diffs
> -
> 
>   samza-log4j/src/main/java/org/apache/samza/config/Log4jSystemConfig.java 
> 59015a90acaf822469b0b24164e2d4c4a8000bce 
>   
> samza-log4j/src/main/java/org/apache/samza/logging/log4j/StreamAppender.java 
> 0c6329ede9b3df4dc05125729b5b44ba2c98803a 
>   
> samza-log4j/src/test/java/org/apache/samza/logging/log4j/TestStreamAppender.java
>  e2e17a06159a35234270f342e8ae1c81031d971b 
> 
> Diff: https://reviews.apache.org/r/40313/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> VENKATA KRISHNA NANNAPANENI
> 
>

Re: Review Request 40313: SAMZA-785

[Navina Ramesh]

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



samza-log4j/src/main/java/org/apache/samza/logging/log4j/StreamAppender.java 
(line 242)


What happens if serdeName is not defined at stream or system level? You 
should throw an exception where a serde is not defined.

Can you please change the multiple if statments to a more logic if-else 
statement?


- Navina Ramesh


On Nov. 13, 2015, 10:51 p.m., VENKATA KRISHNA NANNAPANENI wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40313/
> ---
> 
> (Updated Nov. 13, 2015, 10:51 p.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> check for system serde name for stream appender.
> 
> 
> Diffs
> -
> 
>   samza-log4j/src/main/java/org/apache/samza/config/Log4jSystemConfig.java 
> 59015a90acaf822469b0b24164e2d4c4a8000bce 
>   
> samza-log4j/src/main/java/org/apache/samza/logging/log4j/StreamAppender.java 
> 0c6329ede9b3df4dc05125729b5b44ba2c98803a 
>   
> samza-log4j/src/test/java/org/apache/samza/logging/log4j/TestStreamAppender.java
>  e2e17a06159a35234270f342e8ae1c81031d971b 
> 
> Diff: https://reviews.apache.org/r/40313/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> VENKATA KRISHNA NANNAPANENI
> 
>

Re: Review Request 40313: SAMZA-785

[Navina Ramesh]

> On Nov. 16, 2015, 7:34 p.m., Boris Shkolnik wrote:
> >
> 
> VENKATA KRISHNA NANNAPANENI wrote:
> Is there anything I need to do like commit or create pull request? I 
> wasn't aware of the process from here.

Sorry for the delay, Venkata Krishna Nannapaneni. We are currently closing up 
the JIRAs related to 0.10.0 release. Will review and commit your code once that 
is done. Thanks!


- Navina


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


On Nov. 13, 2015, 10:51 p.m., VENKATA KRISHNA NANNAPANENI wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40313/
> ---
> 
> (Updated Nov. 13, 2015, 10:51 p.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> check for system serde name for stream appender.
> 
> 
> Diffs
> -
> 
>   samza-log4j/src/main/java/org/apache/samza/config/Log4jSystemConfig.java 
> 59015a90acaf822469b0b24164e2d4c4a8000bce 
>   
> samza-log4j/src/main/java/org/apache/samza/logging/log4j/StreamAppender.java 
> 0c6329ede9b3df4dc05125729b5b44ba2c98803a 
>   
> samza-log4j/src/test/java/org/apache/samza/logging/log4j/TestStreamAppender.java
>  e2e17a06159a35234270f342e8ae1c81031d971b 
> 
> Diff: https://reviews.apache.org/r/40313/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> VENKATA KRISHNA NANNAPANENI
> 
>

Re: Review Request 40313: SAMZA-785

[VENKATA KRISHNA NANNAPANENI]

> On Nov. 16, 2015, 7:34 p.m., Boris Shkolnik wrote:
> >

Is there anything I need to do like commit or create pull request? I wasn't 
aware of the process from here.


- VENKATA KRISHNA


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


On Nov. 13, 2015, 10:51 p.m., VENKATA KRISHNA NANNAPANENI wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40313/
> ---
> 
> (Updated Nov. 13, 2015, 10:51 p.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> check for system serde name for stream appender.
> 
> 
> Diffs
> -
> 
>   samza-log4j/src/main/java/org/apache/samza/config/Log4jSystemConfig.java 
> 59015a90acaf822469b0b24164e2d4c4a8000bce 
>   
> samza-log4j/src/main/java/org/apache/samza/logging/log4j/StreamAppender.java 
> 0c6329ede9b3df4dc05125729b5b44ba2c98803a 
>   
> samza-log4j/src/test/java/org/apache/samza/logging/log4j/TestStreamAppender.java
>  e2e17a06159a35234270f342e8ae1c81031d971b 
> 
> Diff: https://reviews.apache.org/r/40313/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> VENKATA KRISHNA NANNAPANENI
> 
>

Re: Review Request 40313: SAMZA-785

[Boris Shkolnik]

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

Ship it!


- Boris Shkolnik


On Nov. 13, 2015, 10:51 p.m., VENKATA KRISHNA NANNAPANENI wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40313/
> ---
> 
> (Updated Nov. 13, 2015, 10:51 p.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> check for system serde name for stream appender.
> 
> 
> Diffs
> -
> 
>   samza-log4j/src/main/java/org/apache/samza/config/Log4jSystemConfig.java 
> 59015a90acaf822469b0b24164e2d4c4a8000bce 
>   
> samza-log4j/src/main/java/org/apache/samza/logging/log4j/StreamAppender.java 
> 0c6329ede9b3df4dc05125729b5b44ba2c98803a 
>   
> samza-log4j/src/test/java/org/apache/samza/logging/log4j/TestStreamAppender.java
>  e2e17a06159a35234270f342e8ae1c81031d971b 
> 
> Diff: https://reviews.apache.org/r/40313/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> VENKATA KRISHNA NANNAPANENI
> 
>