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

[Dmitry Fazunenko]

Looks good!

-- Dima

On 10.05.2016 21:17, Kirill Zhaldybin wrote:
> Dmitry, Jesper,
>
> Thank you for comments.
>
> Here are a new WebRev: 
> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8150865/webrev.03/
>
> Regards, Kirill
>
> On 10.05.2016 20:55, Dmitry Fazunenko wrote:
>> Jesper,
>>
>> thanks for answer.
>>
>> Kirill,
>>
>> would you move the test back and exclude it from the hotspot_fast_gc_2
>> group
>> (hotspot/test/TEST.groups)
>>
>> Thanks,
>> Dima
>>
>> On 10.05.2016 20:42, Jesper Wilhelmsson wrote:
>>> 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
>>>>>>
>>>>
>>
>