Adar Dembo has posted comments on this change.

Change subject: doxygen for C++ client API

Patch Set 9:

File src/kudu/client/client.h:

High level formatting questions:
1. I noticed that you've reformatted the comments to insert two spaces when 
beginning a new sentence rather than one. There's no standard for this as far 
as Kudu is concerned, but I think most of us only use one space. Is this change 
purely stylistic, or is it mandated by doxygen?
2. Should the sentence summarizing a given method end with a period? In some 
cases (e.g. SetVerboseLogLevel, UninstallLoggingCallback) it does, while in 
others (e.g. SetInternalSignalNumber, GetShortVersionString) it doesn't. Is 
there a protocol here? Or is it just inconsistent? If it's just an 
inconsistency, could you fix it? The same applies for parameters and return 
3. Some getters only have a @return while others have both a @return and a 
short summarizing sentence. Why the inconsistency? If either are acceptable, 
I'd prefer if simple getters just had a @return, as it's less noise.

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 leaving 
it on its own as before?

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, 
free functions (like those at the top of client.h) affect all clients because 
they modify global state.

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 they 
both work the same way?

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 done, 
regardless if "the table has not been opened before" or not.

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 configured 
to talk to multiple Kudu masters.

Line 365:   /// @return Default timeout for RPC operations.
Nit: we do distinguish between "operations" and "RPCs". The former are logical 
actions, like "create this new table and wait until it's fully created", while 
the latter are single messages sent to a server, like "Write() sent to a tablet 
server". The distinction is important because an operation may require sending 
multiple RPCs, or retrying some RPCs.

As such, "RPC operations" is confusing. Should probably just drop "operations".

Line 376:   /// by this client.  Check KuduScanner for more details on 
Nit: why the change from "See KuduScanner" to "Check KuduScanner"? Was the 
former unclear or imprecise?

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 we 
restore them?

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 simply 
"the table". Can we do that here too, instead of "next table"? Likewise for 
table_name(), where we use "result table"?

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

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 the 
same as add_hash_partitions() above, with the exception of ...

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

Same comment applies to the other add_hash_partitions().

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 sentence 
as the "summarizing" sentence?

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

That said, since it's just an example, I think it'd be fine to use 

Line 729:   /// Create an object to perform table renaming operation
Hmm, it doesn't really create an object that's uniquely geared for renaming, 
though. A KuduTableAlterer could be used to both rename a table and add a 
column. Maybe reword this so it's more clear?

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

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 
AlterTable operation here, and once the master updates its hard state, only one 
AlterSchema sent to each tablet in the table. And from the client's 
perspective, the entire Alter() is just one operation.

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 above. 
Maybe remove that reference and put it here? I do think "This is the default 
flush mode" is a little clearer than "This is the default for a session".

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 
emphasize that the callback is only invoked when the flush has completely 

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 
intentionally start each parenthesized fragment on a new line so that the 
entire fragment can fit on a single line?

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 to 

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 summarizing 
sentence altogether?

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 

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 of 
just "foos". Maybe remove that to be consistent?

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 scan"?

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 
tablet server? Please clarify, maybe replace with "server".

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: Dinesh Bhat <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <>
Gerrit-HasComments: Yes

Reply via email to