You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2016/08/04 07:12:48 UTC
[kudu-CR] [C++ client] doxygenized all C++ client API
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/3840
Change subject: [C++ client] doxygenized all C++ client API
......................................................................
[C++ client] doxygenized all C++ client API
Doxygenized the rest of header files distributed along with
Kudu C++ client bundle. The client.h file was already doxygenized.
Change-Id: Id0f82a6c8a500a892bc1daff8444e91191dab3af
---
M src/kudu/client/callbacks.h
M src/kudu/client/client.h
M src/kudu/client/resource_metrics.h
M src/kudu/client/row_result.h
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/schema.h
M src/kudu/client/shared_ptr.h
M src/kudu/client/stubs.h
M src/kudu/client/value.h
M src/kudu/client/write_op.h
M src/kudu/common/partial_row.h
M src/kudu/util/monotime.h
M src/kudu/util/slice.h
M src/kudu/util/status.h
15 files changed, 1,245 insertions(+), 388 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/3840/1
--
To view, visit http://gerrit.cloudera.org:8080/3840
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id0f82a6c8a500a892bc1daff8444e91191dab3af
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [C++ client] doxygenized all C++ client API
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: [C++ client] doxygenized all C++ client API
......................................................................
[C++ client] doxygenized all C++ client API
Doxygenized the rest of header files distributed along with
Kudu C++ client bundle. The client.h file was already doxygenized.
Change-Id: Id0f82a6c8a500a892bc1daff8444e91191dab3af
Reviewed-on: http://gerrit.cloudera.org:8080/3840
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/client/callbacks.h
M src/kudu/client/client.h
M src/kudu/client/resource_metrics.h
M src/kudu/client/row_result.h
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/schema.h
M src/kudu/client/shared_ptr.h
M src/kudu/client/stubs.h
M src/kudu/client/value.h
M src/kudu/client/write_op.h
M src/kudu/common/partial_row.h
M src/kudu/util/monotime.h
M src/kudu/util/slice.h
M src/kudu/util/status.h
15 files changed, 1,315 insertions(+), 424 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/3840
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id0f82a6c8a500a892bc1daff8444e91191dab3af
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [C++ client] doxygenized all C++ client API
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: [C++ client] doxygenized all C++ client API
......................................................................
Patch Set 4:
Build Started http://104.196.14.100/job/kudu-gerrit/2749/
--
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [C++ client] doxygenized all C++ client API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [C++ client] doxygenized all C++ client API
......................................................................
Patch Set 5: Code-Review+2
--
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: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [C++ client] doxygenized all C++ client API
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3840
to look at the new patch set (#4).
Change subject: [C++ client] doxygenized all C++ client API
......................................................................
[C++ client] doxygenized all C++ client API
Doxygenized the rest of header files distributed along with
Kudu C++ client bundle. The client.h file was already doxygenized.
Change-Id: Id0f82a6c8a500a892bc1daff8444e91191dab3af
---
M src/kudu/client/callbacks.h
M src/kudu/client/client.h
M src/kudu/client/resource_metrics.h
M src/kudu/client/row_result.h
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/schema.h
M src/kudu/client/shared_ptr.h
M src/kudu/client/stubs.h
M src/kudu/client/value.h
M src/kudu/client/write_op.h
M src/kudu/common/partial_row.h
M src/kudu/util/monotime.h
M src/kudu/util/slice.h
M src/kudu/util/status.h
15 files changed, 1,288 insertions(+), 401 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/3840/4
--
To view, visit http://gerrit.cloudera.org:8080/3840
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id0f82a6c8a500a892bc1daff8444e91191dab3af
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [C++ client] doxygenized all C++ client API
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3840
to look at the new patch set (#5).
Change subject: [C++ client] doxygenized all C++ client API
......................................................................
[C++ client] doxygenized all C++ client API
Doxygenized the rest of header files distributed along with
Kudu C++ client bundle. The client.h file was already doxygenized.
Change-Id: Id0f82a6c8a500a892bc1daff8444e91191dab3af
---
M src/kudu/client/callbacks.h
M src/kudu/client/client.h
M src/kudu/client/resource_metrics.h
M src/kudu/client/row_result.h
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/schema.h
M src/kudu/client/shared_ptr.h
M src/kudu/client/stubs.h
M src/kudu/client/value.h
M src/kudu/client/write_op.h
M src/kudu/common/partial_row.h
M src/kudu/util/monotime.h
M src/kudu/util/slice.h
M src/kudu/util/status.h
15 files changed, 1,315 insertions(+), 424 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/3840/5
--
To view, visit http://gerrit.cloudera.org:8080/3840
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id0f82a6c8a500a892bc1daff8444e91191dab3af
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [C++ client] doxygenized all C++ client API
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3840
to look at the new patch set (#3).
Change subject: [C++ client] doxygenized all C++ client API
......................................................................
[C++ client] doxygenized all C++ client API
Doxygenized the rest of header files distributed along with
Kudu C++ client bundle. The client.h file was already doxygenized.
Change-Id: Id0f82a6c8a500a892bc1daff8444e91191dab3af
---
M src/kudu/client/callbacks.h
M src/kudu/client/client.h
M src/kudu/client/resource_metrics.h
M src/kudu/client/row_result.h
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/schema.h
M src/kudu/client/shared_ptr.h
M src/kudu/client/stubs.h
M src/kudu/client/value.h
M src/kudu/client/write_op.h
M src/kudu/common/partial_row.h
M src/kudu/util/monotime.h
M src/kudu/util/slice.h
M src/kudu/util/status.h
15 files changed, 1,288 insertions(+), 404 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/3840/3
--
To view, visit http://gerrit.cloudera.org:8080/3840
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id0f82a6c8a500a892bc1daff8444e91191dab3af
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [C++ client] doxygenized all C++ client API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [C++ client] doxygenized all C++ client API
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/client/callbacks.h
File src/kudu/client/callbacks.h:
Line 60: /// @param [in] line_number
> Done
You missed the change from "is" to "was".
http://gerrit.cloudera.org:8080/#/c/3840/4/src/kudu/client/callbacks.h
File src/kudu/client/callbacks.h:
Line 46: KuduLoggingCallback() {
Why do we need this?
http://gerrit.cloudera.org:8080/#/c/3840/4/src/kudu/util/slice.h
File src/kudu/util/slice.h:
Line 137: /// @note The data isn't actually modified, just the base/bounds of the slice.
Nit: restyled a bit: "Only the base and bounds of the slice are changed; the data is not modified." Below too.
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?
I think you missed this comment.
--
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [C++ client] doxygenized all C++ client API
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: [C++ client] doxygenized all C++ client API
......................................................................
Patch Set 2:
Build Started http://104.196.14.100/job/kudu-gerrit/2713/
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [C++ client] doxygenized all C++ client API
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [C++ client] doxygenized all C++ client API
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3840/4/src/kudu/client/callbacks.h
File src/kudu/client/callbacks.h:
Line 46: KuduLoggingCallback() {
> Why do we need this?
That's the fun part -- we either need this or call the auto-instantiated constructor of this calls in constructors of the derived classes. Otherwise, the client_samples test fails to compile.
I thought changing the derived classes would be considered as a 'style change'. So, returned back this.
If speaking the best option here, what do you think is the best one: keep this piece of code (kind of strange) or call the KuduLoggingCallback() in the constructors of all the derived classes? Per our discussion, I'm returning the piece of code back.
http://gerrit.cloudera.org:8080/#/c/3840/4/src/kudu/util/slice.h
File src/kudu/util/slice.h:
Line 137: /// @note The data isn't actually modified, just the base/bounds of the slice.
> Nit: restyled a bit: "Only the base and bounds of the slice are changed; th
Done
--
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [C++ client] doxygenized all C++ client API
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: [C++ client] doxygenized all C++ client API
......................................................................
Patch Set 5:
Build Started http://104.196.14.100/job/kudu-gerrit/2754/
--
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: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [C++ client] doxygenized all C++ client API
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: [C++ client] doxygenized all C++ client API
......................................................................
Patch Set 3:
Build Started http://104.196.14.100/job/kudu-gerrit/2748/
--
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [C++ client] doxygenized all C++ client API
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: [C++ client] doxygenized all C++ client API
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/2711/
--
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: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [C++ client] doxygenized all C++ client API
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3840
to look at the new patch set (#2).
Change subject: [C++ client] doxygenized all C++ client API
......................................................................
[C++ client] doxygenized all C++ client API
Doxygenized the rest of header files distributed along with
Kudu C++ client bundle. The client.h file was already doxygenized.
Change-Id: Id0f82a6c8a500a892bc1daff8444e91191dab3af
---
M src/kudu/client/callbacks.h
M src/kudu/client/client.h
M src/kudu/client/resource_metrics.h
M src/kudu/client/row_result.h
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/schema.h
M src/kudu/client/shared_ptr.h
M src/kudu/client/stubs.h
M src/kudu/client/value.h
M src/kudu/client/write_op.h
M src/kudu/common/partial_row.h
M src/kudu/util/monotime.h
M src/kudu/util/slice.h
M src/kudu/util/status.h
15 files changed, 1,270 insertions(+), 392 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/3840/2
--
To view, visit http://gerrit.cloudera.org:8080/3840
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id0f82a6c8a500a892bc1daff8444e91191dab3af
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [C++ client] doxygenized all C++ client API
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [C++ client] doxygenized all C++ client API
......................................................................
Patch Set 2:
(85 comments)
Thank you for the review! Will post the updated patch soon.
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"
Done
Line 62: /// The message to log. It's not terminated with an endline.
> Nit: you have two spaces here after the period.
Done
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"?
Done
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
Done
Line 48: /// Get current count for the specified metrics.
> "specified metric" since it's just one metric.
Done
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 batc
Done
Line 105: /// The returned object is only valid for as long as this KuduScanBatch.
> Nit: "for as long as this KuduScanBatch is valid"
Done
Line 127: class KUDU_EXPORT KuduScanBatch::RowPtr {
> A @brief for this?
RowPtr is documented by the forward declaration for RowPtr earlier. For some reason, if documentation is placed here, it's not shown in a proper place, so I moved it to the place of the forward declaration of RowPtr in KuduScanBatch.
Line 136: bool IsNull(const Slice& col_name) const;
> No summarizing sentences for the two IsNull() methods?
I thought it's not necessary here since we have self-descriptive @return doc. That's one of those cases where @brief (or summary) would just re-iterate whatever @return states (and vice versa).
Line 155: ///@{
> Clever.
I was scared of duplicating that stuff for every method, so I decided to try this and it worked. Nice to have this functionality in doxygen.
Line 172: /// since using indices avoid a hashmap lookup, so index-based getters
> Nit: avoids. Below too.
Done
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.
Done
Line 201: /// @name Getters for string/binary column by column name
> Nit: missing terminating period. Below too.
Done
PS2, Line 255: -- useful
: /// for outputting information into the log.
> Nit: just drop the addendum; "string representation..." is sufficient.
Done
Line 281: class KUDU_EXPORT KuduScanBatch::const_iterator
> No @brief?
Here we have the same situation as with KuduScanBatch::RowPtr -- the documentation for this is tied with the forward-declared KuduScanBatch::const_iterator.
Line 311: /// of the same batch row.
> Nit: "of the same batch"
Done
Line 312: bool operator==(const const_iterator& other) const {
> No summarizing sentence for this or operator!= ?
Done
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 differen
Done
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.
Done
Line 158: /// Construct KuduColumnSchema object as a copy of other object.
> Nit: "as a copy from another object".
For some reason, doxygen tends to show documentation for copy constructors and assignment operators, but not for default constructors and destructor. If there were a configuration directive to change this, it would be nice, but I could not find such a configuration directive.
Line 172: /// Make this object an identical copy of other one.
> Nit: "of the other one."
Done
Line 178: /// Check whether the object is identical to the other.
> Nit: "identical to the other one."
Done
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
Done
Line 233: /// The value to use as the default. The KuduColumnSpec takes ownership
> Got two spaces after the period here.
Done
Line 239: /// Set the preferred compression for the column.
> Nit: "compression type"
Done
Line 242: /// The compression to use.
> Nit: "compression type"
Done
Line 279: /// Set this column to be the primary key of the table.
> Nit: "Set the column" to be consistent.
Done
Line 304: /// Set the type of the column.
> Nit: "data type". Below too.
Done
Line 317: /// Remove the default value for this column.
> Nit: "for the column"
Done
Line 380: KuduColumnSpec* AddColumn(const std::string& name);
> Nit: summarizing sentence?
Done
Line 394: /// The placeholder for the result schema. Upon successful completion,
> Nit: two spaces.
Done
Line 408: /// @brief A representation of schema.
> Nit: perhaps "of a table's schema"?
Done
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/opera
I didn't find how to make doxygen to ignore adding copy constructors/assignment operators into the generated documentation. So, copy the constructor and the assignment operator are added into the auto-generated docs and it's nice to have them documented.
Line 446: bool Equals(const KuduSchema& other) const;
> Missing summarizing sentence for this and for Column() (which is arguably a
Done
Line 469: /// destroyed before the KuduSchema object to omit dangling pointers.
> Nit: "to avoid dangling pointers".
Done
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.
Done
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.
Done
PS2, Line 74: Useful for outputting
: /// information into log, etc.
> Nit: no need for this.
Done
Line 84: explicit KuduWriteOperation(const sp::shared_ptr<KuduTable>& table);
> The protected members aren't accessible to users, so why bother documenting
This is a just trick to avoid warnings from doxygen on undocumented members. I'd like to have not a single warning output by doxygen (now we can turn on the detection of those warnings in the makefile and catch un-documented members, etc.).
The doxygen treats protected members as a part of interface to be documented. The reasoning is that if someone is about to inherit from the class, it's nice to have the accessible members of the base class documented.
Notice the @cond ... statement: effectively, we have no documentation generated for these protected members since the PROTECTED_MEMBERS_DOCUMENTED is set to NO/false.
Line 173: /// in the schema to be set in the embedded PartialRow object.
> Nit: KuduPartialRow. Same with KuduDelete.
Done
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 Ku
Done
Line 62: /// Create a copy of KuduPartialRow instance.
> Do we really need to document these?
Yes -- as I mentioned earlier, the copy constructor and the assignment operator should be documented. Otherwise, there will be a warning from doxygen. I don't know how to change this behavior in current version of doxygen -- may be, in the future versions there will be some configuration switch to change this.
PS2, Line 75: //------------------------------------------------------------
: // Setters
: //------------------------------------------------------------
> How does doxygen treat these separators?
It ignores these. I'll remove them.
PS2, Line 106: The same a
> Nit: "These setters are the same as the corresponding ...".
Done
PS2, Line 151: The same as
> Nit: "These setters are the same as the corresponding ...".
Done
Line 250: bool IsColumnSet(const Slice& col_name) const;
> Do the IsColumnSet() and IsNull() methods deserve summarizing sentences?
Done
PS2, Line 259: the given column has @c NULL value.
> Nit: "the given column's value is @c NULL". Below too.
Done
PS2, Line 295: The same as
> Nit: "These getters are the same as the corresponding ...".
Done
Line 346: /// since using indices avoid a hashmap lookup, so index-based getters
> "since they use indices to avoid hashmap lookups".
Done
Line 379: /// @return Operation result status. In particular, returns InvalidArgument
> Nit: two spaces here.
Done
PS2, Line 395: if all of the key columns have been specified
: /// for this mutation.
> Nit" If all key column values have been set."
Done
Line 399: /// @return @c true if all columns have been specified.
> Nit: "if all column values have been set."
Done
PS2, Line 402: useful
: /// for outputting information into the log.
> Nit: drop.
Done
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"
Done
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 r
Done
Line 117: /// Represent this time interval as a timespec structure.
> With nanonsecond accuracy.
Done
Line 152: /// accurate to within a millisecond or two. The speed difference will
> Nit: two spaces.
Done
Line 159: /// @name Convertion constants for ubiquitous time units.
> "Conversion"
Done
Line 187: static const MonoTime& Earliest(const MonoTime& a, const MonoTime& b);
> Nit: summarizing sentence?
Done
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"
Done
Line 228: /// is the same as represented by the other.
> Nit: "is the same as the one..."
Done
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 sh
Done
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 element
Done
Line 100: /// @note Further appends to the faststring may invalidate this slice.
> Does this apply to StringPieces too?
It seems it does not. I'll remove this.
Line 126: /// @return the ith byte in the referenced data.
> Nit: nth byte?
Done
Line 132: /// Change this slice to refer to an empty array
> Nit: terminating period.
Done
Line 143: /// Number of bytes to drop/skip in the beginning.
> Nit: "Number of bytes that should be dropped".
Done
Line 155: /// The number of bytes to have after the truncation.
> Nit: "New size of the slice."
Done
Line 161: /// Check the slice has the expected size.
> Nit: "Check that the slice..."
Done
PS2, Line 164: Status::OK iff size() == @c expected_size;
: /// Status::Corruption() otherwise.
> How about just "Corruption iff size() != @c expected_size"?
Done
Line 168: /// @return A string that contains the copy of the referenced data.
> Nit: "...contains a copy of the..."
Done
Line 175: /// @c 0 means there is no limit.
> Nit: "@c 0 means no limit"
Done
Line 179: /// Do three-way comparison of the slice's data.
> Nit: "Do a three-way comparison..."
Done
Line 256: /// @return @c true iff two slices contain byte-to-byte identical data.
> Nit: "byte-for-byte"
Done
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.
Done
http://gerrit.cloudera.org:8080/#/c/3840/2/src/kudu/util/status.h
File src/kudu/util/status.h:
Line 103: /// @brief The class to represent outcome of performed operation.
> Nit: "Representation of an operation's outcome"
Done
Line 111: /// Copy the specified status.
> Do we need these?
Yep -- to avoid doxygen warnings in with our current configuration.
Line 130: /// Assign the specified status using the move semantics (C++11).
> Nit: "using move semantics"
Done
Line 241: /// @return @c true iff the status indicates an InvalidArgument error
> Nit: terminate with period. Below too.
Done
Line 253: /// @return @c true iff the status indicates a IllegalState.
> Nit: an IllegalState.
Done
Line 256: /// @return @c true iff the status indicates a NotAuthorized.
> Nit: NotAuthorized error.
Done
Line 262: /// @return @c true iff the status indicates RemoteError.
> Nit: a RemoteError.
Done
Line 274: /// @return @c true iff the status indicates Configuration error.
> Nit: ConfigurationError.
Done
Line 297: /// @return The message portion of the Status. For @c OK statuses,
> Nit: two spaces
Done
Line 309: Status CloneAndPrepend(const Slice& msg) const;
> Summarizing sentence? Below too.
Done
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [C++ client] doxygenized all C++ client API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes