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

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