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/11/07 04:38:58 UTC
[kudu-CR] [c++ client] implemented session operations stats
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4974
Change subject: [c++ client] implemented session operations stats
......................................................................
[c++ client] implemented session operations stats
The C++ client API updated to provide information on write operations
performed in the context of a Kudu session. The stats are not reset
during the lifetime of a session.
Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/error_collector.cc
M src/kudu/client/error_collector.h
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
A src/kudu/client/write_op_stats.h
8 files changed, 207 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/4974/1
--
To view, visit http://gerrit.cloudera.org:8080/4974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [c++ client] implemented session operations stats
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: [c++ client] implemented session operations stats
......................................................................
Patch Set 2:
Alexey, thanks for doing this. As we discussed, one of the issues Impala faces is that the error codes are too coarse grained right now, so we can't differentiate some important error cases. I think this mechanism will be useful when it can also include breakdowns of the errors, more fine-grained than the error codes (which may need to remain as-is for backwards compatibility). However, until we can report the breakdown into more fine grained error stats, I don't think Impala will use this over the GetErrorStats, so it's up to you if you'd wanna still get this in in the meantime or shelve the change for later.
--
To view, visit http://gerrit.cloudera.org:8080/4974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be
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: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [c++ client] implemented session operations stats
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: [c++ client] implemented session operations stats
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4974/2//COMMIT_MSG
Commit Message:
PS2, Line 10:
nit: extra space
http://gerrit.cloudera.org:8080/#/c/4974/2/src/kudu/client/client.h
File src/kudu/client/client.h:
PS2, Line 1504: CountTotalOperations
Should we call this CountTotalWriteOperations to be more precise? Same below.
http://gerrit.cloudera.org:8080/#/c/4974/2/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:
PS2, Line 326: write_op_stats_->IncFailed();
nit: Here we don't add to the error collector but we increment failed. Below we add to the error collector which will also increment failed. Is a write operation even failing if it doesn't exist?
--
To view, visit http://gerrit.cloudera.org:8080/4974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be
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: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [c++ client] implemented session operations stats
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has abandoned this change.
Change subject: [c++ client] implemented session operations stats
......................................................................
Abandoned
It seems this is not needed in this form -- Impala client could maintain these metrics itself, if needed. IIRC, what they really need is detailed information on failed operations classified by write operation type (insert/upsert/delete/update), especially for upserts. That should be addressed separately and is going to be a more intrusive change.
--
To view, visit http://gerrit.cloudera.org:8080/4974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [c++ client] implemented session operations stats
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [c++ client] implemented session operations stats
......................................................................
Patch Set 2:
> Alexey, thanks for doing this. As we discussed, one of the issues
> Impala faces is that the error codes are too coarse grained right
> now, so we can't differentiate some important error cases. I think
> this mechanism will be useful when it can also include breakdowns
> of the errors, more fine-grained than the error codes (which may
> need to remain as-is for backwards compatibility). However, until
> we can report the breakdown into more fine grained error stats, I
> don't think Impala will use this over the GetErrorStats, so it's up
> to you if you'd wanna still get this in in the meantime or shelve
> the change for later.
All right, thanks for the clarification. The part with the custom error info requires more time, and I think I can start working on that once we are done with the security-related stuff. Meanwhile, we can add this path independently.
--
To view, visit http://gerrit.cloudera.org:8080/4974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be
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: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [c++ client] implemented session operations stats
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [c++ client] implemented session operations stats
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4974/2//COMMIT_MSG
Commit Message:
PS2, Line 10:
> It made it easier to read when using typewriters but that's a long time ago
That hasn't changed since then -- people's eyes does not evolve with the tech :)
As I understand, the only case when it does not make difference is HTTP or some other text processing where the end-result does not contain the original formatting. In that case yes -- it does not make sense to have extra spaces since the text is re-formatted anyway. But for source code and commit messages which are displayed verbatim even in IDEs/browsers, that does make sense.
However, if we want to have single-spaces everywhere, I'll update this. But there is one other place where two spaces are used in every source code file: the Apache license :)
http://gerrit.cloudera.org:8080/#/c/4974/2/src/kudu/client/client.h
File src/kudu/client/client.h:
PS2, Line 1504: CountTotalOperations
> Yeah that sounds good.
All right, I'm going to use total_write_ops() and failed_write_ops().
http://gerrit.cloudera.org:8080/#/c/4974/2/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:
PS2, Line 326: write_op_stats_->IncFailed();
> I'm in favor of consistency, or at least to document the discrepancy.
ok, I'll add comments and corresponding test as well.
--
To view, visit http://gerrit.cloudera.org:8080/4974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be
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: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [c++ client] implemented session operations stats
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Dinesh Bhat, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4974
to look at the new patch set (#3).
Change subject: [c++ client] implemented session operations stats
......................................................................
[c++ client] implemented session operations stats
The C++ client API updated to provide information on write operations
performed in the context of a Kudu session. The stats are not reset
during the lifetime of a session.
Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/error_collector.cc
M src/kudu/client/error_collector.h
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
7 files changed, 194 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/4974/3
--
To view, visit http://gerrit.cloudera.org:8080/4974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be
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: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [c++ client] implemented session operations stats
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [c++ client] implemented session operations stats
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4974/2/src/kudu/client/client.h
File src/kudu/client/client.h:
Line 1503: /// @return Total count of write operations in the session.
What do you think of reusing kudu::client::ResourceMetrics for this purpose?
http://gerrit.cloudera.org:8080/#/c/4974/2/src/kudu/client/error_collector.cc
File src/kudu/client/error_collector.cc:
Line 21: #include <type_traits>
What is this for?
http://gerrit.cloudera.org:8080/#/c/4974/2/src/kudu/client/write_op_stats.h
File src/kudu/client/write_op_stats.h:
Line 20: #include <atomic>
I think the jury is still out regarding std::atomic vs. our built-in atomics. See http://gerrit.cloudera.org:8080/1842 for more details.
I suggest that, outside of unit tests, we continue to use built-in atomics.
--
To view, visit http://gerrit.cloudera.org:8080/4974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be
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: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [c++ client] implemented session operations stats
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [c++ client] implemented session operations stats
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4974/2//COMMIT_MSG
Commit Message:
PS2, Line 10:
> nit: extra space
That's intentional -- two spaces after a stop makes it easier to read, IMO. Since we have no style requirements on commit messages which require just one space after a stop, I'm using the style which I'm used to.
Is it OK?
http://gerrit.cloudera.org:8080/#/c/4974/2/src/kudu/client/client.h
File src/kudu/client/client.h:
PS2, Line 1504: CountTotalOperations
> Should we call this CountTotalWriteOperations to be more precise? Same belo
Yep, that makes sense. However, I don't like the idea having long names a la CountThoseWriteOperationsThatHaveFailed()
What about
* TotalWriteOps()
* total_write_ops()
and
* FailedWriteOps()
* failed_write_ops()
or alike?
http://gerrit.cloudera.org:8080/#/c/4974/2/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:
PS2, Line 326: write_op_stats_->IncFailed();
> nit: Here we don't add to the error collector but we increment failed. Belo
The idea was to provide stats based on total calls of the Apply() method: we don't know what was the intention of the user when this method was called with null pointer.
It might happen that the null pointer happened because of programmatic error the application code. Since that was reported as an error and the call didn't pass, for consistency reasons I think it's better to update the error counter.
I mean, if we update the total counter, then it's a must to update the error counter in such a case. And I think it makes sense to update the error counter for a null pointer argument error because otherwise that error would not be reflected in the stats.
We could ask Matthew to provide his opinion on this matter as well -- this patch appeared due to the recent discussion of those issues with Dan, Matthew and me.
--
To view, visit http://gerrit.cloudera.org:8080/4974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be
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: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [c++ client] implemented session operations stats
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [c++ client] implemented session operations stats
......................................................................
Patch Set 3:
As it turned out, this change is not very useful if not adding additional fine-grained stats on types of errors. Given the fact it modifies the public client API, it does not make sense to continue with it in the absence of demand from the consumer side.
Will need to return back with more systematic approach for fine-grained stats for errors.
--
To view, visit http://gerrit.cloudera.org:8080/4974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be
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: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [c++ client] implemented session operations stats
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [c++ client] implemented session operations stats
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4974/2/src/kudu/client/client.h
File src/kudu/client/client.h:
Line 1503: /// @return Total count of write operations in the session.
> What do you think of reusing kudu::client::ResourceMetrics for this purpose
Thank you for the reference. It seems the ResourceMetrics class is heavier and all its operations require synchronization. I'm not sure that's the best choice for the counter which is put into the hot paths with the KuduSession::Apply() calls.
I think it's better to have something more lightweight for these simple counters.
http://gerrit.cloudera.org:8080/#/c/4974/2/src/kudu/client/error_collector.cc
File src/kudu/client/error_collector.cc:
Line 21: #include <type_traits>
> What is this for?
This is where std::move() comes from.
http://gerrit.cloudera.org:8080/#/c/4974/2/src/kudu/client/write_op_stats.h
File src/kudu/client/write_op_stats.h:
Line 20: #include <atomic>
> I think the jury is still out regarding std::atomic vs. our built-in atomic
All right, will use the in-house atomics then.
--
To view, visit http://gerrit.cloudera.org:8080/4974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be
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: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [c++ client] implemented session operations stats
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [c++ client] implemented session operations stats
......................................................................
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/4974/3//COMMIT_MSG
Commit Message:
PS3, Line 9:
missing a word? "is"?
PS3, Line 10: in the context
s/in the context/during the lifetime
PS3, Line 10: The stats are not reset
: during the lifetime of a session.
with the fix to the previous sentence I don't think you need this one (its implicit)
http://gerrit.cloudera.org:8080/#/c/4974/3/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
PS3, Line 2686: EXPECT_EQ(1, session->total_write_ops());
: EXPECT_EQ(0, session->failed_write_ops());
Is it really interesting to be checking the stats in all these places? this will make changing these tests harder. Are we really measuring something useful?
http://gerrit.cloudera.org:8080/#/c/4974/3/src/kudu/client/client.h
File src/kudu/client/client.h:
PS3, Line 1505:
.. of _valid_/_successful_ invocations?
PS3, Line 1518: and
nit: s/and/or
PS3, Line 1526: failed_write_ops(
on a more general note: does it make sense to measure the failed calls to Apply()? At first glance I would think this would be interesting to measure Kudu client/server errors not API misusage, or did I miss something?
--
To view, visit http://gerrit.cloudera.org:8080/4974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [c++ client] implemented session operations stats
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/4974
to look at the new patch set (#2).
Change subject: [c++ client] implemented session operations stats
......................................................................
[c++ client] implemented session operations stats
The C++ client API updated to provide information on write operations
performed in the context of a Kudu session. The stats are not reset
during the lifetime of a session.
Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/error_collector.cc
M src/kudu/client/error_collector.h
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
A src/kudu/client/write_op_stats.h
8 files changed, 207 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/4974/2
--
To view, visit http://gerrit.cloudera.org:8080/4974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be
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: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [c++ client] implemented session operations stats
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: [c++ client] implemented session operations stats
......................................................................
Patch Set 2: Code-Review+1
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4974/2/src/kudu/client/write_op_stats.h
File src/kudu/client/write_op_stats.h:
PS2, Line 37: ++total_
The standard seem to suggest that ++ operation follows strict synchronization semantics about memory ordering. If we know that this is always getting incremented, we could use :
total_.fetch_add(1, std::memory_order_relaxed);
PS2, Line 37: IncTotal
Nit: suggest using a nomenclature which could reflect that this is an atomic operation vs a regular increment op.
PS2, Line 40: total_
I wonder if this is equivalent to total_.load(memory_order_relaxed) ?
PS2, Line 49: total_count_
Do you want to point out where is this variable declared ?
--
To view, visit http://gerrit.cloudera.org:8080/4974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be
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: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [c++ client] implemented session operations stats
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: [c++ client] implemented session operations stats
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4974/2//COMMIT_MSG
Commit Message:
PS2, Line 10:
> That's intentional -- two spaces after a stop makes it easier to read, IMO.
It made it easier to read when using typewriters but that's a long time ago :)
I'm strongly in favor of not using double spaces anywhere, commit messages are no different than code to me.
http://gerrit.cloudera.org:8080/#/c/4974/2/src/kudu/client/client.h
File src/kudu/client/client.h:
PS2, Line 1504: CountTotalOperations
> Yep, that makes sense. However, I don't like the idea having long names a
Yeah that sounds good.
http://gerrit.cloudera.org:8080/#/c/4974/2/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:
PS2, Line 326: write_op_stats_->IncFailed();
> The idea was to provide stats based on total calls of the Apply() method: w
I'm in favor of consistency, or at least to document the discrepancy.
--
To view, visit http://gerrit.cloudera.org:8080/4974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be
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: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [c++ client] implemented session operations stats
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [c++ client] implemented session operations stats
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4974/2/src/kudu/client/write_op_stats.h
File src/kudu/client/write_op_stats.h:
PS2, Line 37: IncTotal
> Nit: suggest using a nomenclature which could reflect that this is an atomi
Good point -- I'll add a comment on that describing why atomics are used, but I don't think the signature should reflect that.
In my opinion, the interface of this simple class should not expose the implementation details (so-called encapsulation). The atomics here are used only because those counters could be updated from multiple places -- both from the main thread where the KuduSession::Apply() is called and from the background reactor threads which can call these methods (actually, only IncFailed()) with the results of the RPC call.
PS2, Line 37: ++total_
> The standard seem to suggest that ++ operation follows strict synchronizati
Yes, good point.
PS2, Line 40: total_
> I wonder if this is equivalent to total_.load(memory_order_relaxed) ?
I think that due to the consistency it should be an operation with memory_order_seq_cst.
You can the details of the implementation looking into template atomic::<T>::operator T() method.
PS2, Line 49: total_count_
> Do you want to point out where is this variable declared ?
Done
--
To view, visit http://gerrit.cloudera.org:8080/4974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be
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: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes