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 <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
