Thomas Tauber-Marshall has posted comments on this change. Change subject: Implemented a framework for the C++ hiveserver2 client. ......................................................................
Patch Set 2: (13 comments) 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 Done 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: Done Line 30: using namespace std; > nit: line above 30 Done Line 46: > nit remove extra space Done Line 61: > nit: remove these extra lines? I don't think they increase readability here Done. I've gotten a bunch of "remove extra line" comments on my reviews here, so apparently I just have a different whitespace philosophy than the Impala team and I need to shift my whitespace thinking. Line 70: > nit: rm extra line Done http://gerrit.cloudera.org:8080/#/c/2645/2/src/hs2client/hs2service.h File src/hs2client/hs2service.h: Line 32: string > const std::string& Done 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 Done 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? When I wrote this class, it was still early in the implementation and I wasn't sure what we would need so I just copied the 'TStatus'es that hiveserver2 exposes. I think the right idea is to combine the two Success statuses, since we obviously don't need both, and also to rename it 'OK' since that seems to be the standard for these sorts of things. Line 41: StillExecuting > what does this mean? Taken from hiveserver2's TStatuses. This could presumably be returned by Operation::GetStatus, if the operation hasn't finished executing yet. Line 46: IsSuccessWithInfo > Let's see if we can get rid of this, it seems like a caller might have to c Like above, I'll remove 'IsSuccess' and 'IsSuccessWithInfo' in favor of 'ok', which seems to be the standard. Line 51: ok > overlap with the IsSucesses? Done Line 54: string > is this returning internal state (e.g. a member that isn't on this class ye Internal state. Made it const & -- 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-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
