Matthew Jacobs has posted comments on this change. Change subject: Initial structure for the C++ hiveserver2 client. ......................................................................
Patch Set 8: (27 comments) http://gerrit.cloudera.org:8080/#/c/2645/8//COMMIT_MSG Commit Message: Line 16: PIMPL-ing super nit: but can you refer to this as "The PIMPL idiom" Line 16: header public header Line 22: stored in technically "accessible via" Line 23: s not plural? Line 23: via hm maybe too many "via"s now, can use "by" http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/hs2service.cc File src/hs2client/hs2service.cc: Line 67: Close(); Unless there's a good reason to keep Reconnect I think it's just safer to take it out. If we keep it here and Close() fails, the return is never handled or logged. If the client implements this logic themselves they can decide if they want to log it or handle it somehow. http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/hs2service.h File src/hs2client/hs2service.h: Line 67: The Let's start with what this does before going into ownership (which is important too, of course). Please prefix this with a sentence like "Creates a new connection to a HS2 service." Line 72: out Can you call this "service"? Line 81: Reconnect Let's just remove this, it's not essential and not too clear to me when users should use this. Or if it's very clear when/why to use it, let's document it. Line 85: // The client calling OpenSession has ownership of the HS2Session that is created. Let's start with what this does before going into ownership (which is important too, of course). Please prefix this with a sentence like "Opens a new HS2 session using this service." Line 86: Executing RPCs with an Operation corresponding to a particular HS2Session after : // that HS2Session has been closed or deleted is undefined. I think this sentence is better suited on HS2Session. Right here it'd be good to add something like "Operations on the HS2Session are undefined once this HS2Session is closed." Line 89: out session http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/hs2session.h File src/hs2client/hs2session.h: Line 44: out operation, and below as well http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/operation.cc File src/hs2client/operation.cc: Line 36: assert DCHECK? Line 84: status dcheck this is an OK status. I assume we can't just return Status::OK because this could have a message w/ it? http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/operation.h File src/hs2client/operation.h: Line 55: * have Close called on them before they can be deleted. Please add: This class is not thread-safe. Also add that to the other main interface class header comments. Line 61: // May be called after successfully creating the operation and before calling Close. The comment here and for all the public API functions should include a brief description of what these do, parameters, return values. However, I'd also like to get the initial code in soon so I'm OK with a follow-up review to go through and comment more thoroughly. For example, here's how I'd document this function: // Fetches the current state of this operation. If successful, sets the // operation state in 'out' and returns an OK status, otherwise an // error status is returned. May be called after successfully creating // the operation and before calling Close. Line 71: Fetch will return success but 'out' will be empty. fyi this means we have to be careful not to return empty row batches unless we actually get an eos from the server side. While it's probably not likely to happen unless something is wrong, I don't think impala guarantees it gives non-empty result batches and we need to guard against it. We could handle it by continuing to fetch until something comes, or at least we should probably have a DCHECK to verify. http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/sample-usage.cc File src/hs2client/sample-usage.cc: Line 83: string const string& http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/status.h File src/hs2client/status.h: Line 38: Status This should be commented as well. Let's do it in a follow-up review. http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/test-util.h File src/hs2client/test-util.h: Line 109: session_->Close(); : service_->Close(); You need to make sure these are always called. the EXPECT_OKs above will return early. http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/thrift-internal.cc File src/hs2client/thrift-internal.cc: Line 31: unkown typo, also append the value to the string for debugging purposes Line 50: default: This is converting from our types to hs2 types, so we shouldn't have undefined cases. Add a DCHECK we don't get here before returning Line 71: default same dcheck Line 95: default: Let's log if this happens, including the value of tstate Line 120: Unkown typo http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/thrift-internal.h File src/hs2client/thrift-internal.h: Line 70: #define RETURN_NOT_OK(tstatus) \ : if (tstatus.statusCode != hs2::TStatusCode::SUCCESS_STATUS && \ : tstatus.statusCode != hs2::TStatusCode::SUCCESS_WITH_INFO_STATUS) { \ : return TStatusToStatus(tstatus); \ : } : : #define RETURN_MSG_NOT_OK(tstatus) \ : if (tstatus.statusCode != hs2::TStatusCode::SUCCESS_STATUS && \ : tstatus.statusCode != hs2::TStatusCode::SUCCESS_WITH_INFO_STATUS) { \ : return TStatusToStatus(tstatus).GetMessage(); \ : } I don't see these being used and I don't think they're safely written macros. Can you remove them? If you need macros to do this we need to wrap them in a do/while loop or so. -- To view, visit http://gerrit.cloudera.org:8080/2645 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7be82d5897ad6093116915b17924674c4ae508a0 Gerrit-PatchSet: 8 Gerrit-Project: hs2client Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Wes McKinney <[email protected]> Gerrit-HasComments: Yes
