[kudu-CR] KUDU-1444. Get resource metrics of a scan.
Hello Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3013 to look at the new patch set (#9). Change subject: KUDU-1444. Get resource metrics of a scan. .. KUDU-1444. Get resource metrics of a scan. This patch supports to get the resource metrics of a scan in client side. The resource metrics will be sent back to client in every scan RPC response. This is useful for impala to show these stats in a query profile. For now, the resource metrics only contains cfile_cache_miss_bytes and cfile_cache_hit_bytes. We may add more in the future as needed. Change-Id: Iedaf570a7601651c93275ae0a8565f1e33da842d --- M src/kudu/cfile/cfile_reader.cc M src/kudu/client/CMakeLists.txt M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h A src/kudu/client/resource_metrics-internal.h A src/kudu/client/resource_metrics.cc A src/kudu/client/resource_metrics.h M src/kudu/client/scanner-internal.cc M src/kudu/client/scanner-internal.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tserver.proto M src/kudu/util/trace_metrics.h 13 files changed, 288 insertions(+), 12 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/3013/9 -- To view, visit http://gerrit.cloudera.org:8080/3013 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iedaf570a7601651c93275ae0a8565f1e33da842d Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: zhen.zhang Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: zhen.zhang
[kudu-CR] KUDU-1444. Get resource metrics of a scan.
Jean-Daniel Cryans has posted comments on this change. Change subject: KUDU-1444. Get resource metrics of a scan. .. Patch Set 8: > What should I do to help rebase? Try rebasing locally with your patch, see if you can solve the conflicts. -- To view, visit http://gerrit.cloudera.org:8080/3013 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iedaf570a7601651c93275ae0a8565f1e33da842d Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: zhen.zhang Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: zhen.zhang Gerrit-HasComments: No
[kudu-CR] KUDU-1444. Get resource metrics of a scan.
zhen.zhang has posted comments on this change. Change subject: KUDU-1444. Get resource metrics of a scan. .. Patch Set 8: What should I do to help rebase? -- To view, visit http://gerrit.cloudera.org:8080/3013 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iedaf570a7601651c93275ae0a8565f1e33da842d Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: zhen.zhang Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: zhen.zhang Gerrit-HasComments: No
[kudu-CR] Add unique id generation to the client
David Ribeiro Alves has submitted this change and it was merged. Change subject: Add unique id generation to the client .. Add unique id generation to the client This adds unique id generation to the client, which takes the form of an uuid. The uuid is generated when the client is built, by a static OidGenerator. This also adds a test that generates 1000 clients and makes sure that the generated ids are unique. Since this test will now run all the time we should get notified if stuff becomes flaky dues to repeated ids. Change-Id: Ie752ab8337cc76c3be1536958c24a8b6c5794650 Reviewed-on: http://gerrit.cloudera.org:8080/3077 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo Reviewed-by: Todd Lipcon --- M src/kudu/client/client-internal.h M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/integration-tests/client-stress-test.cc 4 files changed, 26 insertions(+), 0 deletions(-) Approvals: Adar Dembo: Looks good to me, but someone else must approve Todd Lipcon: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/3077 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ie752ab8337cc76c3be1536958c24a8b6c5794650 Gerrit-PatchSet: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add a RpcContext::RespondFailure() method
Todd Lipcon has posted comments on this change. Change subject: Add a RpcContext::RespondFailure() method .. Patch Set 6: (2 comments) wonder whether it would be a clearer API to instead have a flag like context->DoNotCacheResponse(); which must be called in these cases? It gets a bit ugly that we have so many different "RespondFailure" code paths now http://gerrit.cloudera.org:8080/#/c/3191/6/src/kudu/rpc/rpc_context.h File src/kudu/rpc/rpc_context.h: Line 84: // Mark this call as failed but don't actually change the response. this isn't clear that it also responds -- "Mark as failed" doesn't have the active verb "responds" Line 87: // to mark the call as failed. would be good to explain what actual effect this has -- To view, visit http://gerrit.cloudera.org:8080/3191 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9f1387ba0f837046a8056e77f73a3982f06c73a2 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] WIP: Integrate the request tracker with the client
Todd Lipcon has posted comments on this change. Change subject: WIP: Integrate the request tracker with the client .. Patch Set 7: (2 comments) would be nice to have some kind of test for this at the rpc layer (against a test server or somesuch) http://gerrit.cloudera.org:8080/#/c/3080/7/src/kudu/rpc/retriable_rpc.h File src/kudu/rpc/retriable_rpc.h: Line 188: Status s = request_tracker_->FirstIncomplete(&first_incomplete); why not have FirstIncomplete just return NO_SEQ_NO instead of a status? Line 210: request_tracker_->RpcCompleted(sequence_number_); in the middle of a comment? can you ad some kind of assertion on the destructor that this has gotten called? may require an extra boolean or state enum as an instance member, but this seems like the kind of thing we could very easily accidentally leak -- To view, visit http://gerrit.cloudera.org:8080/3080 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I94207c294452fcbdb3a7901fdb702674d47553ee Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Add unique id generation to the client
Todd Lipcon has posted comments on this change. Change subject: Add unique id generation to the client .. Patch Set 14: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3077 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie752ab8337cc76c3be1536958c24a8b6c5794650 Gerrit-PatchSet: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1471 NPE in AsyncKuduClient from Spark
Todd Lipcon has posted comments on this change. Change subject: KUDU-1471 NPE in AsyncKuduClient from Spark .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/3243/3/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java: Line 1403: // The client2tables map is assumed to be populated upon the return of the TabletClient If I understand the bug, it's not about the _return_ of the TabletClient object, but rather the _publishing_ of the TabletClient object within the ip2client map? i.e there's an assumption that if it's present in ip2client, then it's also in client2tablets? This also makes me wonder whether there's a race in addTabletClient() such that a tablet can end up listed in the ArrayList twice. Any way we can get a test which triggers this race semi-reliably? -- To view, visit http://gerrit.cloudera.org:8080/3243 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iecb0f0f50f62db3a3d5fb10f80da70fedeb2629d Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Ted Malaska Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Allow to set RequestId in the RPC RequestHeader
Todd Lipcon has posted comments on this change. Change subject: Allow to set RequestId in the RPC RequestHeader .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/3179/3/src/kudu/rpc/rpc_controller.h File src/kudu/rpc/rpc_controller.h: Line 110: void SetRequestIdPB(std::unique_ptr request_id); per some comments on an earlier (committed) patch in this series, I think we're still being too sloppy on the word "Request" and "RPC" here, which makes the interface confusing. For example, the proto now has a call_id and a request_id, and it's not obvious that the request_id refers to some higher-level retriable operation whereas the call_id is a connection-scoped sequence number. Any ideas on some new terminology we can introduce for this higher-level concept? -- To view, visit http://gerrit.cloudera.org:8080/3179 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I92566250683ee69fad57a9ae694842d4a0b17ab4 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Specify guaranteed semantics of GetTableLocations RPC
Todd Lipcon has posted comments on this change.
Change subject: Specify guaranteed semantics of GetTableLocations RPC
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3240/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 3093: StatusToPB(Status::ServiceUnavailable("Tablet not running"),
: resp->mutable_error()->mutable_status());
> BuildLocationsForTablet() can also return Status::NotFound for deleted tabl
no 'break' here?
--
To view, visit http://gerrit.cloudera.org:8080/3240
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc2b5b647e33c0ee8fc052192870f814eff8c178
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1444. Get resource metrics of a scan.
Todd Lipcon has posted comments on this change. Change subject: KUDU-1444. Get resource metrics of a scan. .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3013 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iedaf570a7601651c93275ae0a8565f1e33da842d Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: zhen.zhang Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: zhen.zhang Gerrit-HasComments: No
[kudu-CR] KUDU-1444. Get resource metrics of a scan.
Todd Lipcon has posted comments on this change. Change subject: KUDU-1444. Get resource metrics of a scan. .. Patch Set 8: Looks like this needs a manual rebase -- To view, visit http://gerrit.cloudera.org:8080/3013 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iedaf570a7601651c93275ae0a8565f1e33da842d Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: zhen.zhang Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: zhen.zhang Gerrit-HasComments: No
[kudu-CR] log: Mark allocation finished even if allocation had an error
Todd Lipcon has posted comments on this change. Change subject: log: Mark allocation finished even if allocation had an error .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/3234/2/src/kudu/consensus/log.cc File src/kudu/consensus/log.cc: Line 188: // TODO If a single transaction fails to append, should we hm, this TODO is now scaring me. If we fail to append one operation (eg because the disk is full, or because of a temporary IO error) we could keep going and have a "gap" in txids? Have you checked all call sites to make sure we FATAL on a failure to append to the log? (I see that this was already the case in RELEASE builds, so you aren't making anything worse than it was, but it is uncovering a scariness here) -- To view, visit http://gerrit.cloudera.org:8080/3234 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If22bf946a42d0ec32c35164acd9e6e6cef18dcc3 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
