Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20180 )
Change subject: [subprocess] KUDU-3489: Support reading large messages through pipes ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/20180/2/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java: http://gerrit.cloudera.org:8080/#/c/20180/2/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@96 PS2, Line 96: int totalRead = in.read(bytes, 0, size); : while (totalRead < size) { Why not to rewrite this using do { ... } while (...) to logically unify this implementation with the updated implementation of the doReadAndDiscard() method below? http://gerrit.cloudera.org:8080/#/c/20180/2/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@101 PS2, Line 101: } else if (read == 0) { : break; : } >From the documentation of the BufferedInputStream.read() method: -- This method implements the general contract of the corresponding read method of the InputStream class. As an additional convenience, it attempts to read as many bytes as possible by repeatedly invoking the read method of the underlying stream. This iterated read continues until one of the following conditions becomes true: * The specified number of bytes have been read, * The read method of the underlying stream returns -1, indicating end-of-file, or * The available method of the underlying stream returns zero, indicating that further input requests would block. -- Could it happen that the call at line 96 or 98 read out some non-empty chunk of data up to the 'would block' condition on the underlying stream, while not all the requested data has been actually read out? If so, then next cycle the call at line 98 should have returned 0, and there would be an exception raised below instead of calling BufferedInputStream.read() again that would block awaiting for the next chunk of data to be read from the input stream? http://gerrit.cloudera.org:8080/#/c/20180/2/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@131 PS2, Line 131: } else if (read == 0) { : break; ditto -- To view, visit http://gerrit.cloudera.org:8080/20180 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6523fdaaca19ee089dbac52a7dedec8847926a6c Gerrit-Change-Number: 20180 Gerrit-PatchSet: 2 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 11 Jul 2023 21:46:38 +0000 Gerrit-HasComments: Yes
