Alexey Serbin has posted comments on this change. Change subject: doxygen for C++ client API ......................................................................
Patch Set 9: (64 comments) http://gerrit.cloudera.org:8080/#/c/3619/9/src/kudu/client/client.h 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 Done Line 131: /// Signal number to use for internal > Nit: for internal what? Maybe you meant "Signal number to use internally"? Done 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 Done Line 247: /// @return Pointer to newly created instance: it's the caller's > Nit: semi-colon is more appropriate here. Done Line 277: /// Check if table alteration is in process > Nit: in-progress, to be consistent with IsCreateTableInProgress. Done 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 Done Line 287: /// Get scheme for a table > Nit: schema, not scheme. Done Line 290: /// Name of the table if question > Nit: in question Done Line 292: /// Raw pointer to the schema object: caller gets ownership > Nit: semi-colon. Done Line 310: /// Substring-filter to use; empty sub-string filter matches any tables > Nit: Substring filter Done Line 320: /// Set only on success: set to @c true iff table exists > Nit: semi-colon Done PS9, Line 326: /// If the table has not been opened before, then open the table : /// with the given name. If the table has not been opened before : /// 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 Done Line 346: /// @return A new session object: caller is responsible for destroying it. > Nit: semi-colon. Done 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 Done Line 365: /// @return Default timeout for RPC operations. > Nit: we do distinguish between "operations" and "RPCs". The former are logi Done Line 376: /// by this client. Check KuduScanner for more details on timestamps. > 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. Done 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. Done 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 Done PS9, Line 462: two > Nit: to be consistent with the rest of this paragraph, use '2' instead of ' Done 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 Done PS9, Line 482: Names of columns to build partition > To be consistent with set_range_partition_columns(), how about "to use for Done Line 499: /// Name of columns to use for partitioning. Every column must be > Nit: Names of columns Done 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 Done Line 533: /// If this method is not called, the table's range is be unbounded. > Nit: "is be"? Done Line 539: /// on the table range. If a column of the @c lower_bound row is missing > Nit: missing a value. Below too. Done Line 635: /// Get scheme for the table > Nit: schema Done Line 662: /// This method creates a new instance of comparison predicate which > Nit: "new instance of a comparison predicate" Done Line 666: /// Name of column to use for comparision > Nit: comparison. Below too. Done Line 678: /// The returned predicate object takes ownership over the @c value > Nit: This sentence should end in a period. Done 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? Done 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 Done PS9, Line 743: The caller : /// is responsible for freeing the object. > Nope; the KuduColumnSpec objects belong to the KuduTableAlterer and are fre Done 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 Done PS9, Line 780: /// Whether to wait for any alteration operation to complete before : /// returning control > See above. Done Line 820: /// Release the operation that failed: must only be called only once. > Nit: period instead of semi-colon here. Done PS9, Line 920: This is the default for a session > Not your fault, but "This is the default flush mode" is already printed abo Done Line 952: /// to ensure nothis is pending. > "to ensure nothing is pending" Done Line 1058: /// flush modes that return immediately, 'cb' is triggered with the result. > Nit: should be @c cb, not 'cb'? Done 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 inserted, : /// 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 inserted : /// session->FlushAsync(callback_2); // called immediately! : /// @endcode : /// Note that, as in all other async functions in Kudu, the callback : /// 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()? Done Line 1111: /// Flush any pending writes: the async version. > Nit: how about "Flush any pending writes asynchronously"? Done 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 Done 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 in-flight : /// 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 mode. > Not your fault, but errors may accumulate while in any flush mode. Done Line 1159: int CountPendingErrors() const; > Not your fault, but we should indicate that this count is reset by a call t Done Line 1164: /// Pointer to the container to full with error info objects. Caller takes > Nit: fill, not full Done Line 1192: /// @brief This class represents a single scanner. > Nit: how about "Representation of a single scan"? Done Line 1253: /// Constructor -- it initializes the scanner upon construction. > Somewhat self-explanatory, no? Maybe omit the second part? Or the summarizi Done Line 1377: /// Closing scanner releases resources on the server. This call does not > Nit: Closing the scanner... Done Line 1382: /// to reuse this Scanner object. > Nit: "this object" (the class name has changed since this was originally wr Done Line 1386: /// @return @c true if there may be rows to be fetched from this scanner. > Nit: separate @return from the summarizing sentence with an empty line? Done Line 1413: /// The placeholder for the result. > Nit: "Placeholder for the result" (to be consistent with the others). Done Line 1427: /// @return the cumulative resource metrics since the scan was started. > Nit: "Cumulative resource metrics..." Done Line 1470: /// @return The schema of the projection being scanned. > Nit: "Schema of the projection..." Done Line 1534: /// @return Vector of tablet servers who may be hosting the tablet which > Nit: I think this is the only place where we say "vector of foos" instead o Done Line 1552: /// Client to bound the scanner to. > Nit: "Client to bind to the scanner" Done Line 1554: /// Token to pass the parameters in serialized form. > How about: "Token containing serialized scanner parameters"? Done 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 Done Line 1600: Status SetProjectedColumnNames(const std::vector<std::string>& col_names) > Can we @copydoc this from KuduScanner? Or vice versa? Done Line 1620: /// of this method make the specified set of predicates to work in > Nit: "make the specified set of predicates work in conjunction" Done Line 1621: /// conjunction -- i.e. all predicates must pass for a row to be returned. > Nit: "must be true" instead of "must pass" Done Line 1678: /// Make scans resumable at another tablet server if current one fails. > Nit: "one" suffers from subject confusion; does it refer to a scan or to a 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: 9 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
