Re: Review Request 52550: FLUME-2989: Review Request

[Umesh Chaudhary]

> On Oct. 18, 2016, 9:44 a.m., Balázs Donát Bessenyei wrote:
> > flume-ng-channels/flume-kafka-channel/src/test/java/org/apache/flume/channel/kafka/TestKafkaChannel.java,
> >  line 769
> > 
> >
> > Do you think this Thread.sleep could be avoided?
> > 
> > If not, do you think you can change it in a way that it will be 
> > not-so-prone to flakiness?
> > (Maybe something like at https://reviews.apache.org/r/49025/diff/5#3 )

Need Thread.sleep to spawn the HTTPMetricsServer before making HTTP GET request 
for metrics. 

Reduced the sleep time for 10ms so that it would not show flakiness.


On Oct. 18, 2016, 9:44 a.m., Umesh Chaudhary wrote:
> > Thank you for the patch!
> > 
> > Can you please rebase it on trunk so that it applies cleanly?
> > 
> > Also, please reformat the code to pass checkstyle checks.

Rebased in on trunk and fixed the checkstyle error.


- Umesh


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


On Oct. 19, 2016, 10:17 a.m., Umesh Chaudhary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52550/
> ---
> 
> (Updated Oct. 19, 2016, 10:17 a.m.)
> 
> 
> Review request for Flume, Balázs Donát Bessenyei, Jeff Holoman, and Mike 
> Percy.
> 
> 
> Repository: flume-git
> 
> 
> Description
> ---
> 
> Added appropriate function calls to capture eventTakeAttemptCount and 
> eventPutAttemptCount in KafkaChannel
> 
> 
> 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 
> 
> Diff: https://reviews.apache.org/r/52550/diff/
> 
> 
> Testing
> ---
> 
> # mvn  -DfailIfNoTests=false 
> -Dtest=org.apache.flume.channel.kafka.TestKafkaChannel test
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 04:17 min
> [INFO] Finished at: 2016-10-19T15:31:38+05:30
> [INFO] Final Memory: 98M/1210M
> [INFO] 
> 
> 
> # mvn -pl flume-ng-channels -Drat.skip=true test
> 
> [INFO] 
> 
> [INFO] BUILD SUCCESS
> [INFO] 
> 
> [INFO] Total time: 1.108 s
> [INFO] Finished at: 2016-10-19T15:44:52+05:30
> [INFO] Final Memory: 23M/309M
> [INFO] 
> 
> 
> 
> Thanks,
> 
> Umesh Chaudhary
> 
>

Re: Review Request 52550: FLUME-2989: Review Request

[Umesh Chaudhary]

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

(Updated Oct. 19, 2016, 10:17 a.m.)


Review request for Flume, Balázs Donát Bessenyei, Jeff Holoman, and Mike Percy.


Repository: flume-git


Description
---

Added appropriate function calls to capture eventTakeAttemptCount and 
eventPutAttemptCount in KafkaChannel


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 

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


Testing (updated)
---

# mvn  -DfailIfNoTests=false 
-Dtest=org.apache.flume.channel.kafka.TestKafkaChannel test

[INFO] 
[INFO] BUILD SUCCESS
[INFO] 
[INFO] Total time: 04:17 min
[INFO] Finished at: 2016-10-19T15:31:38+05:30
[INFO] Final Memory: 98M/1210M
[INFO] 

# mvn -pl flume-ng-channels -Drat.skip=true test

[INFO] 
[INFO] BUILD SUCCESS
[INFO] 
[INFO] Total time: 1.108 s
[INFO] Finished at: 2016-10-19T15:44:52+05:30
[INFO] Final Memory: 23M/309M
[INFO] 


Thanks,

Umesh Chaudhary

Re: Review Request 52550: FLUME-2989: Review Request

[Balázs Donát Bessenyei]

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




flume-ng-channels/flume-kafka-channel/src/test/java/org/apache/flume/channel/kafka/TestKafkaChannel.java
 (line 769)


Do you think this Thread.sleep could be avoided?

If not, do you think you can change it in a way that it will be 
not-so-prone to flakiness?
(Maybe something like at https://reviews.apache.org/r/49025/diff/5#3 )


Thank you for the patch!

Can you please rebase it on trunk so that it applies cleanly?

Also, please reformat the code to pass checkstyle checks.

- Balázs Donát Bessenyei


On Oct. 10, 2016, 12:44 p.m., Umesh Chaudhary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52550/
> ---
> 
> (Updated Oct. 10, 2016, 12:44 p.m.)
> 
> 
> Review request for Flume.
> 
> 
> Repository: flume-git
> 
> 
> Description
> ---
> 
> Added appropriate function calls to capture eventTakeAttemptCount and 
> eventPutAttemptCount in KafkaChannel
> 
> 
> Diffs
> -
> 
>   
> flume-ng-channels/flume-kafka-channel/src/main/java/org/apache/flume/channel/kafka/KafkaChannel.java
>  66b553a 
>   
> flume-ng-channels/flume-kafka-channel/src/test/java/org/apache/flume/channel/kafka/TestKafkaChannel.java
>  57c0b28 
> 
> Diff: https://reviews.apache.org/r/52550/diff/
> 
> 
> Testing
> ---
> 
> Testing Done.
> 
> 
> Thanks,
> 
> Umesh Chaudhary
> 
>

Re: Review Request 52550: FLUME-2989: Review Request

[Umesh Chaudhary]

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

(Updated Oct. 10, 2016, 12:44 p.m.)


Review request for Flume.


Changes
---

Added Unit Test and incorporated Jeff's comments.


Repository: flume-git


Description
---

Added appropriate function calls to capture eventTakeAttemptCount and 
eventPutAttemptCount in KafkaChannel


Diffs (updated)
-

  
flume-ng-channels/flume-kafka-channel/src/main/java/org/apache/flume/channel/kafka/KafkaChannel.java
 66b553a 
  
flume-ng-channels/flume-kafka-channel/src/test/java/org/apache/flume/channel/kafka/TestKafkaChannel.java
 57c0b28 

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


Testing (updated)
---

Testing Done.


Thanks,

Umesh Chaudhary

Re: Review Request 52550: FLUME-2989: Review Request

[Umesh Chaudhary]

> On Oct. 5, 2016, 9:12 p.m., Jeff Holoman wrote:
> > Can you elaborate on "Testing passed". I think it would make sense to 
> > unit-test this in order to ensure the counters are incrementing properly.

As this is a trivial change and we have already working counters like 
TIMER_KAFKA_EVENT_GET, COUNTER_EVENT_PUT_SUCCESS and COUNTER_EVENT_TAKE_SUCCESS 
in place so thought this would work in the same way.


> On Oct. 5, 2016, 9:12 p.m., Jeff Holoman wrote:
> > flume-ng-channels/flume-kafka-channel/src/main/java/org/apache/flume/channel/kafka/KafkaChannel.java,
> >  line 533
> > 
> >
> > I don't think this is in the correct place. The previous line adds the 
> > entire batch to the Put Success count. I think we want to add this in the 
> > doPut method instead?

Yes, you're correct. I was thinking to also increment it even when trying to 
commit the get/put. But that is actually not a get/put attempt so I will revert 
it.


> On Oct. 5, 2016, 9:12 p.m., Jeff Holoman wrote:
> > flume-ng-channels/flume-kafka-channel/src/main/java/org/apache/flume/channel/kafka/KafkaChannel.java,
> >  line 550
> > 
> >
> > Same thing here. I don't see why we need this here if we are already 
> > incrementing in the doTake()?

Fixed as above comment


- Umesh


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


On Oct. 5, 2016, 10:51 a.m., Umesh Chaudhary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52550/
> ---
> 
> (Updated Oct. 5, 2016, 10:51 a.m.)
> 
> 
> Review request for Flume.
> 
> 
> Repository: flume-git
> 
> 
> Description
> ---
> 
> Added appropriate function calls to capture eventTakeAttemptCount and 
> eventPutAttemptCount in KafkaChannel
> 
> 
> Diffs
> -
> 
>   
> flume-ng-channels/flume-kafka-channel/src/main/java/org/apache/flume/channel/kafka/KafkaChannel.java
>  66b553a 
> 
> Diff: https://reviews.apache.org/r/52550/diff/
> 
> 
> Testing
> ---
> 
> Testing Passed.
> 
> 
> Thanks,
> 
> Umesh Chaudhary
> 
>

Re: Review Request 52550: FLUME-2989: Review Request

[Jeff Holoman]

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



Can you elaborate on "Testing passed". I think it would make sense to unit-test 
this in order to ensure the counters are incrementing properly.


flume-ng-channels/flume-kafka-channel/src/main/java/org/apache/flume/channel/kafka/KafkaChannel.java
 (line 533)


I don't think this is in the correct place. The previous line adds the 
entire batch to the Put Success count. I think we want to add this in the doPut 
method instead?



flume-ng-channels/flume-kafka-channel/src/main/java/org/apache/flume/channel/kafka/KafkaChannel.java
 (line 550)


Same thing here. I don't see why we need this here if we are already 
incrementing in the doTake()?


- Jeff Holoman


On Oct. 5, 2016, 10:51 a.m., Umesh Chaudhary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52550/
> ---
> 
> (Updated Oct. 5, 2016, 10:51 a.m.)
> 
> 
> Review request for Flume.
> 
> 
> Repository: flume-git
> 
> 
> Description
> ---
> 
> Added appropriate function calls to capture eventTakeAttemptCount and 
> eventPutAttemptCount in KafkaChannel
> 
> 
> Diffs
> -
> 
>   
> flume-ng-channels/flume-kafka-channel/src/main/java/org/apache/flume/channel/kafka/KafkaChannel.java
>  66b553a 
> 
> Diff: https://reviews.apache.org/r/52550/diff/
> 
> 
> Testing
> ---
> 
> Testing Passed.
> 
> 
> Thanks,
> 
> Umesh Chaudhary
> 
>