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.

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 
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.

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.

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.
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 
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.
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 

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 <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>
Gerrit-HasComments: Yes

Reply via email to