Dan Burkert has posted comments on this change. Change subject: Doxygen for C++ client API ......................................................................
Patch Set 12: (23 comments) I'm still working on this so don't push quite yet. http://gerrit.cloudera.org:8080/#/c/3619/12/src/kudu/client/client.h File src/kudu/client/client.h: Line 92: /// Only the first invocation has effect; subsequent invocations are This needs to be 'any effect' or preferably 'an effect' Line 103: /// Only the first invocation has any effect; subsequent invocations are whatever you decide about 'an effect' above should probably be mirrored here. Line 145: /// @brief A "factory" for the KuduClient objects. s/for the/for Line 147: /// This class is used to create instances of KuduClient class s/of/of the Line 162: /// RPC addresses of masterts to add. s/masterts/masters Line 175: /// Set default timeout for administrative operations. s/default/the default Line 195: /// Create client object. s/client/a client Line 200: /// The newly created instance wrapped into a shared pointer. s/into/in Line 233: /// In order to actually access data on the cluster, callers must first s/actually access data on/write data to Line 239: /// @todo Cluster administration functions are likely to be in this class I'm not sure we actually want to change our TODO syntax; it's pretty consistent right now which allows for easy grepping. Perhaps this should remain, but not part of the formatted doxygen (is that possible by only using two slashes?). Line 247: /// @return Pointer to newly created instance; it's the caller's For consistency you should probably drop the contraction here and below. Line 251: /// Check whether CreateTable operation is in-progress. s/CreateTable/ a create table Line 269: /// Create a KuduTableAlterer object. It's probably best to be consistent about object vs. instance, for example NewTableCreator above uses instance. I don't have an opinion on what is more appropriate. Line 272: /// Name of the table to watch for. s/watch for/alter Line 280: /// Name of the table in question. remove 'in question' here and below, I don't think it adds any clarity. Line 310: /// Substring filter to use; empty sub-string filter matches any tables s/any tables/all tables. Line 326: /// If the table has not been opened before, then open the table I think this paragraph can be entirely removed, the first sentence is misleading, since we really don't do anything different in the case where you open an already 'opened' table. Line 359: /// @return @c true iff client was configured to talk to multiple s/was/is Line 373: /// Get highest HybridTime timestamp observed by the client. s/highest/the highest Line 388: /// To use this the user must obtain the HybridTime encoded timestamp from The HybridTime encoded timestamp should be obtained from another client's KuduClient::GetLatestObservedTimestamp() method. Line 442: /// Name of the target table -- it is copied. I would get rid of the note about the copy, it really shouldn't matter to callers. Line 443: /// @return Reference to the modified object. s/object/table creator Line 445: /// @remark Calling this method and setting name for the table-to-be s/name/the name -- To view, visit http://gerrit.cloudera.org:8080/3619 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02 Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Misty Stanley-Jones <[email protected]> Gerrit-HasComments: Yes
