Alexey Serbin has posted comments on this change.

Change subject: doxygen for C++ client API

Patch Set 9:

File src/kudu/client/client.h:

PS9, Line 91: Before a callback is registered, all internal client log events
            : /// are logged to the stderr.
> Nit: why combine this sentence into the previous paragraph instead of leavi

Line 131: ///   Signal number to use for internal
> Nit: for internal what? Maybe you meant "Signal number to use internally"?

PS9, Line 222: Each KuduClient instance is sandboxed with no
             : /// global cross-client state.
> Not related to your change, but this isn't strictly true anymore. Basically

Line 247:   /// @return Pointer to newly created instance: it's the caller's
> Nit: semi-colon is more appropriate here.

Line 277:   /// Check if table alteration is in process
> Nit: in-progress, to be consistent with IsCreateTableInProgress.

PS9, Line 282:   ///   Set to @c true if an AlterTable operation is in-progress;
             :   ///   set to @c false otherwise
> Nit: can you reword to be consistent with IsCreateTableInProgress, since th

Line 287:   /// Get scheme for a table
> Nit: schema, not scheme.

Line 290:   ///   Name of the table if question
> Nit: in question

Line 292:   ///   Raw pointer to the schema object: caller gets ownership
> Nit: semi-colon.

Line 310:   ///   Substring-filter to use; empty sub-string filter matches any 
> Nit: Substring filter

Line 320:   ///   Set only on success: set to @c true iff table exists
> Nit: semi-colon

PS9, Line 326:   /// If the table has not been opened before, then open the 
             :   /// with the given name.  If the table has not been opened 
             :   /// for this client, this will do an RPC to ensure that the 
table exists
             :   /// and look up its schema.
> Also not related to this change, but the RPC and schema lookup are always d

Line 346:   /// @return A new session object: caller is responsible for 
destroying it.
> Nit: semi-colon.

Line 359:   /// @return @c true iff it's a multi-master session
> This isn't related to sessions per-se, it's true if the client was configur

Line 365:   /// @return Default timeout for RPC operations.
> Nit: we do distinguish between "operations" and "RPCs". The former are logi

Line 376:   /// by this client.  Check KuduScanner for more details on 
> Nit: why the change from "See KuduScanner" to "Check KuduScanner"? Was the 
I was playing with @see directive here, but doxygen itself inserts hyperlink to 
KuduScanner here.  I dropped the '@' after realizing that.  OK, now it's back 
to 'See'.

PS9, Line 385: To use this the user must obtain
             :   /// the HybridTime encoded timestamp from the first client with
             :   /// KuduClient::GetLatestObservedTimestamp() and then set it
             :   /// in the new client with this method.
> Nit: let's separate this into a new paragraph.

Line 432: class KUDU_EXPORT KuduTableCreator {
> The "Required" and "Optional" tags are gone. I think they were useful; can 
I don't understand what "Required" and "Optional" tags are in this context.  
Could you clarify?

Line 438:   /// This method must be called at an object before .
> This sentence is unclear, and there's an extra space at the end.

Line 447:   /// Set the schema with which to create next table.
> Nit: the subsequent builder methods refer to the table to be created as sim

PS9, Line 462: two
> Nit: to be consistent with the rest of this paragraph, use '2' instead of '

PS9, Line 476:   /// This method takes a seed value, which can be used to 
randomize the
             :   /// mapping of rows to hash buckets.  Setting the seed may 
provide some
             :   /// amount of protection against denial of service attacks 
when the hashed
             :   /// columns contain user provided values.
> Can you add a small note at the beginning explaining that this is exactly t

PS9, Line 482: Names of columns to build partition
> To be consistent with set_range_partition_columns(), how about "to use for 

Line 499:   ///   Name of columns to use for partitioning.  Every column must be
> Nit: Names of columns

PS9, Line 524:   /// Add a partition range based on lower and upper bounds.
             :   ///
             :   /// Add a partition range bound to the table with an inclusive 
lower bound
             :   /// and exclusive upper bound.
> These two sentences are almost the same, maybe we can just the second sente

Line 533:   /// If this method is not called, the table's range is be unbounded.
> Nit: "is be"?

Line 539:   ///   on the table range.  If a column of the @c lower_bound row is 
> Nit: missing a value. Below too.

Line 635:   /// Get scheme for the table
> Nit: schema

Line 662:   /// This method creates a new instance of comparison predicate which
> Nit: "new instance of a comparison predicate"

Line 666:   ///   Name of column to use for comparision
> Nit: comparison. Below too.

Line 678:   ///   The returned predicate object takes ownership over the @c 
> Nit: This sentence should end in a period.

PS9, Line 687:   /// Get the KuduClient object associated with the table
             :   KuduClient* client() const;
             :   /// Get partition scheme for the table
             :   const PartitionSchema& partition_schema() const;
> Shouldn't these two be structured as just @return?

PS9, Line 722: /// @todo there are 2 memory leaks in the example above: fix it 
or change
             : ///   signature of the methods to return managed pointers 
instead of raw ones
             : ///   (unique_ptr, shared_ptr, etc.)
> Those leaks only happen if a method throws an exception, and no Kudu code t
The memory leaks mentioned are alterer->AddColumn("...") and 
alterer->AlterColumn("..."), since the comments for those methods state both 
methods transfer ownership for the created objects to the caller.  OK, I'll 
update those comments and the example accordingly.

Line 729:   /// Create an object to perform table renaming operation
> Hmm, it doesn't really create an object that's uniquely geared for renaming

PS9, Line 743: The caller
             :   ///   is responsible for freeing the object.
> Nope; the KuduColumnSpec objects belong to the KuduTableAlterer and are fre

PS9, Line 775:   /// If set to true, every alteration operation returns control 
only after
             :   /// the opration is complete.  Otherwise, every operation 
returns immediately.
             :   /// By default (i.e. when an alteration object is created) it 
is set to true.
> Not sure why this talks about "every operation"; there's only one logical A

PS9, Line 780:   ///   Whether to wait for any alteration operation to complete 
             :   ///   returning control
> See above.

Line 820:   /// Release the operation that failed: must only be called only 
> Nit: period instead of semi-colon here.

PS9, Line 920: This is the default for a session
> Not your fault, but "This is the default flush mode" is already printed abo

Line 952:   /// to ensure nothis is pending.
> "to ensure nothing is pending"

Line 1058:   /// flush modes that return immediately, 'cb' is triggered with 
the result.
> Nit: should be @c cb, not 'cb'?

PS9, Line 1081:   /// In the case that the async version of this method is 
used, then
              :   /// the callback will be called upon completion of the 
operations which
              :   /// were buffered since the last flush.  In other words, in 
the following
              :   /// sequence:
              :   /// @code
              :   ///   session->Insert(a);
              :   ///   session->FlushAsync(callback_1);
              :   ///   session->Insert(b);
              :   ///   session->FlushAsync(callback_2);
              :   /// @endcode
              :   /// ... @c callback_2 will be triggered once @c b has been 
              :   /// regardless of whether @c a has completed or not.
              :   ///
              :   /// @note This also means that, if FlushAsync is called twice 
in succession,
              :   /// with no intervening operations, the second flush will 
return immediately.
              :   /// For example:
              :   /// @code
              :   ///   session->Insert(a);
              :   ///   session->FlushAsync(callback_1); // called when 'a' is 
              :   ///   session->FlushAsync(callback_2); // called immediately!
              :   /// @endcode
              :   /// Note that, as in all other async functions in Kudu, the 
              :   /// may be called either from an IO thread or the same thread 
which calls
              :   /// FlushAsync.  The callback should not block.
> Maybe this should be moved to FlushAsync()?

Line 1111:   /// Flush any pending writes: the async version.
> Nit: how about "Flush any pending writes asynchronously"?

Line 1114:   ///   Callback to report on flush results.  The @c cb must remain 
valid until
> Nit: how about "upon flush completion" instead of "on flush results" to emp

PS9, Line 1128: /// @return @c true if there are operations which have not yet 
been delivered
              :   ///   to the cluster.  This may include buffered operations
              :   ///   (i.e. those that have not yet been flushed) as well as 
              :   ///   operations
              :   ///   (i.e. those that are in the process of being sent to 
the servers).
> The formatting of the content in parens is a little strange. Did you intent
Yes, I did.  Returned back.

Line 1154:   /// Errors may accumulate when using the @c AUTO_FLUSH_BACKGROUND 
> Not your fault, but errors may accumulate while in any flush mode.

Line 1159:   int CountPendingErrors() const;
> Not your fault, but we should indicate that this count is reset by a call t

Line 1164:   ///   Pointer to the container to full with error info objects.  
Caller takes
> Nit: fill, not full

Line 1192: /// @brief This class represents a single scanner.
> Nit: how about "Representation of a single scan"?

Line 1253:   /// Constructor -- it initializes the scanner upon construction.
> Somewhat self-explanatory, no? Maybe omit the second part? Or the summarizi

Line 1377:   /// Closing scanner releases resources on the server.  This call 
does not
> Nit: Closing the scanner...

Line 1382:   ///   to reuse this Scanner object.
> Nit: "this object" (the class name has changed since this was originally wr

Line 1386:   /// @return @c true if there may be rows to be fetched from this 
> Nit: separate @return from the summarizing sentence with an empty line?

Line 1413:   ///   The placeholder for the result.
> Nit: "Placeholder for the result" (to be consistent with the others).

Line 1427:   /// @return the cumulative resource metrics since the scan was 
> Nit: "Cumulative resource metrics..."

Line 1470:   /// @return The schema of the projection being scanned.
> Nit: "Schema of the projection..."

Line 1534:   /// @return Vector of tablet servers who may be hosting the tablet 
> Nit: I think this is the only place where we say "vector of foos" instead o

Line 1552:   ///   Client to bound the scanner to.
> Nit: "Client to bind to the scanner"

Line 1554:   ///   Token to pass the parameters in serialized form.
> How about: "Token containing serialized scanner parameters"?

Line 1586:   ///   The table to work with.  The given object must remain valid
> Nit: "work with" is a little vague. Perhaps "The table the tokens should sc

Line 1600:   Status SetProjectedColumnNames(const std::vector<std::string>& 
> Can we @copydoc this from KuduScanner? Or vice versa?

Line 1620:   ///   of this method make the specified set of predicates to work 
> Nit: "make the specified set of predicates work in conjunction"

Line 1621:   ///   conjunction -- i.e. all predicates must pass for a row to be 
> Nit: "must be true" instead of "must pass"

Line 1678:   /// Make scans resumable at another tablet server if current one 
> Nit: "one" suffers from subject confusion; does it refer to a scan or to a 

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Dinesh Bhat <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <>
Gerrit-Reviewer: Misty Stanley-Jones <>
Gerrit-HasComments: Yes

Reply via email to