Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )
Change subject: IMPALA-5149: Provide query profile in JSON format ...................................................................... Patch Set 9: (23 comments) http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-hs2-server.cc@988 PS9, Line 988: }else { nit: missing blank space after } http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-server.cc@101 PS9, Line 101: #include <rapidjson/document.h> : #include <rapidjson/error/en.h> : #include <rapidjson/stringbuffer.h> : #include <rapidjson/writer.h> Why not group these together with other rapidjson #include above ? There seems to be some duplicates. http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-server.cc@783 PS9, Line 783: DCHECK(format == TRuntimeProfileFormat::STRING); DCHECK_EQ(format, TRuntimeProfileFormat::STRING); http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-server.cc@818 PS9, Line 818: DCHECK(format == TRuntimeProfileFormat::STRING); DCHECK_EQ(format, TRuntimeProfileFormat::STRING); http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h File be/src/util/runtime-profile-counters.h: http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@126 PS9, Line 126: void ToJson(rapidjson::Document& document, rapidjson::Value* val) const { void ToJson(rapidjson::Document& document, rapidjson::Value* val) const override { http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@131 PS9, Line 131: val->AddMember("current_value", current_value(), document.GetAllocator()); Actually, do you need to output this in json ? It seems sufficient to just call Counter::ToJson() which should add the higher water mark stored in "value_" ? "current_value_" seems to be internal state used for supporting TryAdd() only. http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@165 PS9, Line 165: void ToJson(rapidjson::Document& document, rapidjson::Value* val) const { : Counter::ToJson(document, val); : rapidjson::Value::MemberIterator type_itr = val->FindMember("kind"); : DCHECK(type_itr != val->MemberEnd()); : type_itr->value.SetString("DerivedCounter"); : val->AddMember("counter_fn_val", counter_fn_(), document.GetAllocator()); : } If you take the suggestion in Counter::ToJson(), you probably don't need this function. http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@219 PS9, Line 219: rapidjson::Value::MemberIterator type_itr = val->FindMember("kind"); : DCHECK(type_itr != val->MemberEnd()); : type_itr->value.SetString("AveragedCounter"); See comments in Counter::ToJson() on why we may not need this. http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@222 PS9, Line 222: if (unit_ == TUnit::type::DOUBLE_VALUE){ : val->AddMember("current_double_sum", current_double_sum_, document.GetAllocator()); : } else { : val->AddMember("current_int_sum", current_int_sum_, document.GetAllocator()); : } Same question as above. Do you need to store the internal states (i.e. sum) in the json output object ? Isn't "average" the only state we need ? http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@290 PS9, Line 290: void ToJson(rapidjson::Document& document, rapidjson::Value* val) const { virtual override http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@292 PS9, Line 292: val->RemoveMember("kind"); Why is "kind" removed ? May be worth adding a comment here. http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@406 PS9, Line 406: void ToJson(rapidjson::Document& document, rapidjson::Value* value); Please add a comment about the output format. http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@452 PS9, Line 452: void ToJson(rapidjson::Document& document, rapidjson::Value* val); It'd be nice to add a comment about the output format. http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@583 PS9, Line 583: void ToJson(rapidjson::Document& document, rapidjson::Value* val) const { : Counter::ToJson(document, val); : rapidjson::Value::MemberIterator type_itr = val->FindMember("kind"); : type_itr->value.SetString("ConcurrentTimerCounter"); : val->AddMember("concurrent_stop_watch_total_time", : csw_.TotalRunningTime(), document.GetAllocator()); : } Similar to comments above, this may not be needed since Counter::ToJson() may have covered what's needed. http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.h@123 PS9, Line 123: name, value, human_readable, description is this list actually accurate ? What about "kind" and "unit" ? http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@709 PS9, Line 709: nit: unnecessary blank line http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@797 PS9, Line 797: if(!event_sequence_map_.empty()) { nit: formatting http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@821 PS9, Line 821: if(!summary_stats_map_.empty()) { formatting http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@855 PS9, Line 855: PrettyPrint() incorrect comment from copy-n-paste http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@1582 PS9, Line 1582: value_.Load() Use value() instead in case derived class overrides value(). http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@1583 PS9, Line 1583: _TUnit_VALUES_TO_NAMES.find(unit_) Does it make sense to DCHECK that this cannot be _TUnit_VALUES_TO_NAMES.end() ? http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@1585 PS9, Line 1585: "Counter" Instead of just adding "Counter" here, how about we implement a counter type function which returns a counter type function. In this way, the derived class doesn't need to override ToJson(). They only need to override the CounterType() function so that we will get the right counter type here: virtual std::string CounterType() const override { return "HighWaterMarkCounter"; } http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@1594 PS9, Line 1594: _TUnit_VALUES_TO_NAMES.find(unit_) Same question about DCHECK as above. -- To view, visit http://gerrit.cloudera.org:8080/13801 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8181ac818bf22207ca1deabd9220c397ae723ec1 Gerrit-Change-Number: 13801 Gerrit-PatchSet: 9 Gerrit-Owner: Jiawei Wang <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: David Rorke <[email protected]> Gerrit-Reviewer: Greg Rahn <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jiawei Wang <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Comment-Date: Thu, 25 Jul 2019 20:03:23 +0000 Gerrit-HasComments: Yes
