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 6: (9 comments) Actually, this was the reason to have missing_event_instances_expected and mutiple SetUp, TearDowns. Please let me know if I should increase the number of SetUp and TearDown to more than 2. I have added an assertion to maintain the relation to 'event_completeness'. It will skip the test, if there are no missing events or vice versa. Please let me know if I should increase the number of SetUp and TearDown to more than 2. 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@2079 PS3, Line 2079: // avg : > This is not addressed yet. Actually, this was the reason to have missing_event_instances_expected. This would have no elements in case of no missing events, we are equating the size of this with the parsed missing_event_instances. And also to maintain the relation to 'event_completeness', the assertion has now been added. It will skip the test, if there are no missing events or vice versa. Please let me know, what I should assert with respect to plan_node_profile. http://gerrit.cloudera.org:8080/#/c/22482/3/be/src/util/runtime-profile-test.cc@2102 PS3, Line 2102: // Validate the number of recorded events : EXPECT_EQ(events_json.Size(), sizeof(NOD > Assert that if expect_complete_event is False, test must enter this branch. Actually, this was the reason to have missing_event_instances_expected. This would have no elements in case of no missing events, we are equating the size of this with the parsed missing_event_instances. And also to maintain the relation to 'event_completeness', the assertion has now been added. It will skip the test, if there are no missing events or vice versa. http://gerrit.cloudera.org:8080/#/c/22482/5/be/src/util/runtime-profile-test.cc File be/src/util/runtime-profile-test.cc: http://gerrit.cloudera.org:8080/#/c/22482/5/be/src/util/runtime-profile-test.cc@1960 PS5, Line 1960: 1, > Start with 5 at minimum. This was the reason we had mutiple SetUp and TearDowns. Please let me know if I should increase the number of SetUp and TearDown to more than 2. http://gerrit.cloudera.org:8080/#/c/22482/5/be/src/util/runtime-profile-test.cc@1965 PS5, Line 1965: if (NUM_PROFILES <= 1) ++NUM_PROFILES; : std::uniform_int_d > Eliminate this case. Otherwise, it violates timestamp_aggregation=True. Done. I should have just incremented NUM_PROFILES. http://gerrit.cloudera.org:8080/#/c/22482/5/be/src/util/runtime-profile-test.cc@2020 PS5, Line 2020: if (ber_dis_event_probability(gen)){ > After exiting this loop, assert that missing_event_instances_expected is no Actually, this was the reason to have missing_event_instances_expected. This would have no elements in case of no missing events, we are equating the size of this with the parsed missing_event_instances. And also to maintain the relation to 'event_completeness', the assertion has now been added. It will skip the test, if there are no missing events or vice versa. http://gerrit.cloudera.org:8080/#/c/22482/5/be/src/util/runtime-profile-test.cc@2023 PS5, Line 2023: ssing_event_instances_expected > What guarantee that this will never hit all True? It is highly possible tha This actually runs for each instance and for each event. Also, we had mutiple SetUp and TearDowns to ensure this. events_count x num of profiles x 2 setups and teardowns Please let me know if I should increase the number of SetUp and TearDown to more than 2. http://gerrit.cloudera.org:8080/#/c/22482/5/be/src/util/runtime-profile-test.cc@2029 PS5, Line 2029: > What if NUM_PROFILES randomized to 1? This will not be a missing event case Please let me know if I should increase the number of SetUp and TearDown to more than 2. http://gerrit.cloudera.org:8080/#/c/22482/5/be/src/util/runtime-profile-test.cc@2120 PS5, Line 2120: EXPECT_EQ(missing_event_instances_expected.count( > If expect_complete_event is False, assert that both number is non zero. I have added the assertion within SetUp to hold the relation between event_completeness and skip current test in case that relation is not held. http://gerrit.cloudera.org:8080/#/c/22482/5/be/src/util/runtime-profile-test.cc@2273 PS5, Line 2273: 0.5f > What threshold guarantee that randomization will never hit all True in L202 This was the reason we had mutiple SetUp and TearDowns. Please let me know if I should increase the number of SetUp and TearDown to more than 2. -- 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: 6 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 18:36:12 +0000 Gerrit-HasComments: Yes
