[PATCH] JMC-5499 Add XFlagChanged events to JVM Information or subpage

Fri Jul 12 14:18:37 UTC 2019

Hi Jessye,

On Tue, Jul 9, 2019 at 1:03 PM Jessye Coleman Shapiro <jescolem at redhat.com>
wrote:

> Hi,
>
> Here is the webrev for an updated patch:
> http://cr.openjdk.java.net/~aptmac/JMC-5499/webrev.02/
>

Great, thank you for accommodating the changes. I have two minor nits,
being:
- JVMInformationPage
- - line 48: the new line that separates the eclipse & jmc imports was
removed
- - line 298: there should be a space before the SWT.NONE

These two changes aren't large enough to warrant a re-review, so this looks
good to me!

>
>
> On Thu, Jul 4, 2019 at 1:43 PM Alex Macdonald <almacdon at redhat.com> wrote:
> >
> > On Thu, Jul 4, 2019 at 11:43 AM Jessye Coleman Shapiro <
> jescolem at redhat.com> wrote:
> >>
> >> Here is an updated version of the patch:
> >>  http://cr.openjdk.java.net/~aptmac/JMC-5499/webrev.01/
> >> <http://cr.openjdk.java.net/~aptmac/JMC-5499/webrev.01/>
> >>
> >> I modified the uitests in JVMInformationPageTest to act as general
> checks
> >> for table population instead of asserting exact table entries.  String
> >> comparisons of dates were failing in some environments and not others.
> >
> >
> > The test file looks good, just a couple nits:
> > - there will need to be a license header for the file
> > - the second test is missing the "Log" part in the method name to
> indicate the "LogTable" like written in the previous test
> > - the string value for "Old Value" could be retrieved from Messages
> > - now that JMC-5327 is pushed there's a folder for flightrecorder page
> uitests, this could be placed in there (alongside FileIOPageTest, etc.)
> >
>
> I have added those changed to the patch - thanks
>
> > No issues or failures when running these tests:
> https://travis-ci.org/aptmac/jmc-qa/builds/554307170
> >
> >>
> >>
> >> Let me know what you think,
> >>
> >> Jessye
> >>
> >>
> >> On Tue, Jul 2, 2019 at 3:56 PM Jessye Coleman Shapiro <
> jescolem at redhat.com>
> >> wrote:
> >>
> >> > Here is an updated patch. It includes:
> >> >
> >> > - A message that reads "No 'Flag Changed' events found".  I didn't add
> >> > an explanation for why no events were found because it is possible
> >> > that the table could be empty for two reasons simultaneously- some
> >> > XFlagChanged events could be disabled and others could have simply not
> >> > occurred. However, I am not sure if this message is informative
> >> > enough.  I have placed the message under the title of the table - let
> >> > me know if this is a good location for it.
> >
> >
> > I think this is good for now. I do agree with Marcus's statement about
> not hiding away UI elements that we're not using. My initial inclination
> was that the table itself could be not displayed, but because this table is
> a component of sash form then the header title would still be visible (and
> the sash weights adjusted to reclaim space). However, that's still a
> band-aid solution to the bigger problem of better handling empty states.
> There is already an issue in JIRA for this sort of behaviour (JMC-4009), so
> I'll take a couple ideas and chime in there.
> >
> >> > - Two uitests that test if the table is correctly populated for a
> >> > flight recording that has multiple XFlagChanged events and if the "No
> >> > 'Flag Changed' events found" message appears for a flight recording
> >> > where no XFlagChanged events occur.
> >> > - Removed empty space/lines
> >> >
> >> > Let me know what you think,
> >> >
> >> > Jessye
> >> >
> >> > On Mon, Jun 24, 2019 at 10:58 AM Ken Dobson <kdobson at redhat.com>
> wrote:
> >> > >
> >> > > I agree, a simple string added to the table saying "No X events
> found" or
> >> > > "X event has been disabled" would more than suffice.
> >> > >
> >> > > Ken
> >> > >
> >> > >
> >> > > On Mon, Jun 24, 2019 at 7:07 AM Marcus Hirt <
> marcus.hirt at datadoghq.com>
> >> > > wrote:
> >> > >
> >> > > > Agree. Perhaps now would be a good time to discuss?
> >> > > >
> >> > > > I am, in general, hesitant to automatically hide UI elements. It
> has
> >> > been
> >> > > > tried in the past (e.g. Microsoft hiding menu elements not used
> much),
> >> > with
> >> > > > little success. That said, we should at the very least have some
> >> > feedback
> >> > > > showing people whether the lack of data is due to events being
> >> > disabled or
> >> > > > events not having occurred. What do you guys think?
> >> > > >
> >> > > > Kind regards,
> >> > > > Marcus
> >> > > >
> >> > > > On Thu, Jun 20, 2019 at 8:35 PM Ken Dobson <kdobson at redhat.com>
> wrote:
> >> > > >
> >> > > >> There's a ticket here about how we should handle empty tables
> >> > > >> https://bugs.openjdk.java.net/browse/JMC-4009
> >> > > >> I think it's an issue that might have a larger scope than just
> this
> >> > bug
> >> > > >> but
> >> > > >> it's definitely worth addressing.
> >> > > >>
> >> > > >> Ken Dobson
> >> > > >>
> >> > > >> On Thu, Jun 20, 2019 at 1:30 PM Alex Macdonald <
> almacdon at redhat.com>
> >> > > >> wrote:
> >> > > >>
> >> > > >> > On Thu, Jun 20, 2019 at 9:05 AM Jessye Coleman Shapiro <
> >> > > >> > jescolem at redhat.com>
> >> > > >> > wrote:
> >> > > >> >
> >> > > >> > > Hi Alex,
> >> > > >> > >
> >> > > >> > > I will make those changes and send an updated patch.
> >> > > >> > >
> >> > > >> >
> >> > > >> > Err, whoops. I was reviewing your JMC-5706 patch at the same
> time as
> >> > > >> this
> >> > > >> > one, and must have applied the other patch when I thought it
> was
> >> > this
> >> > > >> one.
> >> > > >> > The previous review actually applies to your JMC-5706 patch,
> so I'll
> >> > > >> copy
> >> > > >> > the review on that thread.
> >> > > >> >
> >> > > >> > As for this patch:
> >> > > >> >
> >> > > >> > My initial reaction was that the new table uses up a good
> portion
> >> > of the
> >> > > >> > page real-estate (which is okay), but I'm wondering if the
> table
> >> > should
> >> > > >> > only be displayed if it has values to display. Perhaps someone
> else
> >> > > >> could
> >> > > >> > share their opinions as well.
> >> > > >> >
> >> > > >> > There are empty spaces/lines at the following locations:
> >> > > >> > - defaultPages.xml @ line 597
> >> > > >> > - JVMInformationPage.java @ lines 122, 162, 187, 227, 231,
> 258, 262,
> >> > > >> 295,
> >> > > >> > 296, 303, 305, 342, 346
> >> > > >> >
> >> > > >> > The build & uitests pass [0], however because a new component
> was
> >> > added
> >> > > >> to
> >> > > >> > the UI I'd like to see a uitest to ensure that everything is
> >> > working as
> >> > > >> > intended.
> >> > > >> >
> >> > > >> > [0] https://travis-ci.org/aptmac/jmc-qa/builds/548262559
> >> > > >> >
> >> > > >> >
> >> > > >> > >
> >> > > >> > > Thank you!
> >> > > >> > >
> >> > > >> > > Jessye
> >> > > >> > >
> >> > > >> > > On Wed, Jun 19, 2019 at 3:45 PM Alex Macdonald <
> >> > almacdon at redhat.com>
> >> > > >> > > wrote:
> >> > > >> > >
> >> > > >> > >> Hi Jessye,
> >> > > >> > >>
> >> > > >> > >> On Fri, Jun 7, 2019 at 2:48 PM Jessye Coleman Shapiro <
> >> > > >> > >> jescolem at redhat.com> wrote:
> >> > > >> > >>
> >> > > >> > >>> Hi,
> >> > > >> > >>>
> >> > > >> > >>> This patch addresses JMC-5499: Add XFlagChanged events to
> JVM
> >> > > >> > Information
> >> > > >> > >>> or subpage [0].  I have added a JVM Flags Log table to the
> JVM
> >> > > >> > Internals
> >> > > >> > >>> Page that displays XFlagChanged events.
> >> > > >> > >>>
> >> > > >> > >>> Please see the attached patch and let me know what you
> think.
> >> > > >> > >>>
> >> > > >> > >>
> >> > > >> > >> The content of this patch look good to me.
> >> > > >> > >>
> >> > > >> > >> However, there are test failures:
> >> > > >> > >>
> >> > > >> > >> >
> TestRulesWithJfr.verifyAllResults:132->verifyRuleResults:159
> >> > > >> > >>
> >> > > >> > >> because the generated JfrRuleBaseline.xml doesn't match the
> >> > hardcopy
> >> > > >> one
> >> > > >> > >> used to verify the test. This will need to be updated.
> >> > > >> > >>
> >> > > >> > >> There's also some empty spaces that can be cleaned up.
> >> > > >> > >>
> >> > > >> > >> JavaBlockingRule.java
> >> > > >> > >> - empty space @ lines 134, 138, 151
> >> > > >> > >> - trailing empty space @ line 142
> >> > > >> > >>
> >> > > >> > >>
> >> > > >> > >>>
> >> > > >> > >>> Thank you!
> >> > > >> > >>>
> >> > > >> > >>> Jessye Coleman-Shapiro
> >> > > >> > >>>
> >> > > >> > >>>  [0] https://bugs.openjdk.java.net/browse/JMC-5499
> >> > > >> > >>
> >> > > >> > >>
> >> > > >> > >> Cheers,
> >> > > >> > >>
> >> > > >> > >> Alex
> >> > > >> > >>
> >> > > >> > >
> >> > > >> >
> >> > > >>
> >> > > >
> >> >
>