Dan Burkert has posted comments on this change.
Change subject: Doxygen for C++ client API
Patch Set 12:
I'm still working on this so don't push quite yet.
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
whatever you decide about 'an effect' above should probably be mirrored here.
Line 145: /// @brief A "factory" for the KuduClient objects.
Line 147: /// This class is used to create instances of KuduClient class
Line 162: /// RPC addresses of masterts to add.
Line 175: /// Set default timeout for administrative operations.
Line 195: /// Create client object.
Line 200: /// The newly created instance wrapped into a shared pointer.
Line 233: /// In order to actually access data on the cluster, callers must
s/actually access data on/write data to
Line 239: /// @todo Cluster administration functions are likely to be in this
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
Line 272: /// Name of the table to watch for.
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
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
Line 373: /// Get highest HybridTime timestamp observed by the client.
Line 388: /// To use this the user must obtain the HybridTime encoded
The HybridTime encoded timestamp should be obtained from another client's
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
Line 443: /// @return Reference to the modified object.
Line 445: /// @remark Calling this method and setting name for the table-to-be
To view, visit http://gerrit.cloudera.org:8080/3619
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Owner: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Misty Stanley-Jones <mi...@apache.org>