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 8: (17 comments) Thanks so much for the valid feedback. Have adopted the changes you suggested. Also add some more unit tests. Let me know if the code need more changes. Thanks! http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/service/impala-http-handler.cc@270 PS6, Line 270: if (format != TRuntimeProfileFormat::JSON){ > nit: Add a quick comment why? Done http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-counters.h File be/src/util/runtime-profile-counters.h: http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-counters.h@126 PS6, Line 126: > nit: space const { . Done http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-counters.h@128 PS6, Line 128: rapidjson::Value::MemberIterator type_itr = val->FindMember("kind"); > Add DHCECK (type_itr != val->MemberEnd())? (multiple places below too) Done http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-counters.h@164 PS6, Line 164: > same (multiple places below) Done http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-counters.h@289 PS6, Line 289: > why this? I am moving this away because SummaryStatsCounter and TimeSeriesCounter are all separate in our output JSON schema. “counters”: [ { “counter_name”: xxx, “value”: xxx, “kind”: xxx, “unit”: xxx, “child_counters”: [{...}] // same structure as counter … //type special values },{...} ], “summary_stats_counters” : [ { “counter_name”: xxx, “value”: xxx, “unit”: xxx, “min”: xxx, “max”: xxx “avg”: xxx “num_of_samples”: xxx },{...} ] I feel like it is a redundant content if we add its "kind". What do you think? I can always add it back if you feel that's better. http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-counters.h@403 PS6, Line 403: > move to cc file? Done http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-counters.h@421 PS6, Line 421: EventList events_; > instantiate value = ...directly in L407? I am not sure what to do with this... I am using the same schema as metric collection https://github.com/apache/impala/blob/95a1da2d32ea7b28585ad574b22c2bb9dd921029/be/src/util/collection-metrics.cc#L28 http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-test.cc File be/src/util/runtime-profile-test.cc: http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-test.cc@1173 PS6, Line 1173: > check other units too and assert the unit data? Done http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-test.cc@1187 PS6, Line 1187: // Serialize to json > some tests for info_strings and event_sequences too? Done http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile-test.cc@1202 PS6, Line 1202: // Check counter value matches > check other TimeSeriesCounter implementations too? Done http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile.cc@705 PS6, Line 705: ); > remove rapidjson classifiers with using... Done http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile.cc@713 PS6, Line 713: CounterM > use d->GetAllocator() (avoid temp variable) Done http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile.cc@735 PS6, Line 735: } > formatting Done http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile.cc@1590 PS6, Line 1590: lock_guard<SpinLock> lock(lock_); > formatting Done http://gerrit.cloudera.org:8080/#/c/13801/6/be/src/util/runtime-profile.cc@1600 PS6, Line 1600: > formatting Done http://gerrit.cloudera.org:8080/#/c/13801/6/tests/hs2/test_hs2.py File tests/hs2/test_hs2.py: http://gerrit.cloudera.org:8080/#/c/13801/6/tests/hs2/test_hs2.py@659 PS6, Line 659: get_profile_req.format = 3 # json format > why not do this in the above test after L654? Yeah, I can put this inside the above test. http://gerrit.cloudera.org:8080/#/c/13801/6/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/13801/6/tests/webserver/test_web_pages.py@585 PS6, Line 585: # the query is in the file. > merge with test_download_text_profile? You can do something like 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: 8 Gerrit-Owner: Jiawei Wang <jiawei.w...@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: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Comment-Date: Mon, 15 Jul 2019 23:55:48 +0000 Gerrit-HasComments: Yes
