Adar Dembo 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 2: (2 comments) Just passing through, will wait for an update to actually review. http://gerrit.cloudera.org:8080/#/c/14329/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14329/1//COMMIT_MSG@15 PS1, Line 15: optional JSON encoding) from > I agree I don't see a strong need for it. We could remove all the configura Agreed with Andrew and Grant. The "subprocess protocol" module must support both JSON and PB, but that doesn't mean this new code needs to support both. We can get good debuggability using the usual methods (i.e. VLOG(1) << SecureDebugString(...)). http://gerrit.cloudera.org:8080/#/c/14329/1/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/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessMain.java@55 PS1, Line 55: static void run(SubprocessConfiguration conf) { Could be private? Or if this visibility is only for testing, annotate with @InterfaceAudience.LimitedPrivate("Test") -- 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: 2 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: Wed, 23 Oct 2019 04:28:25 +0000 Gerrit-HasComments: Yes
