Matthew Jacobs has posted comments on this change. Change subject: Implemented a framework for the C++ hiveserver2 client. ......................................................................
Patch Set 2: (14 comments) I think the interface is getting pretty close, but it's hard to sign off on just the interface when I think some things may change as you discover small things that come up in the implementation. I think it's ready to start moving forward with the implementation. Let's see if we can get a proof-of-concept plumbing an ExecStatement all the way through? http://gerrit.cloudera.org:8080/#/c/2645/2/src/hs2client/columnar_row_set.cc File src/hs2client/columnar_row_set.cc: can you use dashes between words in filenames instead of underscores to be consistent with impala c++ file naming? http://gerrit.cloudera.org:8080/#/c/2645/2/src/hs2client/hs2service.cc File src/hs2client/hs2service.cc: Line 21: #include "gen-cpp/TCLIService.h" we typically have includes ordered like this: #include <this_header> <blank line> #include <ext libraries> <blank line> #include "impala headers" <blank line> #include "impala generated headers (i.e. gen-cpp/*" Line 30: using namespace std; nit: line above 30 Line 46: nit remove extra space Line 54: HS2Service::~HS2Service() = default; Can you implement Close() & Reconnect()? Then this would be a good place to put DCHECKs that ensure the state is already cleaned up properly. Line 61: nit: remove these extra lines? I don't think they increase readability here (or push back if you think they do :) ) Line 70: nit: rm extra line http://gerrit.cloudera.org:8080/#/c/2645/2/src/hs2client/hs2service.h File src/hs2client/hs2service.h: Line 32: string const std::string& http://gerrit.cloudera.org:8080/#/c/2645/2/src/hs2client/hs2session.cc File src/hs2client/hs2session.cc: Line 27: default we should also figure out if we can check everything is cleaned up properly by the time this is destructed i.e. that Close() was called if it needs to be http://gerrit.cloudera.org:8080/#/c/2645/2/src/hs2client/status.h File src/hs2client/status.h: Line 40: SuccessWithInfo Do we really need this? Line 41: StillExecuting what does this mean? Line 46: IsSuccessWithInfo Let's see if we can get rid of this, it seems like a caller might have to check both IsSuccess and IsSuccessWithInfo to find out if the rpc worked Line 51: ok overlap with the IsSucesses? Line 54: string is this returning internal state (e.g. a member that isn't on this class yet) or a string that this creates on the stack? -- 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: 2 Gerrit-Project: hs2client Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
