You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Steven Yates <sy...@stevendyates.com> on 2014/02/09 01:19:24 UTC
Review Request 17875: SAMZA-30
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17875/
-----------------------------------------------------------
Review request for samza.
Repository: samza
Description
-------
SAMZA-30 - Back off in BrokerProxy when fetch fails
Diffs
-----
samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala 95a1fb5
samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestBrokerProxy.scala d1df781
Diff: https://reviews.apache.org/r/17875/diff/
Testing
-------
Some
Thanks,
Steven Yates
Re: Review Request 17875: SAMZA-30 : Back off in BrokerProxy when fetch fails
Posted by Steven Yates <sy...@stevendyates.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17875/
-----------------------------------------------------------
(Updated Feb. 25, 2014, 10:51 a.m.)
Review request for samza.
Bugs: SAMZA-30
https://issues.apache.org/jira/browse/SAMZA-30
Repository: samza
Description
-------
SAMZA-30 - Back off in BrokerProxy when fetch fails
Diffs
-----
samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala 34727e9
samza-kafka/src/main/scala/org/apache/samza/util/ExponentialThreadSleepStrategy.scala PRE-CREATION
samza-kafka/src/test/scala/org/apache/samza/util/TestExponentialThreadSleepStrategy.scala PRE-CREATION
Diff: https://reviews.apache.org/r/17875/diff/
Testing
-------
Some
File Attachments
----------------
SAMZA-30-4.diff
https://reviews.apache.org/media/uploaded/files/2014/02/15/d4695ce4-f96c-47ab-aadb-5f3460f398d4__SAMZA-30-4.diff
SAMZA-30-3.diff
https://reviews.apache.org/media/uploaded/files/2014/02/15/f158e7d4-577c-4bae-9ffc-9d982b4411e0__SAMZA-30-3.diff
Thanks,
Steven Yates
Re: Review Request 17875: SAMZA-30 : Back off in BrokerProxy when fetch fails
Posted by Steven Yates <sy...@stevendyates.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17875/
-----------------------------------------------------------
(Updated Feb. 25, 2014, 10:51 a.m.)
Review request for samza.
Summary (updated)
-----------------
SAMZA-30 : Back off in BrokerProxy when fetch fails
Repository: samza
Description
-------
SAMZA-30 - Back off in BrokerProxy when fetch fails
Diffs
-----
samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala 34727e9
samza-kafka/src/main/scala/org/apache/samza/util/ExponentialThreadSleepStrategy.scala PRE-CREATION
samza-kafka/src/test/scala/org/apache/samza/util/TestExponentialThreadSleepStrategy.scala PRE-CREATION
Diff: https://reviews.apache.org/r/17875/diff/
Testing
-------
Some
File Attachments
----------------
SAMZA-30-4.diff
https://reviews.apache.org/media/uploaded/files/2014/02/15/d4695ce4-f96c-47ab-aadb-5f3460f398d4__SAMZA-30-4.diff
SAMZA-30-3.diff
https://reviews.apache.org/media/uploaded/files/2014/02/15/f158e7d4-577c-4bae-9ffc-9d982b4411e0__SAMZA-30-3.diff
Thanks,
Steven Yates
Re: Review Request 17875: SAMZA-30
Posted by Steven Yates <sy...@stevendyates.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17875/
-----------------------------------------------------------
(Updated Feb. 22, 2014, 11:12 a.m.)
Review request for samza.
Changes
-------
Class name and method name reviewed and modified as discussed with Jakob.
Repository: samza
Description
-------
SAMZA-30 - Back off in BrokerProxy when fetch fails
Diffs (updated)
-----
samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala 34727e9
samza-kafka/src/main/scala/org/apache/samza/util/ExponentialThreadSleepStrategy.scala PRE-CREATION
samza-kafka/src/test/scala/org/apache/samza/util/TestExponentialThreadSleepStrategy.scala PRE-CREATION
Diff: https://reviews.apache.org/r/17875/diff/
Testing
-------
Some
File Attachments
----------------
SAMZA-30-4.diff
https://reviews.apache.org/media/uploaded/files/2014/02/15/d4695ce4-f96c-47ab-aadb-5f3460f398d4__SAMZA-30-4.diff
SAMZA-30-3.diff
https://reviews.apache.org/media/uploaded/files/2014/02/15/f158e7d4-577c-4bae-9ffc-9d982b4411e0__SAMZA-30-3.diff
Thanks,
Steven Yates
Re: Review Request 17875: SAMZA-30
Posted by Jakob Homan <jg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17875/#review35129
-----------------------------------------------------------
Looks good except for my being unclear on the class name versus method name, mentioned below. Sorry for the delay.
- Jakob Homan
On Feb. 15, 2014, 4:06 a.m., Steven Yates wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17875/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2014, 4:06 a.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-30 - Back off in BrokerProxy when fetch fails
>
>
> Diffs
> -----
>
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala 34727e9
> samza-kafka/src/main/scala/org/apache/samza/util/ThreadUtil.scala PRE-CREATION
> samza-kafka/src/test/scala/org/apache/samza/util/TestThreadUtil.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17875/diff/
>
>
> Testing
> -------
>
> Some
>
>
> File Attachments
> ----------------
>
> SAMZA-30-4.diff
> https://reviews.apache.org/media/uploaded/files/2014/02/15/d4695ce4-f96c-47ab-aadb-5f3460f398d4__SAMZA-30-4.diff
> SAMZA-30-3.diff
> https://reviews.apache.org/media/uploaded/files/2014/02/15/f158e7d4-577c-4bae-9ffc-9d982b4411e0__SAMZA-30-3.diff
>
>
> Thanks,
>
> Steven Yates
>
>
Re: Review Request 17875: SAMZA-30
Posted by Steven Yates <sy...@stevendyates.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17875/
-----------------------------------------------------------
(Updated Feb. 15, 2014, 12:06 p.m.)
Review request for samza.
Changes
-------
Updates as discussed with Jakob Homan. There are two diff files to be reviewed, SAMZA-30-3 and SAMZA-30-4 .diff
Repository: samza
Description
-------
SAMZA-30 - Back off in BrokerProxy when fetch fails
Diffs (updated)
-----
samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala 34727e9
samza-kafka/src/main/scala/org/apache/samza/util/ThreadUtil.scala PRE-CREATION
samza-kafka/src/test/scala/org/apache/samza/util/TestThreadUtil.scala PRE-CREATION
Diff: https://reviews.apache.org/r/17875/diff/
Testing
-------
Some
File Attachments (updated)
----------------
SAMZA-30-4.diff
https://reviews.apache.org/media/uploaded/files/2014/02/15/d4695ce4-f96c-47ab-aadb-5f3460f398d4__SAMZA-30-4.diff
SAMZA-30-3.diff
https://reviews.apache.org/media/uploaded/files/2014/02/15/f158e7d4-577c-4bae-9ffc-9d982b4411e0__SAMZA-30-3.diff
Thanks,
Steven Yates
Re: Review Request 17875: SAMZA-30
Posted by Steven Yates <sy...@stevendyates.com>.
> On Feb. 13, 2014, 8:31 p.m., Jakob Homan wrote:
> > samza-kafka/src/main/scala/org/apache/samza/util/ExponentialThreadSleepStrategy.scala, line 19
> > <https://reviews.apache.org/r/17875/diff/2/?file=483236#file483236line19>
> >
> > in the class, import ExpotentialThreadSleepStrategy._ and we can avoid all the extra qualification, to aid in readability.
Fixed in patch SAMZA-30-4.diff
> On Feb. 13, 2014, 8:31 p.m., Jakob Homan wrote:
> > samza-kafka/src/main/scala/org/apache/samza/util/ExponentialThreadSleepStrategy.scala, line 29
> > <https://reviews.apache.org/r/17875/diff/2/?file=483236#file483236line29>
> >
> > Since the class has been broken out and can be used for more than just redelivery delays, can we make this a more generic name to ease re-use?
Renamed to ThreadUtil
> On Feb. 13, 2014, 8:31 p.m., Jakob Homan wrote:
> > samza-kafka/src/test/scala/org/apache/samza/util/TestExponentialThreadSleepStrategy.scala, line 23
> > <https://reviews.apache.org/r/17875/diff/2/?file=483237#file483237line23>
> >
> > Can we extend the test to make sure we don't go past the maximum allowed? ie, we never go above 10000 ms?
We have captured this in testGetNextRedeliveryDelayCorrectlyReturnsMaximumDelayWhenDelayCapReached(). Let me know your thoughts
> On Feb. 13, 2014, 8:31 p.m., Jakob Homan wrote:
> > samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala, line 252
> > <https://reviews.apache.org/r/17875/diff/2/?file=483235#file483235line252>
> >
> > Part of the thread renaming loss
Apologies updated local master and merged fixed this
- Steven
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17875/#review34384
-----------------------------------------------------------
On Feb. 12, 2014, 3:33 a.m., Steven Yates wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17875/
> -----------------------------------------------------------
>
> (Updated Feb. 12, 2014, 3:33 a.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-30 - Back off in BrokerProxy when fetch fails
>
>
> Diffs
> -----
>
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala 95a1fb5
> samza-kafka/src/main/scala/org/apache/samza/util/ExponentialThreadSleepStrategy.scala PRE-CREATION
> samza-kafka/src/test/scala/org/apache/samza/util/TestExponentialThreadSleepStrategy.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17875/diff/
>
>
> Testing
> -------
>
> Some
>
>
> Thanks,
>
> Steven Yates
>
>
Re: Review Request 17875: SAMZA-30
Posted by Jakob Homan <jg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17875/#review34384
-----------------------------------------------------------
samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala
<https://reviews.apache.org/r/17875/#comment64465>
Looks like we're still losing SAMZA096 with this patch. Make sure you've merged in the latest changes from trunk.
samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala
<https://reviews.apache.org/r/17875/#comment64504>
Part of the thread renaming loss
samza-kafka/src/main/scala/org/apache/samza/util/ExponentialThreadSleepStrategy.scala
<https://reviews.apache.org/r/17875/#comment64505>
Need ASF file header
samza-kafka/src/main/scala/org/apache/samza/util/ExponentialThreadSleepStrategy.scala
<https://reviews.apache.org/r/17875/#comment64466>
These can be vals since they are constants.
samza-kafka/src/main/scala/org/apache/samza/util/ExponentialThreadSleepStrategy.scala
<https://reviews.apache.org/r/17875/#comment64467>
in the class, import ExpotentialThreadSleepStrategy._ and we can avoid all the extra qualification, to aid in readability.
samza-kafka/src/main/scala/org/apache/samza/util/ExponentialThreadSleepStrategy.scala
<https://reviews.apache.org/r/17875/#comment64510>
Since the class has been broken out and can be used for more than just redelivery delays, can we make this a more generic name to ease re-use?
samza-kafka/src/test/scala/org/apache/samza/util/TestExponentialThreadSleepStrategy.scala
<https://reviews.apache.org/r/17875/#comment64506>
Need ASF file header
samza-kafka/src/test/scala/org/apache/samza/util/TestExponentialThreadSleepStrategy.scala
<https://reviews.apache.org/r/17875/#comment64508>
Not logging so don't need to extend.
samza-kafka/src/test/scala/org/apache/samza/util/TestExponentialThreadSleepStrategy.scala
<https://reviews.apache.org/r/17875/#comment64509>
Can we extend the test to make sure we don't go past the maximum allowed? ie, we never go above 10000 ms?
- Jakob Homan
On Feb. 11, 2014, 7:33 p.m., Steven Yates wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17875/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2014, 7:33 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-30 - Back off in BrokerProxy when fetch fails
>
>
> Diffs
> -----
>
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala 95a1fb5
> samza-kafka/src/main/scala/org/apache/samza/util/ExponentialThreadSleepStrategy.scala PRE-CREATION
> samza-kafka/src/test/scala/org/apache/samza/util/TestExponentialThreadSleepStrategy.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17875/diff/
>
>
> Testing
> -------
>
> Some
>
>
> Thanks,
>
> Steven Yates
>
>
Re: Review Request 17875: SAMZA-30
Posted by Steven Yates <sy...@stevendyates.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17875/
-----------------------------------------------------------
(Updated Feb. 12, 2014, 3:33 a.m.)
Review request for samza.
Changes
-------
Updated diff inline with suggested modificaitons
Repository: samza
Description
-------
SAMZA-30 - Back off in BrokerProxy when fetch fails
Diffs (updated)
-----
samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala 95a1fb5
samza-kafka/src/main/scala/org/apache/samza/util/ExponentialThreadSleepStrategy.scala PRE-CREATION
samza-kafka/src/test/scala/org/apache/samza/util/TestExponentialThreadSleepStrategy.scala PRE-CREATION
Diff: https://reviews.apache.org/r/17875/diff/
Testing
-------
Some
Thanks,
Steven Yates