Matthew Jacobs has posted comments on this change. Change subject: Initial structure for the C++ hiveserver2 client. ......................................................................
Patch Set 10: (9 comments) Looks really good, just a few tiny things here then I'm ready to get this in. Thanks! http://gerrit.cloudera.org:8080/#/c/2645/10/CMakeLists.txt File CMakeLists.txt: Line 282: src/hs2client/sample-usage.cc : src/hs2client/status.cc : src/hs2client/thrift-internal.cc : src/hs2client/util.cc Fine for a follow-up task, but please factor out sample-usage and util into a separate module as they don't need to be in the actual client binary. I filed https://github.com/cloudera/hs2client/issues/6 http://gerrit.cloudera.org:8080/#/c/2645/10/src/hs2client/hs2service-test.cc File src/hs2client/hs2service-test.cc: Line 25: How are you tracking test coverage to add later? We should add timeouts to that list. http://gerrit.cloudera.org:8080/#/c/2645/10/src/hs2client/hs2service.h File src/hs2client/hs2service.h: Line 83: use_ssl comment that this is unsupported Line 83: conn_timeout Add a comment for this. What units is this in? What is 0? Are negative vals allowed (maybe no timeout)? Line 95: // Set the receive timeout. units? what are valid values? what is the default? Feel free to combine the comments for here and Send, as long as the text is clear. http://gerrit.cloudera.org:8080/#/c/2645/10/src/hs2client/operation.h File src/hs2client/operation.h: Line 73: , if : // given remove, let's have it always set. Line 74: If all of the results have been fetched already, Fetch will return success : // but 'results' will be empty Remove this too, the caller should check has_more_rows to determine eos. Line 76: = NULL remove default parameter so that it's always set Line 78: = NULL same -- 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: 10 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
