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

[Kirill Zhaldybin]

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
>>>>>
>>>
>