Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/14329 )
Change subject: [java] KUDU-2791: process communicates via protobuf-based protocol ...................................................................... Patch Set 5: (8 comments) http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java: http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@89 PS5, Line 89: if (in.available() <= 0) { can this be negative? anyway, we should return "" instead of new String() so that it can be cached. http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@125 PS5, Line 125: out.write(Bytes.concat(size, body)); : // Always do a flush after write to ensure no partial message is written. : out.flush(); these two statements should be synchronized on out. http://gerrit.cloudera.org:8080/#/c/14329/5/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/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@60 PS5, Line 60: true this should be while (!Thread.currentThread().isInterrupted()). Right now there's no way to actually interrupt the thread as interrupted will be set to true, but we never get to finally. http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@94 PS5, Line 94: LOG.debug("Message: {} has been put on the queue", data); this should be in an if (LOG.isDebugEnabled()) block so we don't format a string with potentially large data with needlessly when debug is turned off. http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@96 PS5, Line 96: } catch (InterruptedException e) { why are we retrying when the thread is interrupted? Shouldn't we simply interrupt and immediately return? http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java: http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java@63 PS5, Line 63: while (true) { this should be while (!Thread.currentThread().isInterrupted()) as well, simply run Thread.currentThread().interrupt() in the catch block instead of setting the interrupted flag and then there's no need for the finally block. http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java@69 PS5, Line 69: LOG.debug("Message: {} has been taken from the queue", data); if (LOG.isDebugEnabled()) http://gerrit.cloudera.org:8080/#/c/14329/5/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessMain.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessMain.java: PS5: we should register a shutdown hook that shuts down the threadpools (shutdownNow() to make sure the threads are interrupted) -- 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: 5 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: Sat, 14 Dec 2019 02:08:17 +0000 Gerrit-HasComments: Yes
