You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Clemens Valiente (Code Review)" <ge...@cloudera.org> on 2017/12/28 09:35:34 UTC
[kudu-CR] KUDU-2249 give the TableRecordReader their own KuduClient to use.
Clemens Valiente has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8921
Change subject: KUDU-2249 give the TableRecordReader their own KuduClient to use.
......................................................................
KUDU-2249 give the TableRecordReader their own KuduClient to use.
Change-Id: I24f45ee9253790c5348cabd0afe6c6a4b6d3f3d4
---
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
1 file changed, 27 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/8921/1
--
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: newchange
Gerrit-Change-Id: I24f45ee9253790c5348cabd0afe6c6a4b6d3f3d4
Gerrit-Change-Number: 8921
Gerrit-PatchSet: 1
Gerrit-Owner: Clemens Valiente <cl...@gmail.com>
[kudu-CR] KUDU-2249 give the TableRecordReader their own KuduClient to use.
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8921 )
Change subject: KUDU-2249 give the TableRecordReader their own KuduClient to use.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8921/1/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/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java@253
PS1, Line 253: private KuduClient getKuduClient() {
:
: String masterAddresses = conf.get(MASTER_ADDRESSES_KEY);
: this.operationTimeoutMs = conf.getLong(OPERATION_TIMEOUT_MS_KEY,
: AsyncKuduClient.DEFAULT_OPERATION_TIMEOUT_MS);
: KuduClient kuduClient = new KuduClient.KuduClientBuilder(masterAddresses)
: .defaultOperationTimeoutMs(operationTimeoutMs)
: .build();
: KuduTableMapReduceUtil.importCredentialsFromCurrentSubject(kuduClient);
: return kuduClient;
: }
> Hi David,
Thanks for pointing out the discussion.
I'd still like to, at the very least, have a comment, on the commit message and/or on the code explaining: i) the rationale, in terms of how many new clients we can expect; ii) how this is not going to cause 10s or 100s of new clients to spawn, increasing the load on the master iii) or if it is, how there is no way to avoid it.
--
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: 1
Gerrit-Owner: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 04 Jan 2018 04:16:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2249 Avoid sharing the client between the InputFormat and RecordReader
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8921 )
Change subject: KUDU-2249 Avoid sharing the client between the InputFormat and RecordReader
......................................................................
KUDU-2249 Avoid sharing the client between the InputFormat and RecordReader
This commit prevents a possible race condition between getSplits() method and
TableRecordReader in the KuduTableInputFormat, when both try to access and
shutdown the KuduClient.
Both are sharing the same client and shut it down after use. In some scenarios
the client might still be accessed after that and throwing an error.
So the TableRecordReader gets its own client with this commit. This increases
the number of opened Kudu clients by a MR application at most by one (The one
that was shared by getSplits() with a TableRecordReader)
Also clarified the behaviour of MR applications and how many open Kudu clients
one might have to expect in total.
Change-Id: I24f45ee9253790c5348cabd0afe6c6a4b6d3f3d4
Reviewed-on: http://gerrit.cloudera.org:8080/8921
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves <da...@gmail.com>
---
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
1 file changed, 45 insertions(+), 21 deletions(-)
Approvals:
Kudu Jenkins: Verified
David Ribeiro Alves: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: I24f45ee9253790c5348cabd0afe6c6a4b6d3f3d4
Gerrit-Change-Number: 8921
Gerrit-PatchSet: 6
Gerrit-Owner: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2249 Avoid sharing the client between the InputFormat and RecordReader
Posted by "Clemens Valiente (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8921
to look at the new patch set (#4).
Change subject: KUDU-2249 Avoid sharing the client between the InputFormat and RecordReader
......................................................................
KUDU-2249 Avoid sharing the client between the InputFormat and RecordReader
This commit prevents a possible race condition between getSplits() method and
TableRecordReader in the KuduTableInputFormat
Both are sharing the same client and shut it down after use. In some scenarios
the client might still be accessed after that and throwing an error.
So the TableRecordReader gets its own client with this commit. This increases
the number of opened Kudu clients by a MR application at most by one (The one
that was shared by getSplits() with a TableRecordReader)
Also clarified the behaviour of MR applications and how many open Kudu clients
one might have to expect in total.
Change-Id: I24f45ee9253790c5348cabd0afe6c6a4b6d3f3d4
---
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
1 file changed, 45 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/8921/4
--
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: newpatchset
Gerrit-Change-Id: I24f45ee9253790c5348cabd0afe6c6a4b6d3f3d4
Gerrit-Change-Number: 8921
Gerrit-PatchSet: 4
Gerrit-Owner: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2249 Prevent race condition between getSplits() method and TableRecordReader
Posted by "Clemens Valiente (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8921
to look at the new patch set (#2).
Change subject: KUDU-2249 Prevent race condition between getSplits() method and TableRecordReader
......................................................................
KUDU-2249 Prevent race condition between getSplits() method and TableRecordReader
Both are sharing the same client and shut it down after use. In some scenarios the client might
still be accessed after that and throwing an error.
So the TableRecordReader gets its own client with this commit.
Also clarified the behaviour of MR applications and how many open Kudu clients
one might have to expect.
Change-Id: I24f45ee9253790c5348cabd0afe6c6a4b6d3f3d4
---
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
1 file changed, 45 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/8921/2
--
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: newpatchset
Gerrit-Change-Id: I24f45ee9253790c5348cabd0afe6c6a4b6d3f3d4
Gerrit-Change-Number: 8921
Gerrit-PatchSet: 2
Gerrit-Owner: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2249 Avoid sharing the client between the InputFormat and RecordReader
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8921 )
Change subject: KUDU-2249 Avoid sharing the client between the InputFormat and RecordReader
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8921/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8921/4//COMMIT_MSG@9
PS4, Line 9: This commit prevents a possible race condition between getSplits() method and
: TableRecordReader in the KuduTableInputFormat
missing period, also maybe finish with: ".. in the KuduTableInputFormat that happens when both try and shutdown the client."
http://gerrit.cloudera.org:8080/#/c/8921/4//COMMIT_MSG@12
PS4, Line 12: Both are sharing the same client and shut it down after use. In some scenarios
: the client might still be accessed after that and throwing an error.
: So the TableRecordReader gets its own client with this commit. This increases
: the number of opened Kudu clients by a MR application at most by one (The one
: that was shared by getSplits() with a TableRecordReader)
: Also clarified the behaviour of MR applications and how many open Kudu clients
: one might have to expect in total.
I my previous comment I meant to keep this under 80 cols, not 80 lines, my bad. It makes it much easier to browse commit messages on a small terminal window
--
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: 4
Gerrit-Owner: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 05 Jan 2018 20:36:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2249 give the TableRecordReader their own KuduClient to use.
Posted by "Clemens Valiente (Code Review)" <ge...@cloudera.org>.
Clemens Valiente has posted comments on this change. ( http://gerrit.cloudera.org:8080/8921 )
Change subject: KUDU-2249 give the TableRecordReader their own KuduClient to use.
......................................................................
Patch Set 1:
(1 comment)
Hi David,
I discussed this on the dev mailing list here:
http://mail-archives.apache.org/mod_mbox/kudu-dev/201712.mbox/%3CAM0PR0502MB405140DD23048A17522BB55C9C070%40AM0PR0502MB4051.eurprd05.prod.outlook.com%3E
The problem is the getInputSplit() closing the client that the Reader want to use.
There's no real clean way of sharing the client and closing it properly. Due to the mapreduce architecture, each tablet usually will read from a separate Map Container and thus need its own client anyway. This just fixes a bug in the rare scenario that the getinputsplit and record reader are executed in one container.
http://gerrit.cloudera.org:8080/#/c/8921/1/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/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java@253
PS1, Line 253: private KuduClient getKuduClient() {
:
: String masterAddresses = conf.get(MASTER_ADDRESSES_KEY);
: this.operationTimeoutMs = conf.getLong(OPERATION_TIMEOUT_MS_KEY,
: AsyncKuduClient.DEFAULT_OPERATION_TIMEOUT_MS);
: KuduClient kuduClient = new KuduClient.KuduClientBuilder(masterAddresses)
: .defaultOperationTimeoutMs(operationTimeoutMs)
: .build();
: KuduTableMapReduceUtil.importCredentialsFromCurrentSubject(kuduClient);
: return kuduClient;
: }
> Kudu clients cache state internally, like tablet locations. Changing this t
Hi David,
I discussed this on the dev mailing list here:
http://mail-archives.apache.org/mod_mbox/kudu-dev/201712.mbox/%3CAM0PR0502MB405140DD23048A17522BB55C9C070%40AM0PR0502MB4051.eurprd05.prod.outlook.com%3E
The problem is the getInputSplit() closing the client that the Reader want to use.
There's no real clean way of sharing the client and closing it properly. Due to the mapreduce architecture, each tablet usually will read from a separate Map Container and thus need its own client anyway. This just fixes a bug in the rare scenario that the getinputsplit and record reader are executed in one container.
--
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: 1
Gerrit-Owner: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 03 Jan 2018 08:39:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2249 Avoid sharing the client between the InputFormat and RecordReader
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8921 )
Change subject: KUDU-2249 Avoid sharing the client between the InputFormat and RecordReader
......................................................................
Patch Set 5: Code-Review+2
Ah, my bad, gerrit's column mark at 76 chars tricked me.
Thanks for addressing my nits.
--
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: 5
Gerrit-Owner: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 08 Jan 2018 18:57:07 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2249 Avoid sharing the client between the InputFormat and RecordReader
Posted by "Clemens Valiente (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8921
to look at the new patch set (#5).
Change subject: KUDU-2249 Avoid sharing the client between the InputFormat and RecordReader
......................................................................
KUDU-2249 Avoid sharing the client between the InputFormat and RecordReader
This commit prevents a possible race condition between getSplits() method and
TableRecordReader in the KuduTableInputFormat, when both try to access and
shutdown the KuduClient.
Both are sharing the same client and shut it down after use. In some scenarios
the client might still be accessed after that and throwing an error.
So the TableRecordReader gets its own client with this commit. This increases
the number of opened Kudu clients by a MR application at most by one (The one
that was shared by getSplits() with a TableRecordReader)
Also clarified the behaviour of MR applications and how many open Kudu clients
one might have to expect in total.
Change-Id: I24f45ee9253790c5348cabd0afe6c6a4b6d3f3d4
---
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
1 file changed, 45 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/8921/5
--
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: newpatchset
Gerrit-Change-Id: I24f45ee9253790c5348cabd0afe6c6a4b6d3f3d4
Gerrit-Change-Number: 8921
Gerrit-PatchSet: 5
Gerrit-Owner: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2249 Avoid sharing the client between the InputFormat and RecordReader
Posted by "Clemens Valiente (Code Review)" <ge...@cloudera.org>.
Clemens Valiente has posted comments on this change. ( http://gerrit.cloudera.org:8080/8921 )
Change subject: KUDU-2249 Avoid sharing the client between the InputFormat and RecordReader
......................................................................
Patch Set 4:
> Patch Set 4:
>
> (2 comments)
I added the clarification.
The lines should below 80 cols already if I am not mistaken?
--
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: 4
Gerrit-Owner: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 08 Jan 2018 10:39:59 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2249 give the TableRecordReader their own KuduClient to use.
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8921 )
Change subject: KUDU-2249 give the TableRecordReader their own KuduClient to use.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8921/1/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/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java@253
PS1, Line 253: private KuduClient getKuduClient() {
:
: String masterAddresses = conf.get(MASTER_ADDRESSES_KEY);
: this.operationTimeoutMs = conf.getLong(OPERATION_TIMEOUT_MS_KEY,
: AsyncKuduClient.DEFAULT_OPERATION_TIMEOUT_MS);
: KuduClient kuduClient = new KuduClient.KuduClientBuilder(masterAddresses)
: .defaultOperationTimeoutMs(operationTimeoutMs)
: .build();
: KuduTableMapReduceUtil.importCredentialsFromCurrentSubject(kuduClient);
: return kuduClient;
: }
Kudu clients cache state internally, like tablet locations. Changing this to creating a new client for each TabletRecordReader makes it so that we send a bunch more requests to the KuduMaster. Is there a way to pass the client around and still make sure we close it appropriately?
--
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: 1
Gerrit-Owner: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 02 Jan 2018 22:25:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2249 Prevent race condition between getSplits() method and TableRecordReader
Posted by "Clemens Valiente (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8921
to look at the new patch set (#3).
Change subject: KUDU-2249 Prevent race condition between getSplits() method and TableRecordReader
......................................................................
KUDU-2249 Prevent race condition between getSplits() method and TableRecordReader
Both are sharing the same client and shut it down after use. In some scenarios the client might
still be accessed after that and throwing an error.
So the TableRecordReader gets its own client with this commit. This increases the number of
opened Kud clients by a MR application at most by one (The one that was shared by getSplits()
with a TableRecordReader)
Also clarified the behaviour of MR applications and how many open Kudu clients
one might have to expect in total.
Change-Id: I24f45ee9253790c5348cabd0afe6c6a4b6d3f3d4
---
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
1 file changed, 45 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/8921/3
--
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: newpatchset
Gerrit-Change-Id: I24f45ee9253790c5348cabd0afe6c6a4b6d3f3d4
Gerrit-Change-Number: 8921
Gerrit-PatchSet: 3
Gerrit-Owner: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2249 Prevent race condition between getSplits() method and TableRecordReader
Posted by "Clemens Valiente (Code Review)" <ge...@cloudera.org>.
Clemens Valiente 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 2:
> Patch Set 1:
>
> (1 comment)
Hi David, I clarified the behaviour of MR applications and number of open clients in the class documentation and motivation for the changes in the commit message.
--
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: 2
Gerrit-Owner: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 04 Jan 2018 09:41:51 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2249 Prevent race condition between getSplits() method and TableRecordReader
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
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 <cl...@gmail.com>
Gerrit-Reviewer: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 04 Jan 2018 18:52:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2249 Avoid sharing the client between the InputFormat and RecordReader
Posted by "Clemens Valiente (Code Review)" <ge...@cloudera.org>.
Clemens Valiente has posted comments on this change. ( http://gerrit.cloudera.org:8080/8921 )
Change subject: KUDU-2249 Avoid sharing the client between the InputFormat and RecordReader
......................................................................
Patch Set 4:
All done, thanks for the review!
--
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: 4
Gerrit-Owner: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: Clemens Valiente <cl...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 05 Jan 2018 10:10:22 +0000
Gerrit-HasComments: No