Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14329 )
Change subject: [java] KUDU-2971: process communicates via protobuf-based protocol ...................................................................... Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/14329/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14329/7//COMMIT_MSG@18 PS7, Line 18: from the queue, process them and write the responses to the standard : output. : > Makes sense. I am thinking to address it in a follow up patch as this one i I'm OK with it as long as the follow-up doesn't end up rewriting a large chunk of the code in this patch. If it does, I'd rather the follow-up be part of this patch so as to minimize overall churn and simplify the review. http://gerrit.cloudera.org:8080/#/c/14329/7/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java: http://gerrit.cloudera.org:8080/#/c/14329/7/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@84 PS7, Line 84: // InterruptedException during the put, record the interruption > In this particular case, I don't think we would want to cancel the read/wri What does it mean to "get interrupted"? I'll admit it's been a while since I wrote systems-level Java code, but my understanding is that it means, for example, receiving a UNIX signal (a la Ctrl-C). That's not a recoverable condition; that's a condition wherein the process is expected to die. Another example: someone calls interrupt() on a single thread with the intent to have it die. Are there situations where InterruptedException is thrown for run-of-the-mill normal events that a process is expected to actually handle? Or is it only used in cases where we need to shutdown but we want to ensure a more orderly shutdown? http://gerrit.cloudera.org:8080/#/c/14329/7/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@98 PS7, Line 98: } > Hmm, not quite following your question, here we retry put() for certain tim Won't the thread's interrupted status still be true, thus breaking out of the while loop on :59? http://gerrit.cloudera.org:8080/#/c/14329/7/java/kudu-subprocess/src/main/resources/log4j2.properties File java/kudu-subprocess/src/main/resources/log4j2.properties: PS7: > I think that is to specify the log level for org.apache.kudu package specif Hmm, why do we provide a log4j2.properties file here at all? AFAICT in every other instance it's only for tests. -- To view, visit http://gerrit.cloudera.org:8080/14329 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaf9ad24dbc9acc681284b6433836271b5b4c7982 Gerrit-Change-Number: 14329 Gerrit-PatchSet: 8 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 14 Jan 2020 06:13:22 +0000 Gerrit-HasComments: Yes
