Alexey Serbin has posted comments on this change.
Change subject: Doxygen for C++ client API
Patch Set 12:
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 her
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 consis
@todo is also easy to grep, right? Using @todo gives benefits of easy tracking
those even in the generated documentation; e.g., see
I prefer to keep @todo for the external-facing APIs like this to allow better
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
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 misle
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
> The HybridTime encoded timestamp should be obtained from another client's K
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
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
Line 469: /// For each set of hash partitions added to the table, the total
> s/table partitions/tablets
Line 472: /// with 4 and 5 buckets respectively, the total number of table
> s/table partitions/tablets
Line 505: /// call this method with an empty vector and set no split rows an
Line 517: /// If no range split rows are added, no range pre-splitting is
> Remove this sentence; it's not true anymore since it can be done via range
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
Line 567: /// Set the timeout for the operation.
> s/operation/table creation operation/
Line 622: /// This class is also a factory for write operation for on the table.
> s/for on/on
Line 675: /// This method creates new instance of comparison predicate which
> s/of/of a
Line 700: /// @return The KuduClient object associated with the table.
> add a note that the caller should not free the client.
Line 725: /// Create a new instance of alterer using
> s/of alterer/of a table alterer
Line 756: /// Alter an existing column.
> Add a note that the column may not be in the primary key.
Line 764: /// Drops an existing column from the table.
> Add a note that they column may not be in the primary key.
Line 771: /// Set timeout for the alteration operation.
> s/timeout/a timeout
Line 814: /// @brief This class represents an error which occurred in a given
> s/operation/write operation
Line 1065: /// Apply the write operation in asynchronous mode.
> s/asynchronous mode/asynchronously
Line 1089: /// @return Operation result status. Particularly, returns a bad
> Returns a non-OK status if there...
Line 1287: /// @copydoc KuduScanTokenBuilder::SetProjectedColumnIndexes()
> It's not an issue if the rendered docs come out the same, but this method s
Line 1368: /// @return Operation result status. Particularly, this method
> s/Particularly/In particular
Line 1493: /// @brief A partial scan limited to a single physical contiguous
> s/partital scan/scan descriptor
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>