New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TkRetry #430 #493

Merged
merged 8 commits into from Jan 8, 2016

Conversation

Projects
None yet
6 participants
@HDouss
Copy link
Contributor

HDouss commented Dec 31, 2015

Resolving #430. Nearly the same as the last suggested PR.
Added an assertion on the last test to ensure expected response is returned after the first fail.

@HDouss

This comment has been minimized.

Copy link
Contributor

HDouss commented Dec 31, 2015

@davvd PR ready for review.

@davvd

This comment has been minimized.

Copy link

davvd commented Dec 31, 2015

@HDouss I will find a reviewer for your pull requests shortly, thanks for contribution!

@davvd

This comment has been minimized.

Copy link

davvd commented Dec 31, 2015

@pinaf review this one, please

* Maximum number of retry attempts.
*/
private final transient Integer count;
/**

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss let's add a blank line here for consistency.

/**
* Maximum number of retry attempts.
*/
private final transient Integer count;

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss why not a primitive int?

/**
* Amount of time between retries, in milliseconds.
*/
private final transient Integer delay;

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss why not a primitive int?

* @param retrydelay Time between retries
* @param originaltake Original take
*/
public TkRetry(final Integer retry, final Integer

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss why not a primitive int?

This comment has been minimized.

@pinaf

This comment has been minimized.

@HDouss

HDouss Jan 1, 2016

Contributor

@pinaf What's the problem with retries ?

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss huh? just asking to rename the variable to retries instead of retry.

* @param originaltake Original take
*/
public TkRetry(final Integer retry, final Integer
retrydelay, final Take originaltake) {

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss we should avoid composite names: originaltake -> original

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss we should avoid composite names: retrydelay -> delay

}

/**
* Added retrying logic.

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss I don't see a need for this javadoc since the class javadoc explains what is done and act is an inherited method.

try {
return this.take.act(req);
} catch (final IOException ex) {
attempts = attempts + 1;

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss why not ++attempts?

*/
private void sleep() {
try {
Thread.sleep(this.delay);

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss how about TimeUnit.MILLISECONDS.sleep(this.delay)? makes it clear we are using ms

@Override
public Response act(final Request req) throws IOException {
int attempts = 0;
IOException exeption = new IOException();

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss typo: exception

public void worksWithNoException() throws Exception {
final String test = "test";
MatcherAssert.assertThat(
new RsPrint(

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss too much indentation here


/**
* TkRetry can work when no IOException.
*

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss no need for these blank lines

This comment has been minimized.

@HDouss

HDouss Jan 1, 2016

Contributor

@pinaf That's what is done in almost every class in the project

/**
* TkRetry can retry when initial take fails with IOException,
* till retry count is reached.
*

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss no need for these blank lines

This comment has been minimized.

@HDouss

HDouss Jan 1, 2016

Contributor

@pinaf That's what is done in almost every class in the project

/**
* TkRetry can retry when initial take fails with IOException,
* till get successful result.
*

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss no need for these blank lines

This comment has been minimized.

@HDouss

HDouss Jan 1, 2016

Contributor

@pinaf That's what is done in almost every class in the project


/**
* Constructor.
*

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss no need for these blank lines

Mockito.when(take.act(Mockito.any(Request.class))).thenThrow(
new IOException()
);
final long startTime = System.currentTimeMillis();

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss let's avoid composite names: start is fine

MatcherAssert.assertThat(
new Long(count * delay),
Matchers.lessThanOrEqualTo(
System.currentTimeMillis() - startTime

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss too much indentation here

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss let's measure delays using System.nanoTime since currentTimeMillis is very inaccurate. Also, we should capture the current time as soon as the action is complete on line 84.

final int count = 3;
final int delay = 1000;
final Take take = Mockito.mock(Take.class);
final Request req = new RqFake(RqMethod.GET);

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss we can inline this var

}

/**
* TkRetry can retry when initial take fails with IOException,

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss fails once with IOException

*/
@Test
public void retriesOnExceptionTillSuccess() throws Exception {
final int count = 3;

This comment has been minimized.

@pinaf

This comment has been minimized.

@HDouss

HDouss Jan 1, 2016

Contributor

@pinaf Right ! Have you an idea why Chekstyle is not complaining with MagicNumberCheck rule ?

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss Nope! I wondered about that also... maybe some kind of suppression in pom.xml ?

@Test
public void retriesOnExceptionTillSuccess() throws Exception {
final int count = 3;
final int delay = 1000;

This comment has been minimized.

@pinaf
*/
@Test(expected = IOException.class)
public void retriesOnExceptionTillCount() throws Exception {
final int count = 3;

This comment has been minimized.

@pinaf
@Test(expected = IOException.class)
public void retriesOnExceptionTillCount() throws Exception {
final int count = 3;
final int delay = 1000;

This comment has been minimized.

@pinaf
final int delay = 1000;
final String data = "data";
final Take take = Mockito.mock(Take.class);
Mockito.when(take.act(Mockito.any(Request.class))).thenThrow(

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss let's format it like this

Mockito
    .when(take.act(Mockito.any(Request.class)))
    .thenThrow(new IOException())
    .thenReturn(new RsText(data));
final int delay = 1000;
final Take take = Mockito.mock(Take.class);
final Request req = new RqFake(RqMethod.GET);
Mockito.when(take.act(Mockito.any(Request.class))).thenThrow(

This comment has been minimized.

@pinaf

pinaf Jan 1, 2016

@HDouss let's format it like this

Mockito
    .when(take.act(Mockito.any(Request.class)))
    .thenThrow(new IOException());
@HDouss

This comment has been minimized.

Copy link
Contributor

HDouss commented Jan 6, 2016

@yegor256 Thanks. Done !

private static List<String> strings(final List<IOException> failures) {
final List<String> result = new ArrayList<String>(failures.size());
final Iterable<String> transform = new Transform<IOException, String>(
failures,

This comment has been minimized.

@yegor256

yegor256 Jan 6, 2016

Owner

@HDouss what happened with indentation here?

@yegor256

This comment has been minimized.

Copy link
Owner

yegor256 commented Jan 6, 2016

@HDouss something is wrong with indentation

@HDouss

This comment has been minimized.

Copy link
Contributor

HDouss commented Jan 6, 2016

@yegor256 You are right. Corrected.

@HDouss

This comment has been minimized.

Copy link
Contributor

HDouss commented Jan 6, 2016

@yegor256 Unit tests are failing due to inaccuracy in thread sleep (OS/Hardware dependent). Travis build is always suceeding, not the Appveyor one. Could I add a tolerance in the tests ? Or do you have another suggestion ?

@yegor256

This comment has been minimized.

Copy link
Owner

yegor256 commented Jan 6, 2016

@HDouss well, you should not make any assumptions in unit tests about the speed of CPU. I see, that you're actually making them. that's wrong.

@HDouss

This comment has been minimized.

Copy link
Contributor

HDouss commented Jan 6, 2016

@yegor256 I am not really making such assumptions. We are testing that TkRetry actually slept at least the delay amount after crashing once. What happens is that 'sometimes' sleeping takes 5 to 6 ms less than expected. Do you suggest removing such a test ?

@HDouss

This comment has been minimized.

Copy link
Contributor

HDouss commented Jan 6, 2016

@yegor256 It is not really a CPU speed assumption. It is inaccuracy in Thread.sleep jdk implementation that is (slightly) dependent on OS and/or hardware.

@yegor256

This comment has been minimized.

Copy link
Owner

yegor256 commented Jan 7, 2016

@HDouss maybe we can check that it slept at least the delay-1 amount of seconds?

@HDouss

This comment has been minimized.

Copy link
Contributor

HDouss commented Jan 7, 2016

@yegor256 delay is often just one second, delay - 1 would give 0. I added a tolerance of 100 milliseconds (delay - 100ms), which is more than enough.

@HDouss

This comment has been minimized.

Copy link
Contributor

HDouss commented Jan 7, 2016

@yegor256 Build suceeds. Please ask to merge.

@HDouss

This comment has been minimized.

Copy link
Contributor

HDouss commented Jan 8, 2016

@yegor256 Please ask for merge.

@yegor256

This comment has been minimized.

Copy link
Owner

yegor256 commented Jan 8, 2016

@rultor try to merge

@rultor

This comment has been minimized.

Copy link
Collaborator

rultor commented Jan 8, 2016

@rultor try to merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit b846c8f into yegor256:master Jan 8, 2016

3 checks passed

Scrutinizer No new issues
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rultor

This comment has been minimized.

Copy link
Collaborator

rultor commented Jan 8, 2016

@rultor try to merge

@yegor256 Done! FYI, the full log is here (took me 23min)

@davvd

This comment has been minimized.

Copy link

davvd commented Jan 10, 2016

@ypshenychka please, review this task for compliance with our quality rules

@davvd

This comment has been minimized.

Copy link

davvd commented Jan 10, 2016

@rultor please deploy

@rultor

This comment has been minimized.

Copy link
Collaborator

rultor commented Jan 10, 2016

@rultor please deploy

@davvd OK, I'll try to deploy now. You can check the progress here

@rultor

This comment has been minimized.

Copy link
Collaborator

rultor commented Jan 10, 2016

@rultor please deploy

@HDouss @davvd Oops, I failed. You can see the full log here (spent 10min)

INFO: "reading src/test/java/org/takes/rq/RqWithHeaderTest.java..."
INFO: "reading src/test/java/org/takes/rq/RqWithHeadersTest.java..."
INFO: "reading src/test/java/org/takes/rq/RqWithoutHeaderTest.java..."
INFO: "reading src/test/java/org/takes/rq/package-info.java..."
INFO: "reading src/test/java/org/takes/rs/RsGzipTest.java..."
INFO: "reading src/test/java/org/takes/rs/RsJSONTest.java..."
INFO: "reading src/test/java/org/takes/rs/RsPrettyJSONTest.java..."
INFO: "reading src/test/java/org/takes/rs/RsPrettyXMLTest.java..."
INFO: "reading src/test/java/org/takes/rs/RsPrintTest.java..."
INFO: "reading src/test/java/org/takes/rs/RsTextTest.java..."
INFO: "reading src/test/java/org/takes/rs/RsVelocityTest.java..."
INFO: "reading src/test/java/org/takes/rs/RsWithCookieTest.java..."
INFO: "reading src/test/java/org/takes/rs/RsWithHeaderTest.java..."
INFO: "reading src/test/java/org/takes/rs/RsWithHeadersTest.java..."
INFO: "reading src/test/java/org/takes/rs/RsWithStatusTest.java..."
INFO: "reading src/test/java/org/takes/rs/RsWithTypeTest.java..."
INFO: "reading src/test/java/org/takes/rs/RsWithoutHeaderTest.java..."
INFO: "reading src/test/java/org/takes/rs/RsXSLTTest.java..."
INFO: "reading src/test/java/org/takes/rs/package-info.java..."
INFO: "reading src/test/java/org/takes/rs/xe/RsXemblyTest.java..."
INFO: "reading src/test/java/org/takes/rs/xe/XeAppendTest.java..."
INFO: "reading src/test/java/org/takes/rs/xe/XeDateTest.java..."
INFO: "reading src/test/java/org/takes/rs/xe/XeLinkHomeTest.java..."
INFO: "reading src/test/java/org/takes/rs/xe/XeLinkSelfTest.java..."
INFO: "reading src/test/java/org/takes/rs/xe/XeLinkTest.java..."
INFO: "reading src/test/java/org/takes/rs/xe/XeLocalhostTest.java..."
INFO: "reading src/test/java/org/takes/rs/xe/XeSLATest.java..."
INFO: "reading src/test/java/org/takes/rs/xe/XeTransformTest.java..."
INFO: "reading src/test/java/org/takes/rs/xe/XeWhenTest.java..."
INFO: "reading src/test/java/org/takes/rs/xe/package-info.java..."
INFO: "reading src/test/java/org/takes/tk/TkCORSTest.java..."
INFO: "reading src/test/java/org/takes/tk/TkClasspathTest.java..."
INFO: "reading src/test/java/org/takes/tk/TkFilesTest.java..."
INFO: "reading src/test/java/org/takes/tk/TkGzipTest.java..."
INFO: "reading src/test/java/org/takes/tk/TkHTMLTest.java..."
INFO: "reading src/test/java/org/takes/tk/TkMeasuredTest.java..."
INFO: "reading src/test/java/org/takes/tk/TkProxyTest.java..."
INFO: "puzzle 377-e6ce369a 30/DEV at src/test/java/org/takes/tk/TkProxyTest.java"
INFO: "reading src/test/java/org/takes/tk/TkReadAlwaysTest.java..."
INFO: "reading src/test/java/org/takes/tk/TkRedirectTest.java..."
INFO: "reading src/test/java/org/takes/tk/TkRetryTest.java..."
INFO: "reading src/test/java/org/takes/tk/TkSlf4jTest.java..."
INFO: "reading src/test/java/org/takes/tk/TkTextTest.java..."
INFO: "reading src/test/java/org/takes/tk/TkVerboseTest.java..."
INFO: "reading src/test/java/org/takes/tk/TkVersionedTest.java..."
INFO: "reading src/test/java/org/takes/tk/TkWithHeadersTest.java..."
INFO: "reading src/test/java/org/takes/tk/package-info.java..."
INFO: "reading src/test/resources/log4j.properties..."
INFO: "reading src/test/resources/org/takes/rs/inclass/imported.xsl..."
INFO: "reading src/test/resources/org/takes/rs/included.xsl..."
INFO: "reading src/test/resources/org/takes/rs/includes.xsl..."
INFO: "reading src/test/resources/org/takes/rs/simple.xsl..."
INFO: "reading src/www.takes.org/index.html..."
+ s3cmd --no-progress put takes.xml --config=../s3cfg s3://pdd.teamed.io/takes.xml
WARNING: Module python-magic is not available. Guessing MIME types based on file extensions.
File 'takes.xml' stored as 's3://pdd.teamed.io/takes.xml' (1387 bytes in 0.1 seconds, 19.70 kB/s) [1 of 1]
+ est --dir=./est --verbose --file=takes.xml --format=xml
/home/r/script.sh: line 7: est: command not found
container 2b173cd76adb1c49ad914403084ed8fc6a707a77195541db212f97737caedc95 is dead

@ypshenychka

This comment has been minimized.

Copy link

ypshenychka commented Jan 10, 2016

@davvd Quality is good.

@davvd

This comment has been minimized.

Copy link

davvd commented Jan 10, 2016

@davvd Quality is good.

@ypshenychka thanks a lot

@davvd

This comment has been minimized.

Copy link

davvd commented Jan 10, 2016

@pinaf I added 10 mins to @ypshenychka (for QA review) in transaction 74106560. paid, thanks, added 52 mins to your account, payment AP-5CH84461N24789621, 206 hours and 48 mins was spent. review comments (c=37) added as a bonus. +52 added to your rating, at the moment it is: +5246

yegor256 added a commit that referenced this pull request Jan 10, 2016

@yegor256

This comment has been minimized.

Copy link
Owner

yegor256 commented Jan 10, 2016

@rultor deploy

@rultor

This comment has been minimized.

Copy link
Collaborator

rultor commented Jan 10, 2016

@rultor deploy

@yegor256 OK, I'll try to deploy now. You can check the progress here

@rultor

This comment has been minimized.

Copy link
Collaborator

rultor commented Jan 10, 2016

@rultor deploy

@yegor256 Done! FYI, the full log is here (took me 13min)

@HDouss HDouss deleted the HDouss:#430 branch Jan 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment