David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8921 )
Change subject: KUDU-2249 Prevent race condition between getSplits() method and TableRecordReader ...................................................................... Patch Set 3: (3 comments) thanks for addressing the comments. couple of nits and should be gtg http://gerrit.cloudera.org:8080/#/c/8921/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8921/3//COMMIT_MSG@7 PS3, Line 7: KUDU-2249 Prevent race condition between getSplits() method and TableRecordReader nits: - try to keep column widths under 80 lines (not mandatory but possible here for the most part) - maybe a better title for this patch would be: "Avoid sharing the client between the InputFormat and RecordReader" (or something close to this) and the explain in the commit message that this is meant to avoid a race condition. http://gerrit.cloudera.org:8080/#/c/8921/3//COMMIT_MSG@12 PS3, Line 12: Kud typo http://gerrit.cloudera.org:8080/#/c/8921/3/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java File java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java: http://gerrit.cloudera.org:8080/#/c/8921/3/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java@268 PS3, Line 268: getKuduClient rename to newKuduClient() or buildKuduClient() so that this doesn't read like a plain getter -- To view, visit http://gerrit.cloudera.org:8080/8921 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I24f45ee9253790c5348cabd0afe6c6a4b6d3f3d4 Gerrit-Change-Number: 8921 Gerrit-PatchSet: 3 Gerrit-Owner: Clemens Valiente <clemens.valie...@gmail.com> Gerrit-Reviewer: Clemens Valiente <clemens.valie...@gmail.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Thu, 04 Jan 2018 18:52:17 +0000 Gerrit-HasComments: Yes
