[kudu-CR] KUDU-2167: fix C++ client crash due to bad assumption regarding scan data
Hello David Ribeiro Alves, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/8204 to review the following change. Change subject: KUDU-2167: fix C++ client crash due to bad assumption regarding scan data .. KUDU-2167: fix C++ client crash due to bad assumption regarding scan data The new unit test triggered the crash reliably, though it's probably not the only way (or perhaps not even the best way) to trigger it. Change-Id: If1a8a4e22082cf39710b9f00894f644a0b34234e --- M src/kudu/client/client-test.cc M src/kudu/client/scanner-internal.cc 2 files changed, 46 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8204/1 -- To view, visit http://gerrit.cloudera.org:8080/8204 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: If1a8a4e22082cf39710b9f00894f644a0b34234e Gerrit-Change-Number: 8204 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Todd Lipcon
[kudu-CR] block compression: fix arg to Substitute
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8203 ) Change subject: block_compression: fix arg to Substitute .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8203 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If00035e39ba3f837f42e398ca6aa31bbc9afb3a9 Gerrit-Change-Number: 8203 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 04 Oct 2017 01:26:17 + Gerrit-HasComments: No
[kudu-CR] block compression: fix arg to Substitute
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8203 ) Change subject: block_compression: fix arg to Substitute .. block_compression: fix arg to Substitute Change-Id: If00035e39ba3f837f42e398ca6aa31bbc9afb3a9 Reviewed-on: http://gerrit.cloudera.org:8080/8203 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M src/kudu/cfile/block_compression.cc 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Kudu Jenkins: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/8203 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: If00035e39ba3f837f42e398ca6aa31bbc9afb3a9 Gerrit-Change-Number: 8203 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] block compression: fix arg to Substitute
Adar Dembo has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8203 Change subject: block_compression: fix arg to Substitute .. block_compression: fix arg to Substitute Change-Id: If00035e39ba3f837f42e398ca6aa31bbc9afb3a9 --- M src/kudu/cfile/block_compression.cc 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/03/8203/1 -- To view, visit http://gerrit.cloudera.org:8080/8203 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: If00035e39ba3f837f42e398ca6aa31bbc9afb3a9 Gerrit-Change-Number: 8203 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo
[kudu-CR](branch-1.4.x) KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/8185 ) Change subject: KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies .. Patch Set 2: nvm the backport-- I forgot it's test-only -- To view, visit http://gerrit.cloudera.org:8080/8185 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.4.x Gerrit-MessageType: comment Gerrit-Change-Id: I6d58abbe6a5d9fc524e54e4182402c5326a60d82 Gerrit-Change-Number: 8185 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 04 Oct 2017 00:01:53 + Gerrit-HasComments: No
[kudu-CR] [webui] Allow custom response codes and headers
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/8141 )
Change subject: [webui] Allow custom response codes and headers
..
Patch Set 8:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.h
File src/kudu/server/webserver.h:
http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.h@66
PS6, Line 66: // Register a route 'path' to be rendered via template.
> Done
What about 'alias'?
http://gerrit.cloudera.org:8080/#/c/8141/8/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/8141/8/src/kudu/server/webserver.cc@472
PS8, Line 472: std::set invalid_headers{"Content-Type",
"Content-Length", "X-Frame-Options"};
Can use unordered_set which is hash based and slightly faster on average.
http://gerrit.cloudera.org:8080/#/c/8141/8/src/kudu/server/webserver.cc@475
PS8, Line 475: if (ContainsKey(invalid_headers, entry.first)) continue;
Hmm, this seems too quiet: if a callback tries to send an invalid header, it'll
just be silently ignored. I think it'd be better to crash, or to go in the
other direction and allow callbacks to override those headers.
--
To view, visit http://gerrit.cloudera.org:8080/8141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9ff890785eeb2df3eed9e7c54d0daf760c8b3924
Gerrit-Change-Number: 8141
Gerrit-PatchSet: 8
Gerrit-Owner: Will Berkeley
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Tue, 03 Oct 2017 23:50:11 +
Gerrit-HasComments: Yes
[kudu-CR] tool: add cluster shell action
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/7853 ) Change subject: tool: add cluster shell action .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc File src/kudu/tools/tool_action_common.cc: http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@735 PS8, Line 735: CHECK_EQ(buf.length(), r); > I agree that in general short reads are possible, but I'm not sure that's p Yep, as I found, short reads are possible for pipes, indeed (shame on me: I forgot my previous experience and mis-sread the man page). However, for writing into pipes, I'm not sure it's the case. >From the other side, if it's possible to implement the write path to work even >with short writes as well (regardless whether it's possible or not for pipes), >that might be the best. Please forgive my bikeshedding :) -- To view, visit http://gerrit.cloudera.org:8080/7853 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e693921ef780dc4a06e536c6b7408f7f0b252f6 Gerrit-Change-Number: 7853 Gerrit-PatchSet: 8 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 03 Oct 2017 22:55:34 + Gerrit-HasComments: Yes
[kudu-CR] [webui] Allow custom response codes and headers
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8141 to look at the new patch set (#8). Change subject: [webui] Allow custom response codes and headers .. [webui] Allow custom response codes and headers Previously, the path handlers used to implement pages in the web ui could only return 200 OK and could not set any headers, as these two aspects of the HTTP response were handled in the underlying webserver code. This patch introduces WebResponse and PrerenderedWebResponse structs that wrap and replace the 'output' EasyJson and ostringstream pointers, respectively, used before, and which have fields for the response code and additional headers. The ability to add headers isn't currently used, but it's nice to have. The response codes are adjusted where necessary to match what one would expect, e.g. navigating to /tablet?id=foo returns 404. Change-Id: I9ff890785eeb2df3eed9e7c54d0daf760c8b3924 --- M src/kudu/master/master-path-handlers.cc M src/kudu/master/master-path-handlers.h M src/kudu/server/default-path-handlers.cc M src/kudu/server/pprof-path-handlers.cc M src/kudu/server/rpcz-path-handler.cc M src/kudu/server/tracing-path-handlers.cc M src/kudu/server/webserver.cc M src/kudu/server/webserver.h M src/kudu/tserver/tserver-path-handlers.cc M src/kudu/tserver/tserver-path-handlers.h M src/kudu/util/thread.cc M src/kudu/util/web_callback_registry.h 12 files changed, 260 insertions(+), 134 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/8141/8 -- To view, visit http://gerrit.cloudera.org:8080/8141 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9ff890785eeb2df3eed9e7c54d0daf760c8b3924 Gerrit-Change-Number: 8141 Gerrit-PatchSet: 8 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] [webui] Allow custom response codes and headers
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8141 to look at the new patch set (#7). Change subject: [webui] Allow custom response codes and headers .. [webui] Allow custom response codes and headers Previously, the path handlers used to implement pages in the web ui could only return 200 OK and could not set any headers, as these two aspects of the HTTP response were handled in the underlying webserver code. This patch introduces WebResponse and PrerenderedWebResponse structs that wrap and replace the 'output' EasyJson and ostringstream pointers, respectively, used before, and which have fields for the response code and additional headers. The ability to add headers isn't currently used, but it's nice to have. The response codes are adjusted where necessary to match what one would expect, e.g. navigating to /tablet?id=foo returns 404. Change-Id: I9ff890785eeb2df3eed9e7c54d0daf760c8b3924 --- M src/kudu/master/master-path-handlers.cc M src/kudu/master/master-path-handlers.h M src/kudu/server/default-path-handlers.cc M src/kudu/server/pprof-path-handlers.cc M src/kudu/server/rpcz-path-handler.cc M src/kudu/server/tracing-path-handlers.cc M src/kudu/server/webserver.cc M src/kudu/server/webserver.h M src/kudu/tserver/tserver-path-handlers.cc M src/kudu/tserver/tserver-path-handlers.h M src/kudu/util/thread.cc M src/kudu/util/web_callback_registry.h 12 files changed, 260 insertions(+), 134 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/8141/7 -- To view, visit http://gerrit.cloudera.org:8080/8141 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9ff890785eeb2df3eed9e7c54d0daf760c8b3924 Gerrit-Change-Number: 8141 Gerrit-PatchSet: 7 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] [webui] Allow custom response codes and headers
Will Berkeley has posted comments on this change. (
http://gerrit.cloudera.org:8080/8141 )
Change subject: [webui] Allow custom response codes and headers
..
Patch Set 6:
(9 comments)
I let Impala people know about this patch.
http://gerrit.cloudera.org:8080/#/c/8141/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8141/6//COMMIT_MSG@14
PS6, Line 14: has
> have ("WebResponse and PrerenderedWebResponse" are plural)
Done
http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:
http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/master/master-path-handlers.cc@242
PS6, Line 242: response
> respond
Done
http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.h
File src/kudu/server/webserver.h:
http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.h@66
PS6, Line 66: // Register a route 'path' to be rendered via template.
> Since you're already updating these docs, could you explain what 'alias', '
Done
http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.cc@88
PS6, Line 88: switch (code) {
> Add a default case that crashes or something? Would be good to know if we'v
Done, but I added outside the switch so the compiler will generate warnings for
unhandled enum class members.
http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.cc@452
PS6, Line 452: HttpResponseHeaders{}
> Just {} doesn't work?
Done
http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.cc@469
PS6, Line 469: for (const auto& entry : resp.response_headers) {
> Should we enforce that response_headers doesn't include Content-Type, Conte
Yeah, I think we should check for it. A repeated header is only allowed if the
header value is CSV type, and none of those are.
http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.cc@486
PS6, Line 486: HttpResponseHeaders{}
> Just {} doesn't work?
Done
http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/util/web_callback_registry.h
File src/kudu/util/web_callback_registry.h:
http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/util/web_callback_registry.h@66
PS6, Line 66: typedef std::map HttpResponseHeaders;
> Is it important that the headers be sorted by key? Or can this be an unorde
Done
http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/util/web_callback_registry.h@69
PS6, Line 69: // - 'status_code' determines the status code of the HTTP
response.
: // - 'response_headers' are additional headers added to the
HTTP response.
: // - 'output' is a JSON object to be rendered in a mustache
template.
> Nit: move each of these so that it's just above the declaration of its fiel
Done
--
To view, visit http://gerrit.cloudera.org:8080/8141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9ff890785eeb2df3eed9e7c54d0daf760c8b3924
Gerrit-Change-Number: 8141
Gerrit-PatchSet: 6
Gerrit-Owner: Will Berkeley
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Tue, 03 Oct 2017 22:38:23 +
Gerrit-HasComments: Yes
[kudu-CR](branch-1.4.x) KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/8185 ) Change subject: KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies .. Patch Set 2: https://issues.apache.org/jira/browse/KUDU-2119 -- To view, visit http://gerrit.cloudera.org:8080/8185 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.4.x Gerrit-MessageType: comment Gerrit-Change-Id: I6d58abbe6a5d9fc524e54e4182402c5326a60d82 Gerrit-Change-Number: 8185 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 03 Oct 2017 22:35:16 + Gerrit-HasComments: No
[kudu-CR](branch-1.4.x) KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies
Jean-Daniel Cryans has posted comments on this change. ( http://gerrit.cloudera.org:8080/8185 ) Change subject: KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies .. Patch Set 2: > The latest Jenkins failure is a known issue fixed on master. Maybe > we should backport it? Mind pointing to it? -- To view, visit http://gerrit.cloudera.org:8080/8185 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.4.x Gerrit-MessageType: comment Gerrit-Change-Id: I6d58abbe6a5d9fc524e54e4182402c5326a60d82 Gerrit-Change-Number: 8185 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 03 Oct 2017 22:32:08 + Gerrit-HasComments: No
[kudu-CR](branch-1.4.x) KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/8185 ) Change subject: KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies .. Patch Set 2: Verified+1 The latest Jenkins failure is a known issue fixed on master. Maybe we should backport it? -- To view, visit http://gerrit.cloudera.org:8080/8185 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.4.x Gerrit-MessageType: comment Gerrit-Change-Id: I6d58abbe6a5d9fc524e54e4182402c5326a60d82 Gerrit-Change-Number: 8185 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 03 Oct 2017 22:29:17 + Gerrit-HasComments: No
[kudu-CR](branch-1.4.x) KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies
Will Berkeley has removed a vote on this change. Change subject: KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/8185 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.4.x Gerrit-MessageType: deleteVote Gerrit-Change-Id: I6d58abbe6a5d9fc524e54e4182402c5326a60d82 Gerrit-Change-Number: 8185 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] tool: add cluster shell action
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/7853 ) Change subject: tool: add cluster shell action .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc File src/kudu/tools/tool_action_common.cc: http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@718 PS8, Line 718: CHECK_EQ(buf->length(), r); > I'm not sure that's possible for pipes. From 'man 7 pipe': Ah, I played with that and I can see you are right: it's possible for pipes as well. For some reason, I missread the man page. Sorry. -- To view, visit http://gerrit.cloudera.org:8080/7853 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e693921ef780dc4a06e536c6b7408f7f0b252f6 Gerrit-Change-Number: 7853 Gerrit-PatchSet: 8 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 03 Oct 2017 22:24:31 + Gerrit-HasComments: Yes
[kudu-CR](branch-1.4.x) KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/8185 ) Change subject: KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies .. Patch Set 2: I'm unable to repro these failures. -- To view, visit http://gerrit.cloudera.org:8080/8185 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.4.x Gerrit-MessageType: comment Gerrit-Change-Id: I6d58abbe6a5d9fc524e54e4182402c5326a60d82 Gerrit-Change-Number: 8185 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 03 Oct 2017 22:06:39 + Gerrit-HasComments: No
[kudu-CR] tool: add cluster shell action
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/7853 )
Change subject: tool: add cluster shell action
..
Patch Set 8:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@685
PS8, Line 685: buf.append("\n");
> It's necessary; the format is newline delimited JSON messages.
Ah, I see. For some reason I thought the serialized JSON is also preceded with
length of the block, but it seems it's not and '\n' is used as a delimiter.
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@718
PS8, Line 718: CHECK_EQ(buf->length(), r);
> Correct, the issue is a short read, not a zero read.
I'm not sure that's possible for pipes. From 'man 7 pipe':
If a process attempts to read from an empty pipe, then read(2) will
block until data is available. If a process attempts to write to a
full pipe (see below), then write(2) blocks until sufficient data has
been read from the pipe to allow the write to complete.
Do you have an example where short reads are visible for pipes? Just curious.
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@735
PS8, Line 735: CHECK_EQ(buf.length(), r);
> Short writes are also possible.
I agree that in general short reads are possible, but I'm not sure that's
possible for pipes. As for writes, I don't think short writes are possible at
all (unless there is an error, interrupt by signal handler, or the descriptor
is set to work in non-blocking mode).
Could you point to some reference/example where it's clear to see that 'short'
write is possible in blocking mode?
I found some info at http://www.linux-mag.com/id/308/ (which is Linux specific
and might be obsolete now), but even from there I could not see write might
return earlier in non-blocking mode.
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc
File src/kudu/tools/tool_action_test.cc:
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@346
PS8, Line 346: ActionBuilder("shell", &RunControlShell)
> I'm in favor of mini_cluster because it's discoverable, self-describing, an
I also think 'kudu test mini_cluster' is a good option. I'm not advocating for
'control' over 'mini_cluster', just throwing in some more options to choose
from.
--
To view, visit http://gerrit.cloudera.org:8080/7853
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e693921ef780dc4a06e536c6b7408f7f0b252f6
Gerrit-Change-Number: 7853
Gerrit-PatchSet: 8
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 03 Oct 2017 22:04:47 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1125: issue one catalog write per tablet report
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8090 ) Change subject: KUDU-1125: issue one catalog write per tablet report .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc@3227 PS6, Line 3227: bootstrapping > I thought bootstrapping and copying were synonymous? We used to have a "remote bootstrap" process and we renamed it to "tablet copy" to differentiate it from "tablet bootstrap", the process of WAL replay followed by tablet startup. The latter still bears the name "bootstrap". -- To view, visit http://gerrit.cloudera.org:8080/8090 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie6f5cf0e4b1cd1160b3b310d89c6dbf3dd62e43b Gerrit-Change-Number: 8090 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 03 Oct 2017 22:04:09 + Gerrit-HasComments: Yes
[kudu-CR] mini-cluster: condition on NO TESTS=0
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8199 ) Change subject: mini-cluster: condition on NO_TESTS=0 .. mini-cluster: condition on NO_TESTS=0 This module will be incorporated into the kudu CLI in the near future, but for now this patch fixes builds that set NO_TESTS=1. Our precommit builds don't disable tests, so I tested this by running a local build with NO_TESTS=1. Without this patch, the build failed while linking libmini-cluster.so. With it, the build passes. Change-Id: I195b3ed5ed50dfbe7afb3004182bf02a3bcda31a Reviewed-on: http://gerrit.cloudera.org:8080/8199 Tested-by: Adar Dembo Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M src/kudu/mini-cluster/CMakeLists.txt 1 file changed, 5 insertions(+), 0 deletions(-) Approvals: Adar Dembo: Verified Kudu Jenkins: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/8199 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I195b3ed5ed50dfbe7afb3004182bf02a3bcda31a Gerrit-Change-Number: 8199 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
..
Patch Set 15:
(3 comments)
> Patch Set 15:
>
> > Patch Set 15:
> >
> > (1 comment)
>
> The behavior I saw before was that when I stopped MVCC on a leader and
> continued to write to that leader, something would successfully complete and
> I would see a mismatch in attempt numbers, resulting in a check failure.
>
> I think this is being caused by the following race with three nodes A*, B,
> and C:
> 1. A* gets a client request for rpc0
> 2. rpc0 gets replicated and the nodes begin applying with attempt 0
> 3. A* fails. The client will try different nodes, but if A* remains the
> leader, it will eventually cycle back to A* and try again, so it replicates
> again.
> 4. B and C begin to apply rpc0 attempt 1, registering it with the result
> tracker and preempting the follower transaction rpc0 attempt 0
> 5. B and C finish Applying rpc0 attempt 0 and fail because attempt 0 is no
> longer the driver
>
> If I put a sleep in ResultTracker::RecordCompletionAndRespond(), the above
> scenario is triggered consistently. We do this preemption to ensure that we
> don't respond to a leader transaction that may no longer be valid, but I
> don't see where we might be aborting this follower transaction.
>
> Currently dong more testing around this.
We just had another in-person chat that I thought I'd summarize:
This race is dependent on the fact that we're aborting _after_ replicating to
followers. This may not be possible on the master branch.
So A* is aborting during the leader's Apply phase, causing the client to retry
and cycle through replicas searching for the leader (since before we would
abort if the replica were not a leader). Eventually it would reach the same
replica and successfully replicate the same RPC _again_.
The followers OTOH are on their merry way Applying this successfully-replicated
attempt0. When the RPC gets replicated the second time, followers preempt the
current attempt and eventually crash because of it.
Getting back to the question of where to abort, I think Init() or Prepare()
would do the trick, I would think the earlier the better. Either _should_
prevent this, but I don't see the point in even submitting to the Prepare pool.
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/mvcc.h@277
PS15, Line 277: SetShuttingDown
> since there is no "action" this entity is doing maybe "closed" with be a be
Done
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/tablet.h@405
PS15, Line 405: // Stops the tablet. Once called, currently-Applying
operations should
: // terminate early, and further transactions should be
aborted.
: void Stop();
:
: bool Stopped() const {
: return mvcc_.IsShuttingDown();
: }
> I don't think we need this in tablet, since it's a plain proxy to mvcc whic
I added this in per Todd's request. You're right that we don't _need_ this in
tablet, but it's a cleaner API than reaching into MVCC directly.
I suppose the argument for it would be more compelling if there were more
things that needed to be stopped on Stop().
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/tablet.cc@438
PS15, Line 438: gscoped_ptr mvcc_tx;
: DCHECK(tx_state->has_timestamp());
: mvcc_tx.reset(new ScopedTransaction(&mvcc_,
tx_state->timestamp()));
: tx_state->SetMvccTx(std::move(mvcc_tx));
> I though we had discussed having transactions fail on the prepare phase if
My thinking was that Prepare could only be run if it were first Inited. There
may be some scenario between Init() and Prepare() the disk fails, but in that
case, once the transaction is replicated, this should fail in the Apply phase.
I.e. nothing should really change in the Prepare phase (yet) because the
Prepare phase only touches the WAL dir.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 15
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 03 Oct 2017 21:08:03 +
Gerrit-HasComments: Ye
[kudu-CR] mini-cluster: condition on NO TESTS=0
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8199 ) Change subject: mini-cluster: condition on NO_TESTS=0 .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8199 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I195b3ed5ed50dfbe7afb3004182bf02a3bcda31a Gerrit-Change-Number: 8199 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 03 Oct 2017 20:22:24 + Gerrit-HasComments: No
[kudu-CR] mini-cluster: condition on NO TESTS=0
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8199 ) Change subject: mini-cluster: condition on NO_TESTS=0 .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/8199 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I195b3ed5ed50dfbe7afb3004182bf02a3bcda31a Gerrit-Change-Number: 8199 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 03 Oct 2017 19:37:30 + Gerrit-HasComments: No
[kudu-CR] mini-cluster: condition on NO TESTS=0
Hello Alexey Serbin, Dan Burkert, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/8199 to review the following change. Change subject: mini-cluster: condition on NO_TESTS=0 .. mini-cluster: condition on NO_TESTS=0 This module will be incorporated into the kudu CLI in the near future, but for now this patch fixes builds that set NO_TESTS=1. Our precommit builds don't disable tests, so I tested this by running a local build with NO_TESTS=1. Without this patch, the build failed while linking libmini-cluster.so. With it, the build passes. Change-Id: I195b3ed5ed50dfbe7afb3004182bf02a3bcda31a --- M src/kudu/mini-cluster/CMakeLists.txt 1 file changed, 5 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/8199/1 -- To view, visit http://gerrit.cloudera.org:8080/8199 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I195b3ed5ed50dfbe7afb3004182bf02a3bcda31a Gerrit-Change-Number: 8199 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert
[kudu-CR] tool: add cluster shell action
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7853 ) Change subject: tool: add cluster shell action .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@28 PS8, Line 28: // The ID of the cluster, unique to the control shell. > What's the usecase for having multiple clusters? Can't a client just spawn Taking this a step further, I think the API would be better/simpler if the cluster creation arguments were provided as flags on the tool itself, so it automatically brings up the cluster when the tool is run. This makes the case of 'give me a mini cluster when I run the tool, shut it all down when I control-D' really easy. I would love this API, personally. -- To view, visit http://gerrit.cloudera.org:8080/7853 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e693921ef780dc4a06e536c6b7408f7f0b252f6 Gerrit-Change-Number: 7853 Gerrit-PatchSet: 8 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 03 Oct 2017 18:51:11 + Gerrit-HasComments: Yes
[kudu-CR] tool: add cluster shell action
Dan Burkert has posted comments on this change. (
http://gerrit.cloudera.org:8080/7853 )
Change subject: tool: add cluster shell action
..
Patch Set 8:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@684
PS8, Line 684: buf.append(serialized);
Have you checked what happens if the message has a string field containing a
newline?
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@685
PS8, Line 685: buf.append("\n");
> I'm curious whether this is really necessary or it's just for better troubl
It's necessary; the format is newline delimited JSON messages.
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@718
PS8, Line 718: CHECK_EQ(buf->length(), r);
> But in case of pipes, reading 0 from the read side of the pipe is possible
Correct, the issue is a short read, not a zero read.
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@735
PS8, Line 735: CHECK_EQ(buf.length(), r);
Short writes are also possible.
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc
File src/kudu/tools/tool_action_test.cc:
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@346
PS8, Line 346: ActionBuilder("shell", &RunControlShell)
> I have a proposal as well (you can consider it as bike shedding) -- it can
I'm in favor of mini_cluster because it's discoverable, self-describing, and
unambiguous. I don't think 'control' has any of those qualities.
--
To view, visit http://gerrit.cloudera.org:8080/7853
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e693921ef780dc4a06e536c6b7408f7f0b252f6
Gerrit-Change-Number: 7853
Gerrit-PatchSet: 8
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 03 Oct 2017 18:30:05 +
Gerrit-HasComments: Yes
[kudu-CR] tool: add cluster shell action
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/7853 )
Change subject: tool: add cluster shell action
..
Patch Set 8:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@663
PS8, Line 663: LOG(INFO)
nit: looks like more VLOG(1) to me.
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@669
PS8, Line 669: LOG(INFO)
ditto: VLOG(1) ?
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@680
PS8, Line 680: unable to print JSON to stdout
nit: 'unable to serialize into JSON: $0'?
The printing to stdout is done later at line 701, right?
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@685
PS8, Line 685: buf.append("\n");
I'm curious whether this is really necessary or it's just for better
troubleshooting, etc.? In any case, it would be nice if you could add a
comment explaining the reason of having '\n' appended.
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@718
PS8, Line 718: CHECK_EQ(buf->length(), r);
> This isn't guaranteed, per man 2 read:
But in case of pipes, reading 0 from the read side of the pipe is possible
with pipes only if the writer closes the write side, no?
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc
File src/kudu/tools/tool_action_test.cc:
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@216
PS8, Line 216: opts.master_rpc_ports = { 11030, 11031, 11032 };
> It's checked above on line 196.
Yes, num_master can only be 3 at this point if the code at line 196 stands as
is. My thinking was to assert that even if the code above changes and this
piece is not updated.
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@346
PS8, Line 346: ActionBuilder("shell", &RunControlShell)
> I'm opposed to calling this a shell. There is precedent for the term shell
I have a proposal as well (you can consider it as bike shedding) -- it can be
called 'control'. So, the invocation would be 'kudu test control' instead of
'kudu test shell'.
--
To view, visit http://gerrit.cloudera.org:8080/7853
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e693921ef780dc4a06e536c6b7408f7f0b252f6
Gerrit-Change-Number: 7853
Gerrit-PatchSet: 8
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 03 Oct 2017 18:15:23 +
Gerrit-HasComments: Yes
[kudu-CR] tool: add cluster shell action
Dan Burkert has posted comments on this change. (
http://gerrit.cloudera.org:8080/7853 )
Change subject: tool: add cluster shell action
..
Patch Set 8:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/CMakeLists.txt
File src/kudu/tools/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/CMakeLists.txt@120
PS8, Line 120: mini-cluster
This should use an underscore to be consistent with the rest of our libraries.
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto
File src/kudu/tools/tool.proto:
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@28
PS8, Line 28: // The ID of the cluster, unique to the control shell.
What's the usecase for having multiple clusters? Can't a client just spawn
multiple instances if they need multiple clusters?
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@29
PS8, Line 29: required int32 cluster_id = 1;
use optional
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@61
PS8, Line 61: required int32 cluster_id = 1;
use optional
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@71
PS8, Line 71: required int32 cluster_id = 1;
optional
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@80
PS8, Line 80: required int32 cluster_id = 1;
optional, etc.
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@613
PS8, Line 613: // Read and accumulate one byte at a time, looking for the
newline.
Seems like it would be easier and more efficient to keep the read buffer as a
field, do bigger reads, and use something like strnchr to find the newline.
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@616
PS8, Line 616: faststring one_byte;
hoist this out of the loop.
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@718
PS8, Line 718: CHECK_EQ(buf->length(), r);
This isn't guaranteed, per man 2 read:
> Upon successful completion, read(), readv(), and pread() return the number of
> bytes actually read and placed in the buffer. The system guarantees to read
> the number of bytes requested if the descriptor references a normal file that
> has that many bytes left before the end-of-file, but in no other case.
I think you may see this in practice if the client sends the length header, and
then sends the request body split across multiple write() calls (perhaps
pausing in between to make it more racy).
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc
File src/kudu/tools/tool_action_test.cc:
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@216
PS8, Line 216: opts.master_rpc_ports = { 11030, 11031, 11032 };
> nit: add DCHECK_EQ(3, opts.num_masters) ?
It's checked above on line 196.
Isn't the hard-coded master ports going to cause problems for multi-master,
multi-cluster shells? I think this is another good reason not to support
multiple clusters.
http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@346
PS8, Line 346: ActionBuilder("shell", &RunControlShell)
I'm opposed to calling this a shell. There is precedent for the term shell
with noSQL databases, and it's not this. People regularly ask if there's a
Kudu shell, and they aren't asking for this. Eventually we might have a proper
shell in the kudu tool. I think 'kudu mini_cluster' is a better name, given
that mini-cluster is an established term of art.
--
To view, visit http://gerrit.cloudera.org:8080/7853
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e693921ef780dc4a06e536c6b7408f7f0b252f6
Gerrit-Change-Number: 7853
Gerrit-PatchSet: 8
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 03 Oct 2017 15:24:47 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1125: issue one catalog write per tablet report
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8090 ) Change subject: KUDU-1125: issue one catalog write per tablet report .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc@3227 PS6, Line 3227: bootstrapping > Yeah. The original wording was "This prevents us from spuriously deleting r I thought bootstrapping and copying were synonymous? -- To view, visit http://gerrit.cloudera.org:8080/8090 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie6f5cf0e4b1cd1160b3b310d89c6dbf3dd62e43b Gerrit-Change-Number: 8090 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 03 Oct 2017 14:48:50 + Gerrit-HasComments: Yes
