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

[Igor Ignatyev]

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().
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?
4) to make the test more robust, at line #118 I'd compare eventsDeliveredWhileProducerIsAlive w/ 1 instead of 1/2 of eventsDeliveredTotal.
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)
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)
7) is it really necessary to wait till "producer" process main is started?
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()
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?
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) ?
9.1) and if EventStream::onEvent is expected to preserve events order, that 'id' is strictly increasing?

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