Thomas Tauber-Marshall has posted comments on this change. Change subject: Initial structure for the C++ hiveserver2 client. ......................................................................
Patch Set 9: (18 comments) http://gerrit.cloudera.org:8080/#/c/2645/9/.gitignore File .gitignore: Line 4: Testing > where does this come from? It is created by cmake and stores things like logging output from the most recent test run, so I suppose I should group it under "# CMake" below (I hadn't grouped it there because its not like that in the Impala .gitignore, but the Impala .gitignore isn't really grouped in any obvious way anyways). http://gerrit.cloudera.org:8080/#/c/2645/9/src/hs2client/columnar-row-set.h File src/hs2client/columnar-row-set.h: Line 47: // cout << col->data()[i] << "\n"; > Dereference needed here. However: API question, should col->data() return c Done http://gerrit.cloudera.org:8080/#/c/2645/9/src/hs2client/fetch-results.h File src/hs2client/fetch-results.h: Line 24: class RowSet {}; > just mention this is empty because it's not supported yet. Done Line 28: // oriented or column oriented protocol is being used, respectively. > How should code know what kind of FetchResults they have (should there be a It will depend on the protocol being used, which the user specifies when they create the service, so all FetchResults from a given service will have the same orientation. We also have Operation::IsColumnar defined, and the user can check which of the unique_ptrs in FetchResults contains NULL. I could certainly add FetchResults::IsColumnar, if desired. http://gerrit.cloudera.org:8080/#/c/2645/9/src/hs2client/hs2service.cc File src/hs2client/hs2service.cc: Line 92: impl_->socket.reset(new TSocket(host_, port_)); > I thought we were going to start passing through the timeout? Done http://gerrit.cloudera.org:8080/#/c/2645/9/src/hs2client/hs2service.h File src/hs2client/hs2service.h: Line 88: serssion > typo lol serssion Done http://gerrit.cloudera.org:8080/#/c/2645/9/src/hs2client/operation-test.cc File src/hs2client/operation-test.cc: Line 56: hasMoreRows > nit: variable names should be lower case with underscores per the coding st Done http://gerrit.cloudera.org:8080/#/c/2645/9/src/hs2client/operation.cc File src/hs2client/operation.cc: Line 70: hasMoreRows > nit: style, here and all instances in this file Done Line 86: *hasMoreRows = row_set_impl->resp.hasMoreRows; > Same. Sorry, I know it's inconsistent with what comes back with thrift :/ Done http://gerrit.cloudera.org:8080/#/c/2645/9/src/hs2client/operation.h File src/hs2client/operation.h: Line 65: Status GetState(OperationState* out); > can these Get*, Fetch, and Cancel methods be marked const? I don't see them Done Line 73: hasMoreRows > same as below Done Line 76: hasMoreRows > nit: style wrt naming variables/parameters Done Line 78: hasMoreRows > same Done Line 89: bool HasResultSet(); > ... const; Done Line 93: bool IsColumnar(); > ... const; Done http://gerrit.cloudera.org:8080/#/c/2645/9/src/hs2client/public-api-test.cc File src/hs2client/public-api-test.cc: Line 28: FAIL() << "Thrift headers should not be in the public API"; > I would suggest adding a hs2client/api.h for public API users (so that refa Done http://gerrit.cloudera.org:8080/#/c/2645/9/src/hs2client/thrift-internal.cc File src/hs2client/thrift-internal.cc: Line 29: Enumeration > nit: typically all caps, e.g. ENUM Done http://gerrit.cloudera.org:8080/#/c/2645/9/src/hs2client/util.h File src/hs2client/util.h: Line 30: Wait > can you move this to TestUtil as it's only used in a test. Done -- 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: 9 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
