Matthew Jacobs has posted comments on this change. Change subject: Implemented a framework for the C++ hiveserver2 client. ......................................................................
Patch Set 3: (9 comments) There used to be more .cc files -- should they be here? http://gerrit.cloudera.org:8080/#/c/2645/3/src/hs2client/hs2session.h File src/hs2client/hs2session.h: Line 29: * the OpenSession RPC and uses it to create and return operations. This should document the lifecycle, e.g. Created by HS2Service::OpenSession(), resources must be released by calling Close(). http://gerrit.cloudera.org:8080/#/c/2645/3/src/hs2client/sample-usage.cc File src/hs2client/sample-usage.cc: Line 26: CHECK_OK why not just make a function? Line 62: 1 can you change the query to make it more clear that col 1 is a string, e.g. "select int_col, string_col from test"? Line 62: > most likely you'd want this to be the refernce, otherwise you're copying all the data Line 63: string const string& s: Line 63: nit: we don't use spaces here Line 69: &op This might be confusing. Even though GetTables should reset the ptr, we should make it clear. E.g. add a line above this that resets it explicitly. Line 72: row_set same for the row batch http://gerrit.cloudera.org:8080/#/c/2645/3/src/hs2client/status.h File src/hs2client/status.h: Line 42: ToString It's not clear if this is just the msg or also something else. I see in the impl it's just the msg, so how about calling this GetMessage()? -- 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: 3 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
