Todd Lipcon has posted comments on this change. Change subject: [java] KUDU-2013 re-acquire authn token if expired ......................................................................
Patch Set 10: (2 comments) just leaving some reponses to the comments on r8. will look at the new rev following. http://gerrit.cloudera.org:8080/#/c/7250/8/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java File java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java: PS8, Line 276: euedMessages = null; : // Send out all the enqueu > Is it better to use assert instead of Preconditions here? Could you elabor true that assertions aren't on by default, so they are more like DCHECK() in our C code. I believe we do enable them in our unit test runs, at least. I dislike Preconditions here because the type of exception thrown is "IllegalStateException" which is supposed to indicate to a caller that they called a method at an inappropriate time. In my mind it's meant to indicate "misuse" of an API, rather than "mis-programming/bug" of an API (which an AssertionError more clearly represents). Again to make the comparison to our C++ style, we use CHECK/DCHECK for cases where it would be an internal bug to see it fail, and "if (...) return Status::IllegalArgument(...)" or whatever when it would just be misuse of an API. Perhaps worth starting a thread about this so we can bike-shed an appropriate amount? Line 482: > Yes, it might. But as I understand if we don't want to keep the channel ar I'm not particularly concerned with the change, but generally would rather such things not be "hidden" in a larger patch that doesn't really relate. It's easy to miss a small one-line change like this when reviewing a big patch, but the small change may actually require as much thinking/review as the remaining hundreds, when it's something subtle :) (totally OK with small "cleanup" like fixing formatting or other trivial stuff, but at least I dont really know the difference between Netty channel close vs disconnect without having to go spend 10-15 minutes reading Netty docs or source) -- To view, visit http://gerrit.cloudera.org:8080/7250 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0be620629c9a8345ecd5e5679c80ee76ca4eaa57 Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-HasComments: Yes
