Abhishek Chennaka 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 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/20180/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20180/1//COMMIT_MSG@15 PS1, Line 15: excpetion > exception Done http://gerrit.cloudera.org:8080/#/c/20180/1/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/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@96 PS1, Line 96: byte[] b = new byte[size] > Instead of allocating an extra memory buffer, is it possible to use the 'of Yes, that makes sense. For some reason I thought it was an offset from which we read data from, in the pipe. http://gerrit.cloudera.org:8080/#/c/20180/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@140 PS1, Line 140: if (rem > 4096) { : throw new IOException( : String.format("unable to read next chunk of oversized message (%d bytes), " + : "expected %d bytes but read %d bytes", size, size, size - rem)); : } > What if that was less that 4096? Shouldn't it be an exceptional situation Yes, true. It should be 0. The same issue is seen here as well, if we don't "read" the expected size ("toRead") of the data while discarding the oversized message. I just modified the loop to keep reading until we cannot read anymore and then checking if we have read the expected amount of data. http://gerrit.cloudera.org:8080/#/c/20180/1/src/kudu/subprocess/subprocess_server-test.cc File src/kudu/subprocess/subprocess_server-test.cc: http://gerrit.cloudera.org:8080/#/c/20180/1/src/kudu/subprocess/subprocess_server-test.cc@426 PS1, Line 426: > nit: remove the extra empty line Done http://gerrit.cloudera.org:8080/#/c/20180/1/src/kudu/subprocess/subprocess_server-test.cc@432 PS1, Line 432: the current limit > Does it make sense to set FLAGS_subprocess_max_message_size_bytes to 2x (or Done. -- 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: 1 Gerrit-Owner: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 11 Jul 2023 00:04:40 +0000 Gerrit-HasComments: Yes
