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

[Kirill Zhaldybin]

Dmitry,

Thank you for review!

Regards, Kirill

On 10.05.2016 19:32, Dmitry Fazunenko wrote:
> 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
>>>
>