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: (4 comments) > Patch Set 5: > > (8 comments) Sorry, missed your replies. Went back and expanded on them. http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageConsumer.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageConsumer.java: http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageConsumer.java@70 PS4, Line 70: > As mentioned below, we would want to retry with InterruptedException for ta hm... there's no way to gracefully shut down in this case though. If we don't want to interrupt the thread at all, why do we need the whole interrupt logic at all? http://gerrit.cloudera.org:8080/#/c/14329/4/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/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@148 PS4, Line 148: */ > The write is synchronized in BufferedOutputStream. even if BufferedOutputStream synchronizes, write and flush, I think it should still be in the same synchronization block. Also, if we ever replace BufferedOutputStream with another OutputStream it would be an issue and even if the methods are synchronized on BufferedOutputStream, it doesn't say anything about thread safety in its contract so theoretically there could be a JVM implementation where this is not the case. http://gerrit.cloudera.org:8080/#/c/14329/4/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: http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessMain.java@80 PS4, Line 80: > I don't see why it is necessary to have separate consumer instance for each yeah I think you're right and it's not needed. http://gerrit.cloudera.org:8080/#/c/14329/4/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessMain.java@88 PS4, Line 88: consumerFuture.exceptionally(errorHandler); > I agree this is not needed, and as in each CompletableFuture has error hand yeah we can skip shutdowns here, but maybe add them in a shutdown hook? -- 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:25:05 +0000 Gerrit-HasComments: Yes
