You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Umesh Chaudhary <um...@gmail.com> on 2016/10/05 10:51:02 UTC
Review Request 52550: FLUME-2989: Review Request
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52550/
-----------------------------------------------------------
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
Posted by Umesh Chaudhary <um...@gmail.com>.
> 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
> > <https://reviews.apache.org/r/52550/diff/1/?file=1522919#file1522919line533>
> >
> > 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
> > <https://reviews.apache.org/r/52550/diff/1/?file=1522919#file1522919line550>
> >
> > 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
Posted by Jeff Holoman <je...@gmail.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/52550/#comment220002>
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)
<https://reviews.apache.org/r/52550/#comment220003>
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
>
>
Re: Review Request 52550: FLUME-2989: Review Request
Posted by Umesh Chaudhary <um...@gmail.com>.
-----------------------------------------------------------
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
Posted by Umesh Chaudhary <um...@gmail.com>.
> 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
> > <https://reviews.apache.org/r/52550/diff/2/?file=1529617#file1529617line769>
> >
> > 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
Posted by Balázs Donát Bessenyei <be...@cloudera.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/52550/#comment222338>
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
Posted by Umesh Chaudhary <um...@gmail.com>.
-----------------------------------------------------------
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