Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21683 )
Change subject: IMPALA-13304: Include aggregate instance-level metrics within experimental profile(V2) ...................................................................... Patch Set 11: (9 comments) http://gerrit.cloudera.org:8080/#/c/21683/11/be/src/util/runtime-profile-test.cc File be/src/util/runtime-profile-test.cc: http://gerrit.cloudera.org:8080/#/c/21683/11/be/src/util/runtime-profile-test.cc@1877 PS11, Line 1877: // Test aggregation of event sequences, during generation of JSON profiles nit: Test aggregation of well-formed event sequences, during generation of JSON profiles. http://gerrit.cloudera.org:8080/#/c/21683/11/be/src/util/runtime-profile-test.cc@1919 PS11, Line 1919: TEST(AggregatedEventSequenceToJson, MissingEventsTest) { Add comment on what this test is about. http://gerrit.cloudera.org:8080/#/c/21683/11/be/src/util/runtime-profile-test.cc@1977 PS11, Line 1977: ToJsonHelper Better to call the public ToJson() function? http://gerrit.cloudera.org:8080/#/c/21683/11/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: http://gerrit.cloudera.org:8080/#/c/21683/11/be/src/util/runtime-profile.h@848 PS11, Line 848: // struct AggEventSequence; nit: Can be removed? http://gerrit.cloudera.org:8080/#/c/21683/11/be/src/util/runtime-profile.h@880 PS11, Line 880: // AggEventSequence* GetAggregatedEventSequence(const std::string& name) const; nit: Can be removed? http://gerrit.cloudera.org:8080/#/c/21683/11/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/21683/11/be/src/util/runtime-profile.cc@2736 PS11, Line 2736: /*AggregatedRuntimeProfile::AggEventSequence* AggregatedRuntimeProfile:: : GetAggregatedEventSequence(const std::string& name) const { : lock_guard<SpinLock> l(event_sequence_lock_); : AggEventSequenceMap::const_iterator it = event_sequence_map_.find(name); : if (it == event_sequence_map_.end()) return NULL; : return &(AggEventSequence)it->second; : }*/ Lets remove this. Can add this back when we really need it in more relevant patch. http://gerrit.cloudera.org:8080/#/c/21683/11/be/src/util/runtime-profile.cc@3009 PS11, Line 3009: "info_strings" Turn this into a constant and use it everywhere? $ git grep -n '"info_strings"' be/ be/src/util/runtime-profile-test.cc:1737: EXPECT_EQ(1, content["info_strings"].Size()); be/src/util/runtime-profile-test.cc:1738: EXPECT_EQ("Key", content["info_strings"][0]["key"]); be/src/util/runtime-profile-test.cc:1739: EXPECT_EQ("Value", content["info_strings"][0]["value"]); be/src/util/runtime-profile-test.cc:1785: EXPECT_TRUE(!content.HasMember("info_strings")); be/src/util/runtime-profile.cc:1359: parent->AddMember("info_strings", info_strings_json, allocator); http://gerrit.cloudera.org:8080/#/c/21683/11/be/src/util/runtime-profile.cc@3014 PS11, Line 3014: AGGREGATED_INFO_STRINGS_REQUIRED This array only has 1 element. I suggest turning the loop instruction into a function and call that function directly. Say, CollectInfoString("Table Name", kObjectType, allocator, info_strings_json); Then drop AGGREGATED_INFO_STRINGS_REQUIRED. http://gerrit.cloudera.org:8080/#/c/21683/11/be/src/util/runtime-profile.cc@3034 PS11, Line 3034: parent->AddMember("info_strings", info_strings_json, allocator); This is not needed if info_strings_json == (*parent)["info_strings"] (assignment in L3010)? -- To view, visit http://gerrit.cloudera.org:8080/21683 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I49e18a7a7e1288e3e674e15b6fc86aad60a08214 Gerrit-Change-Number: 21683 Gerrit-PatchSet: 11 Gerrit-Owner: Surya Hebbar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Comment-Date: Fri, 06 Dec 2024 17:08:35 +0000 Gerrit-HasComments: Yes
