RFR(S): 8150865: SQE test: GC unified logging: check that dynamic log level doesn't break anything

[Jesper Wilhelmsson]

Hi Dima,

I would prefer to have the test in gc/logging as long as it doesn't interfere 
with test selection in nightlies etc.

It has Stress in its name so that should be enough to indicate what kind of test 
it is. If there is a lot of randomness in a test a comment in the test would 
help a lot when investigating a potential future failure in the test.

Thanks,
/Jesper

Den 10/5/16 kl. 18:32, skrev Dmitry Fazunenko:
> Hi Jesper,
>
>
> On 10.05.2016 18:51, Jesper Wilhelmsson wrote:
>>
>>
>> Den 10/5/16 kl. 08:12, skrev Dmitry Fazunenko:
>>> Hi Kirill,
>>>
>>> The test looks very good!
>>> Some comments which will make it look even better :)
>>>
>>> 1) Please remove dependency on G1. Unified logging is a common
>>> feature and should be tested in all collectors.
>>> You can keep two kinds of objects: small and large. To avoid confusion,
>>> "large" or "huge" term could be used instead of "humongous".
>>>
>>> 2) Because this is a stress test, please move it: gc/logging --> gc/stress
>>
>> I assume this is done to make it easier to select tests in different test lists?
>>
>> In general I would prefer to sort the tests based on what they test, not what
>> kind of test it is. It's easier to find the right test when one has made a
>> change in some part of the code if all tests related to that code is in the
>> same place.
>>
>> Updating the test lists to manually include/exclude the stress tests is surely
>> a tedious job, but it is something that would be done once for every new test.
>> Trying to find the right set of tests to run on a change is a problem that
>> every developer is facing more or less on a daily basis. I think it is more
>> important to make this the easy task.
>
> You rose a very good question: where to store stress tests.  And I agree with
> you, it's better to store logging tests in the logging directory.
> And use gc/stress for storing general purpose tests like GCOld.
>
> But this particular test (by Kirill) doesn't check any specific logging
> assertion, it just checks that VM doesn't crash trying to log something.
> Because it's based on random, passing of the test doesn't guarantee "no bugs".
> I think it's better to locate it in gc/stress/
> to emphasize that a failure is not necessary caused by the resent changes, but
> may exist before.
>
> If you still believe, the best test for that test is gc/logging, I will not
> object :)
>
>
> Kirill,
>
> New version looks good to me, thanks for the prompt addressing my comments.
> I found one more minor change which will make the test stronger:
>
>    String logLevelCommand = "what='gc=" + LOG_LEVELS[logLevel] + "'";
> -->
>    String logLevelCommand = "what='gc*=" + LOG_LEVELS[logLevel] + "'";
>
>
> I don't need a separate review.
>
> Thanks,
> Dima
>
>
>>
>> /Jesper
>>
>>
>>>
>>>
>>> 3)
>>>
>>> please add:
>>>  * @key gc stress
>>>
>>> you don't need:
>>>  * @build gc.logging.TestUnifiedLoggingSwitchStress
>>>
>>>
>>> 4) Other comments are rather minor recommendations:
>>>
>>>     private final int filesCount;
>>>     private final String fileNamePrefix;
>>> -->
>>>     private final int logCount;  // how many various log files will be used
>>>     private final String logFilePrefix;  // name of log file will be
>>> logFilePrefix + index
>>>
>>>
>>>
>>>     System.out.format("Diagnostic command vmLog with arguments %s,%s returned
>>> not empty output %s\n",
>>> -->
>>>     System.out.format("WARNING: Diagnostic command vmLog with arguments %s,%s
>>> returned not empty output %s\n",
>>>
>>>     LinkedList<Thread> threads = new LinkedList<>();
>>> -->
>>>     List<Thread> threads = new LinkedList<>();
>>>
>>>     Calendar calendar = Calendar.getInstance();
>>>     long startTime = calendar.getTimeInMillis();
>>> -->
>>>     long startTime = System.currentTimeMillis()
>>>
>>>
>>>     while (Calendar.getInstance().getTimeInMillis() - startTime < duration) {
>>>         Thread.yield();
>>>     }
>>> -->
>>>     while (System.currentTimeMillis() - startTime < duration) {
>>>         Thread.sleep(1000);
>>>     }
>>>
>>>
>>>
>>> Thanks,
>>> Dima
>>>
>>> On 06.05.2016 14:10, Kirill Zhaldybin wrote:
>>>> Dear all,
>>>>
>>>> Could you please review this fix for 8150865?
>>>>
>>>> The test creates two types of threads: the first type stresses gc and the
>>>> second type switches gc unified loggingl level/file output.
>>>> The test is a stress test and it is expected that it finishes normally (no oom
>>>> or crash).
>>>>
>>>> WebRev: http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8150865/webrev.00/
>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8150865
>>>>
>>>> Thank you.
>>>>
>>>> Regards, Kirill
>>>
>