Alexey Serbin has posted comments on this change.

Change subject: Doxygen for C++ client API
......................................................................


Patch Set 12:

(42 comments)

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'
Done


Line 103: /// Only the first invocation has any effect; subsequent invocations 
are
> whatever you decide about 'an effect' above should probably be mirrored her
Done


Line 145: /// @brief A "factory" for the KuduClient objects.
> s/for the/for
Done


Line 147: /// This class is used to create instances of KuduClient class
> s/of/of the
Done


Line 162:   ///   RPC addresses of masterts to add.
> s/masterts/masters
Done


Line 175:   /// Set default timeout for administrative operations.
> s/default/the default
Done


Line 195:   /// Create client object.
> s/client/a client
Done


Line 200:   ///   The newly created instance wrapped into a shared pointer.
> s/into/in
Done


Line 233: /// In order to actually access data on the cluster, callers must 
first
> s/actually access data on/write data to
Done


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 consis
@todo is also easy to grep, right?  Using @todo gives benefits of easy tracking 
those even in the generated documentation; e.g., see 
https://alexeyserbin.github.io/todo.html

I prefer to keep @todo for the external-facing APIs like this to allow better 
visibility.


Line 247:   /// @return Pointer to newly created instance; it's the caller's
> For consistency you should probably drop the contraction here and below.
Done


Line 251:   /// Check whether CreateTable operation is in-progress.
> s/CreateTable/ a create table
Done


Line 269:   /// Create a KuduTableAlterer object.
> It's probably best to be consistent about object vs. instance, for example 
Done


Line 272:   ///   Name of the table to watch for.
> s/watch for/alter
Done


Line 280:   ///   Name of the table in question.
> remove 'in question' here and below, I don't think it adds any clarity.
Done


Line 310:   ///   Substring filter to use; empty sub-string filter matches any 
tables
> s/any tables/all tables.
Done


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 misle
Done


Line 359:   /// @return @c true iff client was configured to talk to multiple
> s/was/is
Done


Line 373:   /// Get highest HybridTime timestamp observed by the client.
> s/highest/the highest
Done


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 K
Done


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 c
Done


Line 443:   /// @return Reference to the modified object.
> s/object/table creator
Done


Line 445:   /// @remark Calling this method and setting name for the table-to-be
> s/name/the name
Done


Line 469:   /// For each set of hash partitions added to the table, the total 
number of
> s/table partitions/tablets
Done


Line 472:   /// with 4 and 5 buckets respectively, the total number of table 
partitions
> s/table partitions/tablets
Done


Line 505:   /// call this method with an empty vector and set no split rows an 
no hash
> s/an/and
Done


Line 517:   /// If no range split rows are added, no range pre-splitting is 
performed.
> Remove this sentence; it's not true anymore since it can be done via range 
Done


Line 559:   /// Set the number of replicas for each tablet in the table.
> Referring to tablets in this is probably going to cause confusion.  I think
Done


Line 567:   /// Set the timeout for the operation.
> s/operation/table creation operation/
Done


Line 622: /// This class is also a factory for write operation for on the table.
> s/for on/on
Done


Line 675:   /// This method creates new instance of comparison predicate which
> s/of/of a
Done


Line 700:   /// @return The KuduClient object associated with the table.
> add a note that the caller should not free the client.
Done


Line 725: /// Create a new instance of alterer using 
KuduClient::NewTableAlterer().
> s/of alterer/of a table alterer
Done


Line 756:   /// Alter an existing column.
> Add a note that the column may not be in the primary key.
Done


Line 764:   /// Drops an existing column from the table.
> Add a note that they column may not be in the primary key.
Done


Line 771:   /// Set timeout for the alteration operation.
> s/timeout/a timeout
Done


Line 814: /// @brief This class represents an error which occurred in a given 
operation.
> s/operation/write operation
Done


Line 1065:   /// Apply the write operation in asynchronous mode.
> s/asynchronous mode/asynchronously
Done


Line 1089:   /// @return Operation result status. Particularly, returns a bad 
status
> Returns a non-OK status if there...
Done


Line 1287:   /// @copydoc KuduScanTokenBuilder::SetProjectedColumnIndexes()
> It's not an issue if the rendered docs come out the same, but this method s
Done


Line 1368:   /// @return Operation result status. Particularly, this method 
returns
> s/Particularly/In particular
Done


Line 1493: /// @brief A partial scan limited to a single physical contiguous 
location.
> s/partital scan/scan descriptor
Done


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