Adar Dembo has posted comments on this change.

Change subject: [C++ client] doxygenized all C++ client API
......................................................................


Patch Set 2:

(86 comments)

Looks like client_samples-test failed, btw.

http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/client/callbacks.h
File src/kudu/client/callbacks.h:

Line 60:   ///   The absolute time when the loggin event is generated.
"when the log event was"


Line 62:   ///   The message to log.  It's not terminated with an endline.
Nit: you have two spaces here after the period.


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/client/client.h
File src/kudu/client/client.h:

Line 309:   ///   Result parameter @c tables is appended only on success.
Maybe "The placeholder for the result. Appended only on success"?


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/client/resource_metrics.h
File src/kudu/client/resource_metrics.h:

Line 45:   ///   The increment/decrement to aggregate.
Maybe "The amount to increment/decrement", with some mention of how amount can 
be negative for decrement?


Line 48:   /// Get current count for the specified metrics.
"specified metric" since it's just one metric.


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/client/scan_batch.h
File src/kudu/client/scan_batch.h:

Line 44: /// As a result of a scan operation, KuduScanner object returns batch 
of
Maybe something like "Every call to KuduScanner::NextBatch() returns a batch of 
zero or more rows."


Line 105:   ///   The returned object is only valid for as long as this 
KuduScanBatch.
Nit: "for as long as this KuduScanBatch is valid"


Line 127: class KUDU_EXPORT KuduScanBatch::RowPtr {
A @brief for this?


Line 136:   bool IsNull(const Slice& col_name) const;
No summarizing sentences for the two IsNull() methods?


Line 155:   ///@{
Clever.


Line 172:   /// since using indices avoid a hashmap lookup, so index-based 
getters
Nit: avoids. Below too.


PS2, Line 179:   ///   Note that the method does not copy the value. Callers 
should copy
             :   ///   the resulting Slice if necessary.
Doesn't seem right for these getters.


Line 201:   /// @name Getters for string/binary column by column name
Nit: missing terminating period. Below too.


PS2, Line 255: -- useful
             :   ///   for outputting information into the log.
Nit: just drop the addendum; "string representation..." is sufficient.


Line 281: class KUDU_EXPORT KuduScanBatch::const_iterator
No @brief?


Line 311:   ///   of the same batch row.
Nit: "of the same batch"


Line 312:   bool operator==(const const_iterator& other) const {
No summarizing sentence for this or operator!= ?


PS2, Line 318: different row
             :   ///   of or associated with a different batch row.
Maybe "different row in the same batch, or to a row belonging to a different 
batch altogether?"


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/client/schema.h
File src/kudu/client/schema.h:

Line 151:   ///   Column attributes.
Nit: Column storage attributes.


Line 158:   /// Construct KuduColumnSchema object as a copy of other object.
Nit: "as a copy from another object".

Alternatively, do we even need to document the copy constructor and assignment 
operator? We don't document the destructor, and these two methods behave as any 
copy constructor and assignment operator behave, right?


Line 172:   /// Make this object an identical copy of other one.
Nit: "of the other one."


Line 178:   /// Check whether the object is identical to the other.
Nit: "identical to the other one."


PS2, Line 229: When inserting a new row with no value for this column,
             :   /// the default value will also be used.
Nit: "The default value will also be used when inserting a new row with no 
value for this column." Maybe merge with the previous paragraph to better 
connect.


Line 233:   ///   The value to use as the default.  The KuduColumnSpec takes 
ownership
Got two spaces after the period here.


Line 239:   /// Set the preferred compression for the column.
Nit: "compression type"


Line 242:   ///   The compression to use.
Nit: "compression type"


Line 279:   /// Set this column to be the primary key of the table.
Nit: "Set the column" to be consistent.


Line 304:   /// Set the type of the column.
Nit: "data type". Below too.


Line 317:   /// Remove the default value for this column.
Nit: "for the column"


Line 380:   KuduColumnSpec* AddColumn(const std::string& name);
Nit: summarizing sentence?


Line 394:   ///   The placeholder for the result schema.  Upon successful 
completion,
Nit: two spaces.


Line 408: /// @brief A representation of schema.
Nit: perhaps "of a table's schema"?


Line 413:   /// Create a KuduSchema object as a copy of the other one.
As I wrote in an earlier comment, do we have to document constructors/operators 
whose behavior is basically the default?


Line 446:   bool Equals(const KuduSchema& other) const;
Missing summarizing sentence for this and for Column() (which is arguably a 
non-standard getter).


Line 469:   ///   destroyed before the KuduSchema object to omit dangling 
pointers.
Nit: "to avoid dangling pointers".


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/client/stubs.h
File src/kudu/client/stubs.h:

Line 160: /// @brief A helper for nil log sink.
Should explain that the "nil log sink" means nothing gets logged.


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/client/write_op.h
File src/kudu/client/write_op.h:

Line 66:   /// @note To work with a row, use KuduPartialRow API for field 
getters, etc.
Nit: "use the KuduPartialRow..." Below too.


PS2, Line 74:   Useful for outputting
            :   ///   information into log, etc.
Nit: no need for this.


Line 84:   explicit KuduWriteOperation(const sp::shared_ptr<KuduTable>& table);
The protected members aren't accessible to users, so why bother documenting 
them? Below too.


Line 173: ///   in the schema to be set in the embedded PartialRow object.
Nit: KuduPartialRow. Same with KuduDelete.


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:

PS2, Line 50: This type
This is a strange way to refer to the object. Maybe "This object"? Or "A 
KuduPartialRow"?


Line 62:   /// Create a copy of KuduPartialRow instance.
Do we really need to document these?


PS2, Line 75:   //------------------------------------------------------------
            :   // Setters
            :   //------------------------------------------------------------
How does doxygen treat these separators?


PS2, Line 106: The same a
Nit: "These setters are the same as the corresponding ...".


PS2, Line 151: The same as
Nit: "These setters are the same as the corresponding ...".


Line 250:   bool IsColumnSet(const Slice& col_name) const;
Do the IsColumnSet() and IsNull() methods deserve summarizing sentences?


PS2, Line 259: the given column has @c NULL value.
Nit: "the given column's value is @c NULL". Below too.


PS2, Line 295: The same as
Nit: "These getters are the same as the corresponding ...".


Line 346:   /// since using indices avoid a hashmap lookup, so index-based 
getters
"since they use indices to avoid hashmap lookups".


Line 379:   /// @return Operation result status.  In particular, returns 
InvalidArgument
Nit: two spaces here.


PS2, Line 395: if all of the key columns have been specified
             :   ///   for this mutation.
Nit" If all key column values have been set."


Line 399:   /// @return @c true if all columns have been specified.
Nit: "if all column values have been set."


PS2, Line 402: useful
             :   ///   for outputting information into the log.
Nit: drop.


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/util/monotime.h
File src/kudu/util/monotime.h:

Line 39: /// @brief A representation of time interval.
Nit: "of a time interval"


PS2, Line 110:   /// @note If MonoDelta::IsPositive() returns true, the struct 
timeval is
             :   ///   guaranteed to hold a positive number as well (at least 1 
microsecond).
Is this still relevant? I don't see MonoDelta::IsPositive(); we must have 
removed it.


Line 117:   /// Represent this time interval as a timespec structure.
With nanonsecond accuracy.


Line 152:   /// accurate to within a millisecond or two.  The speed difference 
will
Nit: two spaces.


Line 159:   /// @name Convertion constants for ubiquitous time units.
"Conversion"


Line 187:   static const MonoTime& Earliest(const MonoTime& a, const MonoTime& 
b);
Nit: summarizing sentence?


Line 200:   ///   The object to specify the left boundary of the time interval,
"The object that corresponds to the left boundary of the time interval"


Line 228:   ///   is the same as represented by the other.
Nit: "is the same as the one..."


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/util/slice.h
File src/kudu/util/slice.h:

PS2, Line 40: /// Slices can be built around faststrings and StringPieces using 
constructors
            : /// with implicit casts. Both StringPieces and faststrings depend 
on a great
            : /// deal of gutil code, so these constructors are conditionalized 
on
            : /// KUDU_HEADERS_USE_RICH_SLICE. Likewise, 
KUDU_HEADERS_USE_RICH_SLICE controls
            : /// whether to use gutil-based memeq/memcmp substitutes; if it is 
unset, Slice
            : /// will fall back to standard memcmp.
Clients never define KUDU_HEADERS_USE_RICH_SLICE, so I wonder whether we should 
omit this from the "public" doxygen docs in some way.


Line 52:   /// Create a slice that refers to a @c uint8_t array.
Nit: maybe "to a @c uint8_t byte array"? Below too. Then "Number of elements" 
can be "Number of bytes", which is more informative.


Line 100:   /// @note Further appends to the faststring may invalidate this 
slice.
Does this apply to StringPieces too?


Line 126:   /// @return the ith byte in the referenced data.
Nit: nth byte?


Line 132:   /// Change this slice to refer to an empty array
Nit: terminating period.


Line 143:   ///   Number of bytes to drop/skip in the beginning.
Nit: "Number of bytes that should be dropped".

May also want to add that the data isn't actually modified, just the 
base/bounds of the slice. For truncate() too.


Line 155:   ///   The number of bytes to have after the truncation.
Nit: "New size of the slice."


Line 161:   /// Check the slice has the expected size.
Nit: "Check that the slice..."


PS2, Line 164: Status::OK iff size() == @c expected_size;
             :   ///   Status::Corruption() otherwise.
How about just "Corruption iff size() != @c expected_size"?


Line 168:   /// @return A string that contains the copy of the referenced data.
Nit: "...contains a copy of the..."


Line 175:   ///   @c 0 means there is no limit.
Nit: "@c 0 means no limit"


Line 179:   /// Do three-way comparison of the slice's data.
Nit: "Do a three-way comparison..."


Line 256: /// @return @c true iff two slices contain byte-to-byte identical 
data.
Nit: "byte-for-byte"


PS2, Line 275: /// This method is useful in debug and troubleshooting scenarios 
when it's
             : /// necessary to output slice information in printable form into 
log file, etc.
Eh, I think this is self-explanatory.


http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/util/status.h
File src/kudu/util/status.h:

Line 30: #define KUDU_RETURN_NOT_OK(s) do { \
These macros are user-facing; should we doxygenate their comments too?


Line 103: /// @brief The class to represent outcome of performed operation.
Nit: "Representation of an operation's outcome"


Line 111:   /// Copy the specified status.
Do we need these?


Line 130:   /// Assign the specified status using the move semantics (C++11).
Nit: "using move semantics"


Line 241:   /// @return @c true iff the status indicates an InvalidArgument 
error
Nit: terminate with period. Below too.


Line 253:   /// @return @c true iff the status indicates a IllegalState.
Nit: an IllegalState.


Line 256:   /// @return @c true iff the status indicates a NotAuthorized.
Nit: NotAuthorized error.


Line 262:   /// @return @c true iff the status indicates RemoteError.
Nit: a RemoteError.


Line 274:   /// @return @c true iff the status indicates Configuration error.
Nit: ConfigurationError.


Line 297:   /// @return The message portion of the Status.  For @c OK statuses,
Nit: two spaces


Line 309:   Status CloneAndPrepend(const Slice& msg) const;
Summarizing sentence? Below too.


-- 
To view, visit http://gerrit.cloudera.org:8080/3840
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0f82a6c8a500a892bc1daff8444e91191dab3af
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

Reply via email to