On Thu, Jun 14, 2018 at 2:48 PM, Lindenmaier, Goetz <[email protected]> wrote: > Hi Thomas, > > thanks for your review! > New webrev: > http://cr.openjdk.java.net/~goetz/wr18/8204654-fixJstatTest/02/ > I'll run this through our testing again and push tomorrow if all > is green. > >> consider better comment: >> -# The awk scripts parsing the output do not respect any locale >> dependent setting. >> +# The awk scripts parsing the jstat output expect it to be in en-us locale. > Fixed. > >> I assume >> ([0-9])|-+ >> should match a single whole number for column "FindClass", or "-"? > Yes, fixed. > >> similar here: + misplaced: >> - ([0-9]+\.[0-9])|-+ >> + ([0-9]+\.[0-9]+)|- > Also fixed. Great catch! > >> Also, it would be helpful if you could print column name or at least >> number atop the matching sequences, in a comment, that makes the >> reading easer. > Yes, but there are a row of similar files I don't touch. Don't want to do > this for all of them. >
I totally understand :) >> But this is hard to read! Any chance of splitting this expression into >> sub expressions? > Yes, this is ugly. > But I think the whole tests should be rewritten to do the parsing in Java, > as the jstatd tests do. I think this is out of scope here. Sure. > > Best regards, > Goetz. I do not need to see a new webrev. ..Thomas > > > >> >> >> Rest seems fine. >> >> ..Thomas >> >> On Mon, Jun 11, 2018 at 3:14 PM, Lindenmaier, Goetz >> <[email protected]> wrote: >> > Hi, >> > >> > please review this test fix: >> > http://cr.openjdk.java.net/~goetz/wr18/8204654-fixJstatTest/01 >> > >> > gcCauseOutput1.awk: >> > The pattern scans 11 numbers, while the output contains 13. Also, more '-' >> are possible then checked. >> > >> > The other awk scripts need to check more patterns where '-' can appear. >> > >> > We have a machine with user.country=de where jstat prints ',' instead of >> > '.' >> in numbers. Explicitly >> > start with user.country=en as already done for user.language=en. >> > >> > I also refactored the common flags to a variable in utils.sh ... so there >> > won't >> be the need >> > to edit all these files once more :) >> > >> > Best regards, >> > Goetz >> > >> > (Sorry for the second mail, first missed the bug text ...)
