RFR(S): 8229209: [TESTBUG] test for cross-process JFR event streaming

[Mikhailo Seledtsov]

Hi Igor,

   Thank you for reviewing the test.

The original question in your first email: I do not think it is 
necessary to run the test in othervm mode; I will remove the 'othervm' 
from at-run. The crashing process is the child of the main test process, 
so this should not affect the state of the main test VM.

On 11/12/19, 9:59 PM, Igor Ignatyev wrote:
> Hi Misha,
>
> I've finished reading, several more comments:
>
> 2) in consumeEvents at line #167, it'd be clearer and safer to use TestEvent.class.getName().
Fixed
> 3) instead of asking JVM which you create its system properties in getJfrRepository(), can we just start "producer" process w/ jdk.jfr.repository being set to a well-know location?
AFAIK, JFR sub-system picks the location automatically. Part of location 
is a base directory, e.g. "/private/var/folders/" on Linux, while the 
second part is picked at runtime based on time stamp and PID. I would 
like to leave this as is for now, while I start a discussion with JFR 
team on how to improve this. This is a good point. As a consideration, 
the user should be able to specify this property, or at least the base 
directory for JFR repositories. Once this is resolved, I will update the 
test accordingly.
> 4) to make the test more robust, at line #118 I'd compare eventsDeliveredWhileProducerIsAlive w/ 1 instead of 1/2 of eventsDeliveredTotal.
OK.
> 5) at line #110 please use Asserts.assertEqual instead of Assert.assertTrue(a == b) as it not just compare the values but also prints them out in case !(a==b)
Fixed
> 6) at line #118 please use Asserts.assertGreaterThanOrEqual instead of Assert.assertTrue(a>= b) as it not just compare the values but also prints them out in case !(a>=b)
Fixed
> 7) is it really necessary to wait till "producer" process main is started?
This is necessary to make sure that VM has initialized to a point where 
it can correctly receive JCMD requests. We ran into this type of issue 
before.
> 7.1) if it is, you should use String::equals at line #204 instead of contains
> 7.2) if the answer of 7) is true, I'd recommend you to use j.u.c.CountDownLatch (w/ count = 1) instead of AtomicBoolean for producerMainStarted (you might need to find a better name, e.g. producerStartedSignal) to convey a signal that "producer" process reached its main, so line#205 will become producerMainStarted.countDown(), and line#142 -- producerMainStarted.await()
Thanks for suggesting during our conversation the use of 
ProcessTools.startProcess() with wait-until-predicate. I will use this, 
it will simplify the code and be more declarative.
> 8) I find usage of AtomicIntegeres as a way to return two values from consumeEvents() at a very least confusing, could you please create a "record" class for that purpose instead?
I can do that, as we have discussed.
> 9) local 'id' (line #169) isn't use anywhere. will it be beneficial to check that its value is b/w 0 and (NUMBER_OF_EVENTS_PRODUCED - 1) and, for testProducerExitsNormally case, that sum of all id is  ((NUMBER_OF_EVENTS_PRODUCED - 1) * NUMBER_OF_EVENTS_PRODUCED / 2) ?
The id is not necessary. I used it for development and experimentation. 
I will remove it.
> 9.1) and if EventStream::onEvent is expected to preserve events order, that 'id' is strictly increasing?
No guarantee of that.

I will make the updates, test, and post the updated webrev.

Thank you,
Misha
>
> Thanks,
> -- Igor
>
>> On Nov 12, 2019, at 9:12 PM, Igor Ignatyev<igor.ignatyev at oracle.com>  wrote:
>>
>> Hi Misha,
>>
>> I haven't read the test code thoroughly yet, but I have a question -- is there a reason for TestCrossProcessStreaming to be run in othervm mode?
>>
>> Thanks,
>> -- Igor
>>
>>> On Nov 12, 2019, at 4:20 PM, Mikhailo Seledtsov<mikhailo.seledtsov at oracle.com>  wrote:
>>>
>>> Please review this new JFR test. In this test the producer of JFR events and the consumer run in separate processes.
>>> This is expected to be a common scenario with JFR streaming for monitoring applications and services.
>>>
>>>    JBS: https://bugs.openjdk.java.net/browse/JDK-8229209
>>>    Webrev: http://cr.openjdk.java.net/~mseledtsov/8229209.01/
>>>    Testing:
>>>      - ran this new test locally (Mac, Linux-x64) multiple times - PASS
>>>      - running this test on the test cluster - in progress
>>>
>>>
>>> Thank you,
>>> Misha
>>>