Gently reminder

On 17.10.2014 13:38, Konstantin Shefov wrote:
Hi,

I have updated the webrev: http://cr.openjdk.java.net/~kshefov/8059070/webrev.01/

-Konstantin

16.10.2014 17:24, Igor Ignatyev пишет:
Konstantin,

I haven't looked at code religiously, so I wouldn't say that I have reviewed it. however I have some comments, please see them inline.

On 10/16/2014 02:03 PM, Konstantin Shefov wrote:
Paul,

Thanks for reviewing

In the jtreg scripts of the three existing LFCaching tests timeout is
set explicitly to 300 seconds. The file currently being changed is not a
test itself, it is parent class of tests.
In fact we can unset this explicit timeout and use the current fix
together with default JTREG timeout and jtreg timeout factor option.
These test cases are randomly generated, so the more iterations, the
better, but in fact we need at least one iteration.

As for "if (avgIterTime > 2 * remainTime)", I think 2 is enough.

-Konstantin

On 16.10.2014 13:30, Paul Sandoz wrote:
On Oct 16, 2014, at 10:43 AM, Konstantin Shefov
<[email protected]> wrote:

Gently reminder

On 14.10.2014 16:58, Konstantin Shefov wrote:
Hello,

Please review the test bug fix
https://bugs.openjdk.java.net/browse/JDK-8059070
Webrev is http://cr.openjdk.java.net/~kshefov/8059070/webrev.00/

   45     private static final long TIMEOUT = 300000;

Does jtreg define a system property for this value? and is 300s the
same as what jtreg defines as the default?
unfortunately, jtreg doesn't define a property for timeout, but I think it should do. we need to file an RFE against jtreg to add such a property and an RFE against these tests/testlibrary to use this property. could you please file them?

The online documentation (jtreg -onlineHelp) says the default is 2
minutes, but that might be out of date.

Might be more readable to use:

   TimeUnit.SECONDS.toMillis(300);


  143             long passedTime = new Date().getTime() - startTime;

Why don't you use System.currentTimeMillis() instead of "new
Date().getTime()" ?


  145             double timeoutFactor = new
Double(System.getProperty("test.timeout.factor", "1.0"));

You can that pull out into a static and perhaps merge with TIMEOUT.
+ you should use jdk.testlibrary.Utils::adjustTimeout instead of writing this code again.


  147             if (avgIterTime > 2 * remainTime) {

That seems sufficient but it will be interesting to see if
intermittent failures still occur due to high variance.

Paul.



Igor


Reply via email to