Hao Hao 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. : > I'm OK with it as long as the follow-up doesn't end up rewriting a large ch Hmm, I don't see it will require rewriting a large chunk of the code in this patch. As far as I see, it will be mainly touching/breaking down the MessageWriter class. But let me know if you think otherwise. 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 > What does it mean to "get interrupted"? I'll admit it's been a while since My understanding of InterruptedException is to provide the blocking method a way to cancel the activity, and stop blocking early. But not all blocking methods are required to pay attention to interruption, e.g. blockingQueue.put() can get interrupted by another thread when the queue is full and it is waiting to place an element to the queue. I don't see a strong reason to cancel the task for such case here. However, at L59 we are constantly checking if interrupted status is set (may caused by your example "someone calls interrupt() on a single thread with the intent to have it die'"), then the while loop will break and the task will be cancelled. I also found this article helpful on understanding InterruptedException and how to handle it https://www.ibm.com/developerworks/library/j-jtp05236/. http://gerrit.cloudera.org:8080/#/c/14329/7/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@98 PS7, Line 98: } > Won't the thread's interrupted status still be true, thus breaking out of t Hmm, no, Thread.currentThread().isInterrupted() is actually checking the internal interrupted status instead of the one defined locally at L57. 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: > Hmm, why do we provide a log4j2.properties file here at all? AFAICT in ever I was thinking to use it as the default log config for the subprocess. Although we may need to add a file appender for production usage. Maybe I can move it so that it is only used for tests for now? -- 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 <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 14 Jan 2020 07:11:45 +0000 Gerrit-HasComments: Yes
