[kudu-CR] [c++ client] implemented session operations stats

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [c++ client] implemented session operations stats

[David Ribeiro Alves (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] implemented session operations stats

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [c++ client] implemented session operations stats

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] implemented session operations stats

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [c++ client] implemented session operations stats

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] implemented session operations stats

[Matthew Jacobs (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [c++ client] implemented session operations stats

[Jean-Daniel Cryans (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] implemented session operations stats

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] implemented session operations stats

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] implemented session operations stats

[Alexey Serbin (Code Review)]

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