Re: Review Request 53231: FLUME-2857 Kafka Source/Channel/Sink does not restore default values when live update config

2016-10-28 Thread Balázs Donát Bessenyei 

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


Ship it!




Ship It!

- Balázs Donát Bessenyei


On Oct. 28, 2016, 10:24 a.m., Tristan Stevens wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53231/
> ---
> 
> (Updated Oct. 28, 2016, 10:24 a.m.)
> 
> 
> Review request for Flume.
> 
> 
> Repository: flume-git
> 
> 
> Description
> ---
> 
> Change to fix error where Kafka Channel, Sink and Source sub-configurations 
> aren't tolerant of the configure() method being called more than once (as 
> happens when a Live Config Update happens)
> 
> 
> Diffs
> -
> 
>   
> flume-ng-channels/flume-kafka-channel/src/main/java/org/apache/flume/channel/kafka/KafkaChannel.java
>  47c0634 
>   
> flume-ng-channels/flume-kafka-channel/src/test/java/org/apache/flume/channel/kafka/TestKafkaChannel.java
>  276fee1 
>   
> flume-ng-sinks/flume-ng-kafka-sink/src/main/java/org/apache/flume/sink/kafka/KafkaSink.java
>  dd40224 
>   
> flume-ng-sinks/flume-ng-kafka-sink/src/test/java/org/apache/flume/sink/kafka/TestKafkaSink.java
>  7eccf76 
>   
> flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java
>  195eca3 
>   
> flume-ng-sources/flume-kafka-source/src/test/java/org/apache/flume/source/kafka/TestKafkaSource.java
>  9554201 
> 
> Diff: https://reviews.apache.org/r/53231/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests which all would have failed and now pass.
> Also tested manually for Channel and Sink (Producer settings only), Consumer 
> settings are a little harder as you don't get the verbose logs, however unit 
> tests pass.
> 
> 
> Thanks,
> 
> Tristan Stevens
> 
>

Re: Review Request 53231: FLUME-2857 Kafka Source/Channel/Sink does not restore default values when live update config

2016-10-28 Thread Attila Simon 

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


Ship it!




LGTM

- Attila Simon


On Oct. 28, 2016, 10:24 a.m., Tristan Stevens wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53231/
> ---
> 
> (Updated Oct. 28, 2016, 10:24 a.m.)
> 
> 
> Review request for Flume.
> 
> 
> Repository: flume-git
> 
> 
> Description
> ---
> 
> Change to fix error where Kafka Channel, Sink and Source sub-configurations 
> aren't tolerant of the configure() method being called more than once (as 
> happens when a Live Config Update happens)
> 
> 
> Diffs
> -
> 
>   
> flume-ng-channels/flume-kafka-channel/src/main/java/org/apache/flume/channel/kafka/KafkaChannel.java
>  47c0634 
>   
> flume-ng-channels/flume-kafka-channel/src/test/java/org/apache/flume/channel/kafka/TestKafkaChannel.java
>  276fee1 
>   
> flume-ng-sinks/flume-ng-kafka-sink/src/main/java/org/apache/flume/sink/kafka/KafkaSink.java
>  dd40224 
>   
> flume-ng-sinks/flume-ng-kafka-sink/src/test/java/org/apache/flume/sink/kafka/TestKafkaSink.java
>  7eccf76 
>   
> flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java
>  195eca3 
>   
> flume-ng-sources/flume-kafka-source/src/test/java/org/apache/flume/source/kafka/TestKafkaSource.java
>  9554201 
> 
> Diff: https://reviews.apache.org/r/53231/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests which all would have failed and now pass.
> Also tested manually for Channel and Sink (Producer settings only), Consumer 
> settings are a little harder as you don't get the verbose logs, however unit 
> tests pass.
> 
> 
> Thanks,
> 
> Tristan Stevens
> 
>

Re: Review Request 53231: FLUME-2857 Kafka Source/Channel/Sink does not restore default values when live update config

2016-10-28 Thread Tristan Stevens 

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

(Updated Oct. 28, 2016, 10:24 a.m.)


Review request for Flume.


Changes
---

Issues fixed, thanks @Denes


Repository: flume-git


Description
---

Change to fix error where Kafka Channel, Sink and Source sub-configurations 
aren't tolerant of the configure() method being called more than once (as 
happens when a Live Config Update happens)


Diffs (updated)
-

  
flume-ng-channels/flume-kafka-channel/src/main/java/org/apache/flume/channel/kafka/KafkaChannel.java
 47c0634 
  
flume-ng-channels/flume-kafka-channel/src/test/java/org/apache/flume/channel/kafka/TestKafkaChannel.java
 276fee1 
  
flume-ng-sinks/flume-ng-kafka-sink/src/main/java/org/apache/flume/sink/kafka/KafkaSink.java
 dd40224 
  
flume-ng-sinks/flume-ng-kafka-sink/src/test/java/org/apache/flume/sink/kafka/TestKafkaSink.java
 7eccf76 
  
flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java
 195eca3 
  
flume-ng-sources/flume-kafka-source/src/test/java/org/apache/flume/source/kafka/TestKafkaSource.java
 9554201 

Diff: https://reviews.apache.org/r/53231/diff/


Testing
---

Added unit tests which all would have failed and now pass.
Also tested manually for Channel and Sink (Producer settings only), Consumer 
settings are a little harder as you don't get the verbose logs, however unit 
tests pass.


Thanks,

Tristan Stevens

Re: Review Request 53231: FLUME-2857 Kafka Source/Channel/Sink does not restore default values when live update config

2016-10-27 Thread Denes Arvay 

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


Fix it, then Ship it!




Two minor comments, otherwise LGTM, thanks for the patch!


flume-ng-sinks/flume-ng-kafka-sink/src/main/java/org/apache/flume/sink/kafka/KafkaSink.java
 (line 407)


I think this one could be `protected`. Or am I missing something, is there 
a special reason why you changed it to `public`?



flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java
 (line 476)


Same here as at `KafkaSink.getKafkaProps` (package-private this time)


- Denes Arvay


On Oct. 27, 2016, 6:16 p.m., Tristan Stevens wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53231/
> ---
> 
> (Updated Oct. 27, 2016, 6:16 p.m.)
> 
> 
> Review request for Flume.
> 
> 
> Repository: flume-git
> 
> 
> Description
> ---
> 
> Change to fix error where Kafka Channel, Sink and Source sub-configurations 
> aren't tolerant of the configure() method being called more than once (as 
> happens when a Live Config Update happens)
> 
> 
> Diffs
> -
> 
>   
> flume-ng-channels/flume-kafka-channel/src/main/java/org/apache/flume/channel/kafka/KafkaChannel.java
>  47c0634 
>   
> flume-ng-channels/flume-kafka-channel/src/test/java/org/apache/flume/channel/kafka/TestKafkaChannel.java
>  276fee1 
>   
> flume-ng-sinks/flume-ng-kafka-sink/src/main/java/org/apache/flume/sink/kafka/KafkaSink.java
>  dd40224 
>   
> flume-ng-sinks/flume-ng-kafka-sink/src/test/java/org/apache/flume/sink/kafka/TestKafkaSink.java
>  7eccf76 
>   
> flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java
>  195eca3 
>   
> flume-ng-sources/flume-kafka-source/src/test/java/org/apache/flume/source/kafka/TestKafkaSource.java
>  9554201 
> 
> Diff: https://reviews.apache.org/r/53231/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests which all would have failed and now pass.
> Also tested manually for Channel and Sink (Producer settings only), Consumer 
> settings are a little harder as you don't get the verbose logs, however unit 
> tests pass.
> 
> 
> Thanks,
> 
> Tristan Stevens
> 
>

Review Request 53231: FLUME-2857 Kafka Source/Channel/Sink does not restore default values when live update config

2016-10-27 Thread Tristan Stevens 

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

Review request for Flume.


Repository: flume-git


Description
---

Change to fix error where Kafka Channel, Sink and Source sub-configurations 
aren't tolerant of the configure() method being called more than once (as 
happens when a Live Config Update happens)


Diffs
-

  
flume-ng-channels/flume-kafka-channel/src/main/java/org/apache/flume/channel/kafka/KafkaChannel.java
 47c0634 
  
flume-ng-channels/flume-kafka-channel/src/test/java/org/apache/flume/channel/kafka/TestKafkaChannel.java
 276fee1 
  
flume-ng-sinks/flume-ng-kafka-sink/src/main/java/org/apache/flume/sink/kafka/KafkaSink.java
 dd40224 
  
flume-ng-sinks/flume-ng-kafka-sink/src/test/java/org/apache/flume/sink/kafka/TestKafkaSink.java
 7eccf76 
  
flume-ng-sources/flume-kafka-source/src/main/java/org/apache/flume/source/kafka/KafkaSource.java
 195eca3 
  
flume-ng-sources/flume-kafka-source/src/test/java/org/apache/flume/source/kafka/TestKafkaSource.java
 9554201 

Diff: https://reviews.apache.org/r/53231/diff/


Testing
---

Added unit tests which all would have failed and now pass.
Also tested manually for Channel and Sink (Producer settings only), Consumer 
settings are a little harder as you don't get the verbose logs, however unit 
tests pass.


Thanks,

Tristan Stevens