Jiawei Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13801 )
Change subject: IMPALA-5149: Provide query profile in JSON format ...................................................................... Patch Set 10: (23 comments) Thanks for the valuable feedback! Looking forward to the next step and the version control discussion. 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 } Done 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 "common/names.h" : : using boost::adopt_lock_t; : using boost::algorithm::is_an > Why not group these together with other rapidjson #include above ? There se Done http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-server.cc@783 PS9, Line 783: turn Status::OK(); > DCHECK_EQ(format, TRuntimeProfileFormat::STRING); Done http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/service/impala-server.cc@818 PS9, Line 818: > DCHECK_EQ(format, TRuntimeProfileFormat::STRING); Done 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: string CounterType() const override { > void ToJson(rapidjson::Document& document, rapidjson::Value* val) const ove Done http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@131 PS9, Line 131: > Actually, do you need to output this in json ? It seems sufficient to just Done http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@165 PS9, Line 165: private: : SampleFunction counter_fn_; : }; : : /// An AveragedCounter maintains a set of counters and its value is the : /// average of the values in that set. The average is updated through calls : /// > If you take the suggestion in Counter::ToJson(), you probably don't need th Done http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@219 PS9, Line 219: private: : /// Map from counters to their existing values. Modified via UpdateCounter(). : boost::unordered_map<Counter*, int64_t> counter > See comments in Counter::ToJson() on why we may not need this. Done http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@222 PS9, Line 222: : /// Current sums of values from counter_value_map_. Only one of these is used, : /// depending on the unit of the counter. current_double_sum_ is used for : /// DOUBLE_VALUE, current_int_sum_ otherwise. : dou > Same question as above. Do you need to store the internal states (i.e. sum) Done http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@290 PS9, Line 290: /// Summary statistics of values seen so far. > virtual override Done http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@292 PS9, Line 292: t64_t max_; > Why is "kind" removed ? May be worth adding a comment here. Done http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@406 PS9, Line 406: void SortEvents() { > Please add a comment about the output format. Done http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@452 PS9, Line 452: /// “unit” : xxx, > It'd be nice to add a comment about the output format. Done http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile-counters.h@583 PS9, Line 583: : void Add(int64_t delta) override { : DCHECK(false); : } : : string CounterType() const override { : > Similar to comments above, this may not be needed since Counter::ToJson() m Done 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: counter_name, value, kind, unit > is this list actually accurate ? What about "kind" and "unit" ? Done 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 Done http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@797 PS9, Line 797: Value event_sequences_rapid(kArrayType); > nit: formatting Done http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@821 PS9, Line 821: Value summary_stats_counters_rapid(kArrayType); > formatting Done http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@855 PS9, Line 855: Create copy of > incorrect comment from copy-n-paste Done http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@1582 PS9, Line 1582: > Use value() instead in case derived class overrides value(). Done http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@1583 PS9, Line 1583: Member("value", value(), document. > Does it make sense to DCHECK that this cannot be _TUnit_VALUES_TO_NAMES.end Done http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@1585 PS9, Line 1585: _TO_NAMES > Instead of just adding "Counter" here, how about we implement a counter typ Done http://gerrit.cloudera.org:8080/#/c/13801/9/be/src/util/runtime-profile.cc@1594 PS9, Line 1594: ck> lock(lock_); > Same question about DCHECK as above. Done -- 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: 10 Gerrit-Owner: Jiawei Wang <jiawei.w...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: David Rorke <dro...@cloudera.com> Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jiawei Wang <jiawei.w...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Comment-Date: Fri, 26 Jul 2019 20:55:00 +0000 Gerrit-HasComments: Yes
