Thomas Tauber-Marshall has posted comments on this change. Change subject: Initial structure for the C++ hiveserver2 client. ......................................................................
Patch Set 8: (39 comments) Updated to latest review. Still more work to do adding documentation. http://gerrit.cloudera.org:8080/#/c/2645/8//COMMIT_MSG Commit Message: Line 16: PIMPL-ing > super nit: but can you refer to this as "The PIMPL idiom" Done Line 16: header > public header Done Line 22: stored in > technically "accessible via" Done Line 23: via > hm maybe too many "via"s now, can use "by" Done Line 23: s > not plural? Done http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/columnar-row-set.cc File src/hs2client/columnar-row-set.cc: Line 110: assert(impl); > any reason string is the only one to have this assert? also were you going Done http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/columnar-row-set.h File src/hs2client/columnar-row-set.h: Line 27: > nit: no tab, here and below. I think you can remove this if you look at my Done Line 61: Column > A pattern I've used in the past is a base class and a subclass that is temp Done Line 61: class > Per our discussion, we can make the Column classes more concise by using te Done Line 61: Column > Please add a good comment here explaining how this is used, including the d Done Line 63: ~ > FYI destructors need to be virtual if there will ever be subclasses. In thi Done Line 72: protected: : // Hides Thrift objects from the header. : struct ColumnImpl; : : // For access to the c'tor. : friend class ColumnarRowSet; > No longer necessary Done http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/hs2service.cc File src/hs2client/hs2service.cc: Line 23: logging > this file doesn't exist in the review Done Line 67: Close(); > Unless there's a good reason to keep Reconnect I think it's just safer to t Done http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/hs2service.h File src/hs2client/hs2service.h: Line 67: The > Let's start with what this does before going into ownership (which is impor Done Line 72: out > Can you call this "service"? Done Line 81: Reconnect > Let's just remove this, it's not essential and not too clear to me when use Done Line 85: // The client calling OpenSession has ownership of the HS2Session that is created. > Let's start with what this does before going into ownership (which is impor Done Line 86: Executing RPCs with an Operation corresponding to a particular HS2Session after : // that HS2Session has been closed or deleted is undefined. > I think this sentence is better suited on HS2Session. Right here it'd be go Done Line 89: out > session Done http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/hs2session.cc File src/hs2client/hs2session.cc: Line 101: Status Open(hs2::TSessionHandle session_handle, const std::string& shema_pattern) { > Typo: schema_pattern Done Line 123: return op->Open(impl_->handle, shema_pattern); > Typos Done Line 134: req.__set_schemaName(shema_pattern); > shema -> schema Done Line 154: return op->Open(impl_->handle, shema_pattern, table_pattern); > shema -> schema Done Line 165: req.__set_schemaName(shema_pattern); > shema -> schema Done http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/hs2session.h File src/hs2client/hs2session.h: Line 44: out > operation, and below as well Done Line 49: // and table_pattern may contain wildcards. > Can you give an example of what a wild card looks like (e.g. "foo_*" matche Done http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/operation.cc File src/hs2client/operation.cc: Line 36: assert > DCHECK? Done Line 84: status > dcheck this is an OK status. I assume we can't just return Status::OK becau Yes, I made a point of always returning the converted thrift status rather than returning Status::OK so that we're not dropping info if it was actually a SuccessWithInfo status. http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/operation.h File src/hs2client/operation.h: Line 55: * have Close called on them before they can be deleted. > Please add: This class is not thread-safe. Done Line 71: Fetch will return success but 'out' will be empty. > fyi this means we have to be careful not to return empty row batches unless Good point. We should probably actually deal with this the right way, i.e. by returning the hasMoreRows variable from HS2 to the user. Since we're already returning a status, seems like the easiest thing is to have a bool out parameter. http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/sample-usage.cc File src/hs2client/sample-usage.cc: Line 83: string > const string& Done http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/test-util.h File src/hs2client/test-util.h: Line 109: session_->Close(); : service_->Close(); > You need to make sure these are always called. the EXPECT_OKs above will re EXPECT_* in gtest don't return early. https://github.com/google/googletest/blob/master/googletest/docs/Primer.md http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/thrift-internal.cc File src/hs2client/thrift-internal.cc: Line 31: unkown > typo, also append the value to the string for debugging purposes Done Line 50: default: > This is converting from our types to hs2 types, so we shouldn't have undefi Done Line 71: default > same dcheck Done Line 95: default: > Let's log if this happens, including the value of tstate Done Line 120: Unkown > typo Done http://gerrit.cloudera.org:8080/#/c/2645/8/src/hs2client/thrift-internal.h File src/hs2client/thrift-internal.h: Line 70: #define RETURN_NOT_OK(tstatus) \ : if (tstatus.statusCode != hs2::TStatusCode::SUCCESS_STATUS && \ : tstatus.statusCode != hs2::TStatusCode::SUCCESS_WITH_INFO_STATUS) { \ : return TStatusToStatus(tstatus); \ : } : : #define RETURN_MSG_NOT_OK(tstatus) \ : if (tstatus.statusCode != hs2::TStatusCode::SUCCESS_STATUS && \ : tstatus.statusCode != hs2::TStatusCode::SUCCESS_WITH_INFO_STATUS) { \ : return TStatusToStatus(tstatus).GetMessage(); \ : } > I don't see these being used and I don't think they're safely written macro 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: 8 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
