Surya Hebbar has posted comments on this change. ( http://gerrit.cloudera.org:8080/22482 )
Change subject: IMPALA-13751: Fix runtime-profile-test failure since IMPALA-13304 ...................................................................... Patch Set 4: (11 comments) > Patch Set 3: > > (4 comments) > > The use of randomization make it difficult to triage if this test becomes > flaky again in the future. Selected numbers are not printed to JUnit XML when > test failed. Hence, I suggest to fix some numbers here instead of randomizing > them. The reason for creating additional test fixtures, or adding these many tests was to provide all possible test scenarios, to randomly assign bucket sizes and number of profiles. For ensuring they work without breaking for a certain pattern of missing events or during aggregation. Please be assured the tests will not be flaky anymore. Also, the tests will be faster thanks to minimizing sleep timers. I have added the test coverage for expected missing events. Please realize the test coverage has not been reduced, I have only refactored the code into a common validation method. Also, if necessary, instead of VLOG, I can try recording parameters into JUnitXML using gtest-util's RecordProerty. http://gerrit.cloudera.org:8080/#/c/22482/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22482/3//COMMIT_MSG@16 PS3, Line 16: -> Possibility of missing events within the test environment itself > How this may happen? When I run the 50-100 of the same tests in parallell using GNU parallel to exhaust system resources, inadvertently the test environment skips one or two events due to resource exhaustion. But, I had covered it in case something similar was happening on centOS. It is very hard to even simulate such a test, but if the system is running on low resources and high parallelism this may happen. I realize, in general the events miss only when there are other executors or network failure is involved. http://gerrit.cloudera.org:8080/#/c/22482/3/be/src/util/runtime-profile-test.cc File be/src/util/runtime-profile-test.cc: http://gerrit.cloudera.org:8080/#/c/22482/3/be/src/util/runtime-profile-test.cc@1937 PS3, Line 1937: idjson: > nit: typo Done http://gerrit.cloudera.org:8080/#/c/22482/3/be/src/util/runtime-profile-test.cc@2001 PS3, Line 2001: > Fix to 5. Done http://gerrit.cloudera.org:8080/#/c/22482/3/be/src/util/runtime-profile-test.cc@2010 PS3, Line 2010: seqs[i]->MarkE > Remove this sleep. Done http://gerrit.cloudera.org:8080/#/c/22482/3/be/src/util/runtime-profile-test.cc@2019 PS3, Line 2019: if (ber_dis_event_probability(gen)){ > Consider changing this to something more deterministic. Such as: Providing a pattern for missing events generation would not be a good choice here. The added test coverage for expected missing event instances would correctly handle it. http://gerrit.cloudera.org:8080/#/c/22482/3/be/src/util/runtime-profile-test.cc@2022 PS3, Line 2022: missing_even > Remove this sleep. Done http://gerrit.cloudera.org:8080/#/c/22482/3/be/src/util/runtime-profile-test.cc@2024 PS3, Line 2024: } > Add comment like this: Done http://gerrit.cloudera.org:8080/#/c/22482/3/be/src/util/runtime-profile-test.cc@2079 PS3, Line 2079: // counters : < > Add parameter: bool expect_complete_event I have now added the test coverage for expected missing events. http://gerrit.cloudera.org:8080/#/c/22482/3/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/22482/3/be/src/util/runtime-profile.cc@a3078 PS3, Line 3078: : : > Is this a real bug? Only If the event timestamp for an instance is really unreported or missing, we are reporting such unreported instances and padding such values with zero. This is also helpful for textual representation. Similar to aggregated profile, where empty spans/buckets are assigned zeros. http://gerrit.cloudera.org:8080/#/c/22482/3/be/src/util/runtime-profile.cc@3015 PS3, Line 3015: > nit: while you're touching the file, could you also fix the spelling of 'ma Done. Thanks, it was one of those words I keep spelling wrong always. http://gerrit.cloudera.org:8080/#/c/22482/3/be/src/util/runtime-profile.cc@3032 PS3, Line 3032: In case > nit:typo Done -- To view, visit http://gerrit.cloudera.org:8080/22482 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If04744e215cac79f255b3d73c3e91e873c13749a Gerrit-Change-Number: 22482 Gerrit-PatchSet: 4 Gerrit-Owner: Surya Hebbar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Laszlo Gaal <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Comment-Date: Wed, 19 Feb 2025 09:02:43 +0000 Gerrit-HasComments: Yes
