[ https://issues.apache.org/jira/browse/HBASE-7384?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13540604#comment-13540604 ]
Ted Yu edited comment on HBASE-7384 at 12/28/12 9:42 PM: --------------------------------------------------------- Putting patch on review board would make reviewing easier. {code} + * A class that provides a standard waitFor implementation pattern {code} Remove 'implementation' above. {code} +public abstract class Waiter { {code} I don't see abstract method inside Waiter. The class shouldn't be abstract. {code} + private static final Log LOG = LogFactory.getLog(Bytes.class); {code} Fix class name above. {code} + public static final String TEST_WAITFOR_RATIO_PROP = "test.waitfor.ratio"; {code} Please explain the meaning of the ratio. {code} + * {@link #waitFor(long, long, Predicate)} and {@link #waitFor(long, long, boolean, Predicate)} method {code} Wrap long line above. 'method' -> 'methods' {code} + * This is useful when running tests in slow machines for tests that are time sensitive. {code} Rephrase the above as 'This is useful when running time sensitive tests on slow machines' Currently setWaitForRatio() is not called. In what circumstance would it be used ? {code} + * Returns the 'wait for ratio' used in the {@link #sleep(long)}, {@link #waitFor(int, Predicate)} + * and {@link #waitFor(int, boolean, Predicate)} methods for the current test class. <p/> This is {code} I think the parameter types for the two waitFor() methods are wrong. {code} + * useful when running tests in slow machines for tests that are time sensitive. <p/> The default {code} Suggest similar rephrase for above. {code} + * value is obtained from the Java System property <code>test.wait.for.ratio</code> which defaults {code} The property name is different from the value for TEST_WAITFOR_RATIO_PROP {code} + * Makes the current thread sleep for the specified number of milliseconds. {code} The above description is not accurate, please mention the role of WaitForRatio More reviews to follow was (Author: yuzhih...@gmail.com): Putting patch on review board would make reviewing easier. {code} + * A class that provides a standard waitFor implementation pattern {code} Remove 'implementation' above. {code} +public abstract class Waiter { {code} I don't see abstract method inside Waiter. The class shouldn't be abstract. {code} + private static final Log LOG = LogFactory.getLog(Bytes.class); {code} Fix class name above. {code} + public static final String TEST_WAITFOR_RATIO_PROP = "test.waitfor.ratio"; {code} Please explain the meaning of the ratio. {code} + * {@link #waitFor(long, long, Predicate)} and {@link #waitFor(long, long, boolean, Predicate)} method {code} Wrap long line above. 'method' -> 'methods' {code} + * This is useful when running tests in slow machines for tests that are time sensitive. {code} Rephrase the above as 'This is useful when running time sensitive tests on slow machines' Currently setWaitForRatio() is not called. In what circumstance would it be used ? {code} + * Returns the 'wait for ratio' used in the {@link #sleep(long)}, {@link #waitFor(int, Predicate)} + * and {@link #waitFor(int, boolean, Predicate)} methods for the current test class. <p/> This is {code} I think the parameter types for the two waitFor() methods are wrong. {code} + * useful when running tests in slow machines for tests that are time sensitive. <p/> The default {code} Suggest similar rephrase for above. {code} + * value is obtained from the Java System property <code>test.wait.for.ratio</code> which defaults {code} The property name is different from the value for TEST_WAITFOR_RATIO_PROP {code} {code} + * Makes the current thread sleep for the specified number of milliseconds. {code} The above description is not accurate, please mention the role of WaitForRatio More reviews to follow > Introducing waitForCondition function into test cases > ----------------------------------------------------- > > Key: HBASE-7384 > URL: https://issues.apache.org/jira/browse/HBASE-7384 > Project: HBase > Issue Type: Test > Components: test > Reporter: Jeffrey Zhong > Assignee: Jeffrey Zhong > Labels: test > Fix For: 0.96.0 > > Attachments: hbase-7384_1.0.patch, hbase-7384.patch, Waiter.java > > > Recently I'm working on flaky test cases and found we have many places using > while loop and sleep to wait for a condition to be true. There are several > issues in existing ways: > 1) Many similar code doing the same thing > 2) When time out happens, different errors are reported without explicitly > indicating a time out situation > 3) When we want to increase the max timeout value to verify if a test case > fails due to a not-enough time out value, we have to recompile & redeploy code > I propose to create a waitForCondition function as a test utility function > like the following: > {code} > public interface WaitCheck { > public boolean Check() ; > } > public boolean waitForCondition(int timeOutInMilliSeconds, int > checkIntervalInMilliSeconds, WaitCheck s) > throws InterruptedException { > int multiplier = 1; > String multiplierProp = System.getProperty("extremeWaitMultiplier"); > if(multiplierProp != null) { > multiplier = Integer.parseInt(multiplierProp); > if(multiplier < 1) { > LOG.warn(String.format("Invalid extremeWaitMultiplier > property value:%s. is ignored.", multiplierProp)); > multiplier = 1; > } > } > int timeElapsed = 0; > while(timeElapsed < timeOutInMilliSeconds * multiplier) { > if(s.Check()) { > return true; > } > Thread.sleep(checkIntervalInMilliSeconds); > timeElapsed += checkIntervalInMilliSeconds; > } > assertTrue("WaitForCondition failed due to time out(" + > timeOutInMilliSeconds + " milliseconds expired)", > false); > return false; > } > {code} > By doing the above way, there are several advantages: > 1) Clearly report time out error when such situation happens > 2) Use System property extremeWaitMultiplier to increase max time out > dynamically for a quick verification > 3) Standardize current wait situations > Pleas let me know what your thoughts on this. > Thanks, > -Jeffrey -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators For more information on JIRA, see: http://www.atlassian.com/software/jira