[kudu-CR] KUDU-1900: add loopback check and test
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@330 PS6, Line 330: LOG(INFO) << "Using 127.0.0.1 for outbound sockets"; nit: how useful is this information? I guess it was just for initial debugging since the other case (external IP address) is not logged. If that's true, consider not logging it at all. Our tests are too chatty, and sometimes it looks like a flood by non-so-relevant information. http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@361 PS6, Line 361: const char* AUTH_REQUIRED nit: to protect the pointer itself from modifications, use const char* const AUTH_REQUIRED here and below http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@363 PS6, Line 363: RPC_ nit: the auth-related stuff is RPC-related as well, so since you have just AUTH_REQUIRED and AUTH_DISABLED, maybe use just ENCRYPTION_REQUIRED and ENCRYPTION_DISABLED -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 6 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 15 Feb 2019 06:21:47 + Gerrit-HasComments: Yes
[kudu-CR] [tools] Add useful flags to 'kudu remote replica list'
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12450 ) Change subject: [tools] Add useful flags to 'kudu remote_replica list' .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/12450/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/12450/2/src/kudu/tools/kudu-tool-test.cc@1949 PS2, Line 1949: foo Does it make sense to check for the 'positive case' as well (i.e. specify the expected name of the table)? http://gerrit.cloudera.org:8080/#/c/12450/2/src/kudu/tools/kudu-tool-test.cc@1964 PS2, Line 1964: Finally, tombstone the replica and try again Do we expect the schema to be present in the output? If yes, maybe add that check here as well? And if doing so, maybe consider adding a functor to reuse it in both places -- in the very first newly added scenario and here. -- To view, visit http://gerrit.cloudera.org:8080/12450 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I462515f1bc3e8487185aebb6cb99d1c5c00cea40 Gerrit-Change-Number: 12450 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Fri, 15 Feb 2019 05:33:29 + Gerrit-HasComments: Yes
[kudu-CR] [server] Add start time in server description
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12304 ) Change subject: [server] Add start_time in server description .. Patch Set 10: (5 comments) http://gerrit.cloudera.org:8080/#/c/12304/10/src/kudu/tools/tool_action_master.cc File src/kudu/tools/tool_action_master.cc: http://gerrit.cloudera.org:8080/#/c/12304/10/src/kudu/tools/tool_action_master.cc@149 PS10, Line 149: std::move no need for 'std::move' here since the return value of a function is already a temporary (technically I think it's called a "prvalue", and so the rvalue reference argument to emplace_back will already bind to it directly, see https://en.cppreference.com/w/cpp/language/value_category ) http://gerrit.cloudera.org:8080/#/c/12304/10/src/kudu/tools/tool_action_tserver.cc File src/kudu/tools/tool_action_tserver.cc: http://gerrit.cloudera.org:8080/#/c/12304/10/src/kudu/tools/tool_action_tserver.cc@148 PS10, Line 148: std::move same http://gerrit.cloudera.org:8080/#/c/12304/10/src/kudu/util/CMakeLists.txt File src/kudu/util/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/12304/10/src/kudu/util/CMakeLists.txt@259 PS10, Line 259: wire_protocol_proto see comment elsewhere: this is a circular reference between modules that we don't want http://gerrit.cloudera.org:8080/#/c/12304/10/src/kudu/util/pb_util.h File src/kudu/util/pb_util.h: http://gerrit.cloudera.org:8080/#/c/12304/10/src/kudu/util/pb_util.h@39 PS10, Line 39: } // namespace kudu I don't think this belongs in util/pb_util.h. util/ is meant to be very generic utility code that could be shared between projects, so a backward dependency up into kudu server specific stuff like this isn't clean. (In fact, Impala pulls in the util/ directory since it shares our krpc stack) Can you move this utility function somewhere else like common/wire_protocol.h since the PB is in wire_protocol.proto in that same package? http://gerrit.cloudera.org:8080/#/c/12304/10/src/kudu/util/pb_util.h@124 PS10, Line 124: std::string ParseStartTime(const ServerRegistrationPB& reg); nit: better to name this "StartTimeToString" rather than "Parse", since imo "parse" implies going _from_ a string format rather thna _to_ one -- To view, visit http://gerrit.cloudera.org:8080/12304 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I041467014499ad00c07398ad8f61c6d6ca1ceeca Gerrit-Change-Number: 12304 Gerrit-PatchSet: 10 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Fri, 15 Feb 2019 06:06:52 + Gerrit-HasComments: Yes
[kudu-CR] [examples] updated README.md for basic-python-example
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12458 ) Change subject: [examples] updated README.md for basic-python-example .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/12458/3/examples/python/basic-python-example/README.md File examples/python/basic-python-example/README.md: http://gerrit.cloudera.org:8080/#/c/12458/3/examples/python/basic-python-example/README.md@24 PS3, Line 24: directory. > remove Done -- To view, visit http://gerrit.cloudera.org:8080/12458 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I103dde66b69db17099b94d66777df37abb4f9fa8 Gerrit-Change-Number: 12458 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 15 Feb 2019 05:57:17 + Gerrit-HasComments: Yes
[kudu-CR] [examples] updated README.md for basic-python-example
Hello Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12458 to look at the new patch set (#4). Change subject: [examples] updated README.md for basic-python-example .. [examples] updated README.md for basic-python-example Change-Id: I103dde66b69db17099b94d66777df37abb4f9fa8 --- M examples/python/basic-python-example/README.md M examples/python/basic-python-example/basic_example.py 2 files changed, 16 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/12458/4 -- To view, visit http://gerrit.cloudera.org:8080/12458 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I103dde66b69db17099b94d66777df37abb4f9fa8 Gerrit-Change-Number: 12458 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR](branch-1.9.x) add release notes for 1.9.0
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12389 ) Change subject: add release notes for 1.9.0 .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/12389/4/docs/release_notes.adoc File docs/release_notes.adoc: http://gerrit.cloudera.org:8080/#/c/12389/4/docs/release_notes.adoc@234 PS4, Line 234: > remote this extra empty line? I meant: _remove_ this extra empty line -- To view, visit http://gerrit.cloudera.org:8080/12389 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.9.x Gerrit-MessageType: comment Gerrit-Change-Id: I733dfae39c06f15f7f55ae823678caf6ca433bfc Gerrit-Change-Number: 12389 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Fri, 15 Feb 2019 05:46:58 + Gerrit-HasComments: Yes
[kudu-CR](branch-1.9.x) add release notes for 1.9.0
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12389 ) Change subject: add release notes for 1.9.0 .. Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/12389/4/docs/release_notes.adoc File docs/release_notes.adoc: http://gerrit.cloudera.org:8080/#/c/12389/4/docs/release_notes.adoc@145 PS4, Line 145: agressively aggressively http://gerrit.cloudera.org:8080/#/c/12389/4/docs/release_notes.adoc@234 PS4, Line 234: remote this extra empty line? -- To view, visit http://gerrit.cloudera.org:8080/12389 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.9.x Gerrit-MessageType: comment Gerrit-Change-Id: I733dfae39c06f15f7f55ae823678caf6ca433bfc Gerrit-Change-Number: 12389 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Fri, 15 Feb 2019 05:45:37 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2686 python: remove multiprocessing
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12494 ) Change subject: KUDU-2686 python: remove multiprocessing .. KUDU-2686 python: remove multiprocessing The python multiprocessing library doesn't play very nicely when creating Pools after initializing complex state (e.g. the KuduClient). Because of how it forks, the library may even copy lock states, and lead to odd situations, like multiple threads waiting on a lock that isn't held by any thread in the process. This was the case in KUDU-2686, which resulted in a hang in test_scantoken.py. Upon inspection[1], there appeared to be multiple threads in a process waiting on the same sys_futex, but none of them held it. [2] tips some pieces to make it more reproducible (tested on Ubuntu 14.04, where the issue was first reported). Starting with python3.4, there is supposedly a way around this, which is to use a different process-startup pattern, though this isn't available in python2. This patch removes usage of multiprocessing entirely. While it can be argued that multiprocessing provides extra test coverage, given that multiprocessing is known to have such issues[3][4], that this isn't the first time we've been bitten by this forking issue, that it's test-only, and that scan tokens are tested in the C++ client as well, "dumbing" down the test doesn't seem unreasonable. [1] https://gist.github.com/andrwng/d2d21c551362ddd564926c2a4ec406ae [2] https://gist.github.com/andrwng/cc6c211c62b1235cc58944d513ba6655 [3] https://github.com/pytest-dev/pytest/issues/958 [4] https://codewithoutrules.com/2018/09/04/python-multiprocessing/ Change-Id: Ia9aa91191d54801731da27e5f132b3c96af0efa1 Reviewed-on: http://gerrit.cloudera.org:8080/12494 Tested-by: Kudu Jenkins Reviewed-by: Jordan Birdsell Reviewed-by: Alexey Serbin --- M python/kudu/tests/test_scantoken.py M python/setup.py 2 files changed, 4 insertions(+), 18 deletions(-) Approvals: Kudu Jenkins: Verified Jordan Birdsell: Looks good to me, approved Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/12494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ia9aa91191d54801731da27e5f132b3c96af0efa1 Gerrit-Change-Number: 12494 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Jordan Birdsell Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2686 python: remove multiprocessing
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12494 ) Change subject: KUDU-2686 python: remove multiprocessing .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia9aa91191d54801731da27e5f132b3c96af0efa1 Gerrit-Change-Number: 12494 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Jordan Birdsell Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 15 Feb 2019 05:07:36 + Gerrit-HasComments: No
[kudu-CR] Reduce "Overwriting operations" log spam on bootstrap
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12492 ) Change subject: Reduce "Overwriting operations" log spam on bootstrap .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12492 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I965f5950d2076c7a1147b943003b5e5df9a2246b Gerrit-Change-Number: 12492 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 15 Feb 2019 05:03:12 + Gerrit-HasComments: No
[kudu-CR] KUDU-2686 python: remove multiprocessing
Jordan Birdsell has posted comments on this change. ( http://gerrit.cloudera.org:8080/12494 ) Change subject: KUDU-2686 python: remove multiprocessing .. Patch Set 3: Makes sense to me. Looks good -- To view, visit http://gerrit.cloudera.org:8080/12494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia9aa91191d54801731da27e5f132b3c96af0efa1 Gerrit-Change-Number: 12494 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Jordan Birdsell Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 15 Feb 2019 05:03:23 + Gerrit-HasComments: No
[kudu-CR] KUDU-2686 python: remove multiprocessing
Jordan Birdsell has posted comments on this change. ( http://gerrit.cloudera.org:8080/12494 ) Change subject: KUDU-2686 python: remove multiprocessing .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia9aa91191d54801731da27e5f132b3c96af0efa1 Gerrit-Change-Number: 12494 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Jordan Birdsell Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 15 Feb 2019 05:03:08 + Gerrit-HasComments: No
[kudu-CR] [examples] updated README.md for basic-python-example
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12458 ) Change subject: [examples] updated README.md for basic-python-example .. Patch Set 3: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/12458/2/examples/python/basic-python-example/README.md File examples/python/basic-python-example/README.md: http://gerrit.cloudera.org:8080/#/c/12458/2/examples/python/basic-python-example/README.md@22 PS2, Line 22: It's assumed the commands below are run from the directory where : this README.md file is located, i.e. from : `$KUDU_HOME/examples/python/basic-python-example` directory. > That's about building the example as well. I wanted to emphasize that all Gotcha. http://gerrit.cloudera.org:8080/#/c/12458/3/examples/python/basic-python-example/README.md File examples/python/basic-python-example/README.md: http://gerrit.cloudera.org:8080/#/c/12458/3/examples/python/basic-python-example/README.md@24 PS3, Line 24: directory. remove -- To view, visit http://gerrit.cloudera.org:8080/12458 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I103dde66b69db17099b94d66777df37abb4f9fa8 Gerrit-Change-Number: 12458 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 15 Feb 2019 04:33:56 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2686 python: remove multiprocessing
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12494 to look at the new patch set (#3). Change subject: KUDU-2686 python: remove multiprocessing .. KUDU-2686 python: remove multiprocessing The python multiprocessing library doesn't play very nicely when creating Pools after initializing complex state (e.g. the KuduClient). Because of how it forks, the library may even copy lock states, and lead to odd situations, like multiple threads waiting on a lock that isn't held by any thread in the process. This was the case in KUDU-2686, which resulted in a hang in test_scantoken.py. Upon inspection[1], there appeared to be multiple threads in a process waiting on the same sys_futex, but none of them held it. [2] tips some pieces to make it more reproducible (tested on Ubuntu 14.04, where the issue was first reported). Starting with python3.4, there is supposedly a way around this, which is to use a different process-startup pattern, though this isn't available in python2. This patch removes usage of multiprocessing entirely. While it can be argued that multiprocessing provides extra test coverage, given that multiprocessing is known to have such issues[3][4], that this isn't the first time we've been bitten by this forking issue, that it's test-only, and that scan tokens are tested in the C++ client as well, "dumbing" down the test doesn't seem unreasonable. [1] https://gist.github.com/andrwng/d2d21c551362ddd564926c2a4ec406ae [2] https://gist.github.com/andrwng/cc6c211c62b1235cc58944d513ba6655 [3] https://github.com/pytest-dev/pytest/issues/958 [4] https://codewithoutrules.com/2018/09/04/python-multiprocessing/ Change-Id: Ia9aa91191d54801731da27e5f132b3c96af0efa1 --- M python/kudu/tests/test_scantoken.py M python/setup.py 2 files changed, 4 insertions(+), 18 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/12494/3 -- To view, visit http://gerrit.cloudera.org:8080/12494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia9aa91191d54801731da27e5f132b3c96af0efa1 Gerrit-Change-Number: 12494 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [tools] Add tablet data dirs to 'kudu remote replica list'
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12449 )
Change subject: [tools] Add tablet data dirs to 'kudu remote replica list'
..
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12449/2/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:
http://gerrit.cloudera.org:8080/#/c/12449/2/src/kudu/tablet/tablet_replica.cc@448
PS2, Line 448: // an empty 'data_dirs' in this case-- the state and last
status will inform
nit: add a separating space
http://gerrit.cloudera.org:8080/#/c/12449/2/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/12449/2/src/kudu/tools/kudu-tool-test.cc@1898
PS2, Line 1898: workload.set_num_tablets(kNumTablets);
nit: add const ?
http://gerrit.cloudera.org:8080/#/c/12449/2/src/kudu/tools/kudu-tool-test.cc@1901
PS2, Line 1901:
: TServerDetails* ts =
ts_map_[cluster_->tablet_server(0)->uuid()];
: vector tablets;
: ASSERT_OK(WaitForNumTabletsOnTS(ts, kNumTablets, kTim
nit: if you do std:move(opts), maybe move all the related code under a scope?
That's to avoid re-using invalidated opts below in case of modifications, etc.
http://gerrit.cloudera.org:8080/#/c/12449/2/src/kudu/tools/kudu-tool-test.cc@1923
PS2, Line 1923: TEST_F(ToolTest, TestLocalReplicaDelete) {
: NO_FATALS(StartMiniCluster());
:
: // TestWorkLoad.Setup() internally generates a table.
: TestWorkload workload(mini_cluster_.get());
: workload.set_num_replicas(1);
nit: if that info is exactly the same in both alive and tombstoned tablet,
maybe create a functor and use it instead of duplicating the code?
--
To view, visit http://gerrit.cloudera.org:8080/12449
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e509b5c080a76a4110a32ecc073418c28aa0633
Gerrit-Change-Number: 12449
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Fri, 15 Feb 2019 03:56:35 +
Gerrit-HasComments: Yes
[kudu-CR] Reduce "Overwriting operations" log spam on bootstrap
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12492 ) Change subject: Reduce "Overwriting operations" log spam on bootstrap .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12492 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I965f5950d2076c7a1147b943003b5e5df9a2246b Gerrit-Change-Number: 12492 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 15 Feb 2019 02:59:43 + Gerrit-HasComments: No
[kudu-CR] [examples] updated README.md for basic-python-example
Hello Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12458 to look at the new patch set (#3). Change subject: [examples] updated README.md for basic-python-example .. [examples] updated README.md for basic-python-example Change-Id: I103dde66b69db17099b94d66777df37abb4f9fa8 --- M examples/python/basic-python-example/README.md M examples/python/basic-python-example/basic_example.py 2 files changed, 16 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/12458/3 -- To view, visit http://gerrit.cloudera.org:8080/12458 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I103dde66b69db17099b94d66777df37abb4f9fa8 Gerrit-Change-Number: 12458 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [examples] updated README.md for basic-python-example
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12458 ) Change subject: [examples] updated README.md for basic-python-example .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/12458/2/examples/python/basic-python-example/README.md File examples/python/basic-python-example/README.md: http://gerrit.cloudera.org:8080/#/c/12458/2/examples/python/basic-python-example/README.md@22 PS2, Line 22: It's assumed the commands below are run from the directory where : this README.md file is located, i.e. from : `$KUDU_HOME/examples/python/basic-python-example` directory. > This probably belongs down by "Running the example", right? The commands up That's about building the example as well. I wanted to emphasize that all those copy-paste scenarios can be run from the directory that contains this README.md file. http://gerrit.cloudera.org:8080/#/c/12458/2/examples/python/basic-python-example/README.md@24 PS2, Line 24: Also, : the recipe requires the Kudu C++ components already built in : `$KUDU_HOME/build/latest`. > There's probably a way to merge this with the NOTE below, since it mentions Done -- To view, visit http://gerrit.cloudera.org:8080/12458 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I103dde66b69db17099b94d66777df37abb4f9fa8 Gerrit-Change-Number: 12458 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 15 Feb 2019 02:42:35 + Gerrit-HasComments: Yes
[kudu-CR] [tests] fix running tests under the super-user
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12486 ) Change subject: [tests] fix running tests under the super-user .. [tests] fix running tests under the super-user Recently I found myself running Kudu tests as a super-user on a VM. A couple of test failed, and this patch fixes that. This is a test-only changelist, it doesn't change any functionality. Change-Id: I05bf220ee8000209a3e36faa9f21fd43ab8bdc9f Reviewed-on: http://gerrit.cloudera.org:8080/12486 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M src/kudu/rpc/negotiation-test.cc M src/kudu/util/env-test.cc 2 files changed, 20 insertions(+), 6 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/12486 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I05bf220ee8000209a3e36faa9f21fd43ab8bdc9f Gerrit-Change-Number: 12486 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2686 python: remove multiprocessing
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12494 to look at the new patch set (#2). Change subject: KUDU-2686 python: remove multiprocessing .. KUDU-2686 python: remove multiprocessing The python multiprocessing library doesn't play very nicely when creating Pools after initializing complex state (e.g. the KuduClient). Because of how it forks, the library may even copy lock states, and lead to odd situations, like multiple threads waiting on a lock that isn't held by any thread in the process. This was the case in KUDU-2686, which resulted in a hang in test_scantoken.py. Upon inspection[1], there appeared to be multiple threads in a process waiting on the same sys_futex, but none of them held it. [2] tips some pieces to make it more reproducible (tested on Ubuntu 14.04, where the issue was first reported). Starting with python3.4, there is supposedly a way around this, which is to use a different process-startup pattern, though this isn't available in python2. This patch removes usage of multiprocessing entirely. While it can be argued that multiprocessing provides extra test coverage, given that multiprocessing is known to have such issues[3][4], that this isn't the first time we've been bitten by this forking issue, that it's test-only, and that scan tokens are tested in the C++ client as well, "dumbing" down the test doesn't seem unreasonable. [1] https://gist.github.com/andrwng/d2d21c551362ddd564926c2a4ec406ae [2] https://gist.github.com/andrwng/cc6c211c62b1235cc58944d513ba6655 [3] https://github.com/pytest-dev/pytest/issues/958 [4] https://codewithoutrules.com/2018/09/04/python-multiprocessing/ Change-Id: Ia9aa91191d54801731da27e5f132b3c96af0efa1 --- M python/kudu/tests/test_scantoken.py M python/setup.py 2 files changed, 4 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/12494/2 -- To view, visit http://gerrit.cloudera.org:8080/12494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia9aa91191d54801731da27e5f132b3c96af0efa1 Gerrit-Change-Number: 12494 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2686 python: remove multiprocessing
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12494 Change subject: KUDU-2686 python: remove multiprocessing .. KUDU-2686 python: remove multiprocessing The python multiprocessing library doesn't play very nicely when creating Pools after initializing complex state (e.g. the KuduClient). Because of how it forks, the library may even copy lock states, and lead to odd situations, like multiple threads waiting on a lock that isn't held by any thread in the process. This was the case in KUDU-2686, which resulted in a hang in test_scantoken.py. Upon inspection[1], there appeared to be multiple threads in a process waiting on the same sys_futex, but none of them held it. [2] tips some pieces to make it more reproducible (tested on Ubuntu 14.04, where the issue was first reported). Starting with python3.4, there is supposedly a way around this, which is to use a different process-startup pattern, though this isn't available in python2. This patch removes usage of multiprocessing entirely. While it can be argued that multiprocessing provides extra test coverage, given this is a relatively known [3][4] issue, that this isn't the first time we've been bitten by this forking issue, that it's test-only, and that scan tokens are tested in the C++ client as well, "dumbing" down the test doesn't seem unreasonable. [1] https://gist.github.com/andrwng/d2d21c551362ddd564926c2a4ec406ae [2] https://gist.github.com/andrwng/cc6c211c62b1235cc58944d513ba6655 [3] https://github.com/pytest-dev/pytest/issues/958 [4] https://codewithoutrules.com/2018/09/04/python-multiprocessing/ Change-Id: Ia9aa91191d54801731da27e5f132b3c96af0efa1 --- M python/kudu/tests/test_scantoken.py M python/setup.py 2 files changed, 4 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/12494/1 -- To view, visit http://gerrit.cloudera.org:8080/12494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ia9aa91191d54801731da27e5f132b3c96af0efa1 Gerrit-Change-Number: 12494 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] KUDU-2508: [DOCS] Recommend nscd even for static name resolutions
Alex Rodoni has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12493 Change subject: KUDU-2508: [DOCS] Recommend nscd even for static name resolutions .. KUDU-2508: [DOCS] Recommend nscd even for static name resolutions Change-Id: I93b192a2c664262477200f34f3819e48bcbbfc26 --- M docs/troubleshooting.adoc 1 file changed, 13 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/12493/1 -- To view, visit http://gerrit.cloudera.org:8080/12493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I93b192a2c664262477200f34f3819e48bcbbfc26 Gerrit-Change-Number: 12493 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Rodoni
[kudu-CR] Fix compilation warnings under debian8.9
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12294 to look at the new patch set (#4). Change subject: Fix compilation warnings under debian8.9 .. Fix compilation warnings under debian8.9 It seems there are some compilation warnings under debian8.9, with gcc 4.9.2/5.5.0, and the warning info is formatted as follows: "warning: ‘xxx’ may be used uninitialized in this function" Change-Id: I2464ff7938f79d090ebd1676da2108ce7dae4ca5 --- M src/kudu/cfile/bshuf_block.cc M src/kudu/cfile/rle_block.h M src/kudu/common/row_operations.cc M src/kudu/common/table_util.cc M src/kudu/consensus/leader_election.cc M src/kudu/consensus/log-test.cc M src/kudu/fs/log_block_manager-test-util.cc M src/kudu/hms/hms_client-test.cc M src/kudu/integration-tests/log_verifier.cc M src/kudu/rpc/server_negotiation.cc M src/kudu/server/tracing_path_handlers.cc M src/kudu/tablet/cfile_set.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/tablet-test-util.h M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_perf.cc M src/kudu/tserver/mini_tablet_server-test.cc M src/kudu/util/debug/trace_event_impl.cc M src/kudu/util/net/sockaddr.cc M src/kudu/util/pb_util.cc M src/kudu/util/status.cc 22 files changed, 44 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/12294/4 -- To view, visit http://gerrit.cloudera.org:8080/12294 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2464ff7938f79d090ebd1676da2108ce7dae4ca5 Gerrit-Change-Number: 12294 Gerrit-PatchSet: 4 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu
[kudu-CR] KUDU-1900: add loopback check and test
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@341
PS6, Line 341: if ((NetworkByteOrder::FromHost32(addr) >> 24) != 127) {
> Should I instantiate Sockaddr and use Sockaddr::IsAnyLocalAddress instead?
That'd be fine too; any solution that works within our existing abstractions is
preferable.
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 6
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 15 Feb 2019 01:30:58 +
Gerrit-HasComments: Yes
[kudu-CR] Fix tracing of log appending and reduce log-related log spam
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12491 ) Change subject: Fix tracing of log appending and reduce log-related log spam .. Fix tracing of log appending and reduce log-related log spam While messing around on a server I noticed a funny trace: 0212 16:29:26.554846 (+ 0us) service_pool.cc:163] Inserting onto call queue 0212 16:29:26.554854 (+ 8us) service_pool.cc:222] Handling call 0212 16:29:28.219369 (+1664515us) inbound_call.cc:157] Queueing success response Related trace 'txn': 0212 16:29:26.561719 (+ 0us) write_transaction.cc:101] PREPARE: Starting 0212 16:29:26.562102 (+ 383us) write_transaction.cc:268] Acquiring schema lock in shared mode 0212 16:29:26.562103 (+ 1us) write_transaction.cc:271] Acquired schema lock 0212 16:29:26.562104 (+ 1us) tablet.cc:400] PREPARE: Decoding operations 0212 16:29:26.599420 (+ 37316us) tablet.cc:422] PREPARE: Acquiring locks for 6376 operations 0212 16:29:26.611285 (+ 11865us) tablet.cc:426] PREPARE: locks acquired 0212 16:29:26.611286 (+ 1us) write_transaction.cc:126] PREPARE: finished. 0212 16:29:26.611389 (+ 103us) write_transaction.cc:136] Start() 0212 16:29:26.611392 (+ 3us) write_transaction.cc:141] Timestamp: P: 1550017766611388 usec, L: 0 0212 16:29:26.613188 (+ 1796us) log.cc:582] Serialized 10493083 byte log entry 0212 16:29:26.735023 (+121835us) write_transaction.cc:149] APPLY: Starting 0212 16:29:28.213010 (+1477987us) tablet_metrics.cc:365] ProbeStats: bloom_lookups=27143,key_file_lookups=5,delta_file_lookups=0,mrs_lookups=6376 0212 16:29:28.214317 (+ 1307us) log.cc:582] Serialized 38279 byte log entry 0212 16:29:28.214376 (+59us) write_transaction.cc:309] Releasing row and schema locks 0212 16:29:28.216505 (+ 2129us) write_transaction.cc:277] Released schema lock 0212 16:29:28.219357 (+ 2852us) write_transaction.cc:196] FINISH: updating metrics 0212 16:29:28.219709 (+ 352us) write_transaction.cc:309] Releasing row and schema locks 0212 16:29:28.219709 (+ 0us) write_transaction.cc:277] Released schema lock 0212 16:29:28.824261 (+604552us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegment4us8LW 0212 16:29:29.531241 (+706980us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentGJU550 0212 16:29:30.254932 (+723691us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmenteWEBF9 0212 16:29:31.005554 (+750622us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentsAIJzm 0212 16:29:31.695339 (+689785us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentsBimGD 0212 16:29:32.443351 (+748012us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentamea7Y 0212 16:29:33.180797 (+737446us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentSam2No 0212 16:29:33.905360 (+724563us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmenthfhQDS 0212 16:29:34.634994 (+729634us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentxDVwMq 0212 16:29:35.384800 (+749806us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentjMi862 0212 16:29:36.080099 (+695299us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentRgaDAJ 0212 16:29:36.822293 (+742194us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentwSXUHR 0212 16:29:37.540434 (+718141us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentCp3SMG 0212 16:29:38.289865 (+749431us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentyEttfA 0212 16:29:38.993878 (+704013us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegment7OTQ23 0212 16:29:39.759563 (+765685us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentzk7y0x 0212 16:29:40.495284 (+735721us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmenttvkp8B 0212 16:29:41.289037 (+793753us) log.c
[kudu-CR] Fix compilation warnings under debian8.9
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12294 ) Change subject: Fix compilation warnings under debian8.9 .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/12294/3/src/kudu/consensus/leader_election.cc File src/kudu/consensus/leader_election.cc: http://gerrit.cloudera.org:8080/#/c/12294/3/src/kudu/consensus/leader_election.cc@162 PS3, Line 162: #pragma GCC diagnostic push > Nit: add a blank line just above this, and the comment "Suppress false posi Done http://gerrit.cloudera.org:8080/#/c/12294/3/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/12294/3/src/kudu/tablet/cfile_set.cc@324 PS3, Line 324: // Disable compilation warnings. > Nit: change comment to "Suppress false positive about 'rowid' used when uni Done -- To view, visit http://gerrit.cloudera.org:8080/12294 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2464ff7938f79d090ebd1676da2108ce7dae4ca5 Gerrit-Change-Number: 12294 Gerrit-PatchSet: 3 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu Gerrit-Comment-Date: Fri, 15 Feb 2019 01:25:15 + Gerrit-HasComments: Yes
[kudu-CR] Reduce "Overwriting operations" log spam on bootstrap
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12492 Change subject: Reduce "Overwriting operations" log spam on bootstrap .. Reduce "Overwriting operations" log spam on bootstrap While messing around on a cluster, I noticed that when bootstrapping tablets it would absolutely spew messages like the following: I0214 17:18:28.358758 13664 tablet_bootstrap.cc:910] T 97537c2af52940f48f06d32ecfdde2a2 P 09d6bf7a02124145b43f43cb7a667b3d: Overwriting operations starting at: 283482.180847 up to 283482.180847 with operation: 283483.180847 This is an expected outcome when a leader or follower has uncommitted operations that get rolled back because of updates from a new leader in a later term. There's nothing unusual or noteworthy about it. So, let's not bother logging at INFO. I lowered the level to VLOG(1) in case someone finds a need to see it in the future. Change-Id: I965f5950d2076c7a1147b943003b5e5df9a2246b --- M src/kudu/tablet/tablet_bootstrap.cc 1 file changed, 4 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/12492/1 -- To view, visit http://gerrit.cloudera.org:8080/12492 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I965f5950d2076c7a1147b943003b5e5df9a2246b Gerrit-Change-Number: 12492 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@341
PS6, Line 341: if ((NetworkByteOrder::FromHost32(addr) >> 24) != 127) {
> Likewise, this should be encapsulated in a new function in Network. Maybe s
Should I instantiate Sockaddr and use Sockaddr::IsAnyLocalAddress instead?
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 6
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 15 Feb 2019 01:23:25 +
Gerrit-HasComments: Yes
[kudu-CR] Fix tracing of log appending and reduce log-related log spam
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12491 ) Change subject: Fix tracing of log appending and reduce log-related log spam .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12491 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia50698e3af321b4ab87ee3974525dea6fc551613 Gerrit-Change-Number: 12491 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 15 Feb 2019 00:59:38 + Gerrit-HasComments: No
[kudu-CR] [tests] fix running tests under the super-user
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12486 ) Change subject: [tests] fix running tests under the super-user .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12486 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I05bf220ee8000209a3e36faa9f21fd43ab8bdc9f Gerrit-Change-Number: 12486 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 15 Feb 2019 00:58:34 + Gerrit-HasComments: No
[kudu-CR] [tests] fix running tests under the super-user
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12486 )
Change subject: [tests] fix running tests under the super-user
..
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12486/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12486/2//COMMIT_MSG@9
PS2, Line 9: Recent
> Nit: just 'That'
Done
http://gerrit.cloudera.org:8080/#/c/12486/2/src/kudu/rpc/negotiation-test.cc
File src/kudu/rpc/negotiation-test.cc:
http://gerrit.cloudera.org:8080/#/c/12486/2/src/kudu/rpc/negotiation-test.cc@1169
PS2, Line 1169: if (geteuid() == 0) {
: // The super-user can acess the 'inaccessible' keytab file
anyway.
: ASSERT_TRUE(s.ok()) << s.ToString();
: } else {
: ASSERT_FALSE(s.ok()) << s.ToString();
> Maybe cleaner as a ternary?
Yep, that makes sense when there is only one related assertion, but in this
case there is also ASSERT_STR_MATCHES(s.ToString(), "error accessing keytab:
Permission denied") for KRB5_VERSION_LE_1_10. I'll try to apply this in other
places, though.
--
To view, visit http://gerrit.cloudera.org:8080/12486
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I05bf220ee8000209a3e36faa9f21fd43ab8bdc9f
Gerrit-Change-Number: 12486
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 15 Feb 2019 00:54:47 +
Gerrit-HasComments: Yes
[kudu-CR] [tests] fix running tests under the super-user
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12486 to look at the new patch set (#3). Change subject: [tests] fix running tests under the super-user .. [tests] fix running tests under the super-user Recently I found myself running Kudu tests as a super-user on a VM. A couple of test failed, and this patch fixes that. This is a test-only changelist, it doesn't change any functionality. Change-Id: I05bf220ee8000209a3e36faa9f21fd43ab8bdc9f --- M src/kudu/rpc/negotiation-test.cc M src/kudu/util/env-test.cc 2 files changed, 20 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/12486/3 -- To view, visit http://gerrit.cloudera.org:8080/12486 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I05bf220ee8000209a3e36faa9f21fd43ab8bdc9f Gerrit-Change-Number: 12486 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@337
PS6, Line 337: if (GetLocalNetworks(&local_networks).ok()) {
> I think, effectively, this is a hard stop to the current test case. If GetL
That's not a hard stop though; by skipping the remainder of the test, it'll be
marked as PASSED.
A hard stop would be an ASSERT failure that'd mark the test as FAILED. Or a
CHECK failure that crashes the test.
Put another way: does !GetLocalNetworks.ok() signify an unexpected error in the
platform environment? Or an error that might be expected and fully
deterministic on some platforms? Based on a reading of the GetLocalNetworks
code, it looks like the former to me, in which case we should treat it the same
way as e.g. a failure to read a test file: fail the test.
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@342
PS6, Line 342: char s[INET_ADDRSTRLEN];
: inet_ntop(AF_INET, &addr, s, INET_ADDRSTRLEN);
> I wasn't sure if creating new members inside Network is warranted until the
Not really. My view is that I want as much platform-specific code (i.e. all
those headers you had to add) out of random tests, and squirreled away behind
util classes. The Network class purports to provide an abstraction for
interacting with an IPv4 network; adding more methods to it rounds out the
abstraction and makes it more useful for future use cases.
It's especially non-concerning given that the two methods I suggested are
read-only; they just provide slightly different views/transforms of the
abstraction.
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 6
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 15 Feb 2019 00:51:02 +
Gerrit-HasComments: Yes
[kudu-CR] Fix tracing of log appending and reduce log-related log spam
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12491 ) Change subject: Fix tracing of log appending and reduce log-related log spam .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/12491 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia50698e3af321b4ab87ee3974525dea6fc551613 Gerrit-Change-Number: 12491 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 15 Feb 2019 00:43:56 + Gerrit-HasComments: No
[kudu-CR] Fix tracing of log appending and reduce log-related log spam
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12491 Change subject: Fix tracing of log appending and reduce log-related log spam .. Fix tracing of log appending and reduce log-related log spam While messing around on a server I noticed a funny trace: 0212 16:29:26.554846 (+ 0us) service_pool.cc:163] Inserting onto call queue 0212 16:29:26.554854 (+ 8us) service_pool.cc:222] Handling call 0212 16:29:28.219369 (+1664515us) inbound_call.cc:157] Queueing success response Related trace 'txn': 0212 16:29:26.561719 (+ 0us) write_transaction.cc:101] PREPARE: Starting 0212 16:29:26.562102 (+ 383us) write_transaction.cc:268] Acquiring schema lock in shared mode 0212 16:29:26.562103 (+ 1us) write_transaction.cc:271] Acquired schema lock 0212 16:29:26.562104 (+ 1us) tablet.cc:400] PREPARE: Decoding operations 0212 16:29:26.599420 (+ 37316us) tablet.cc:422] PREPARE: Acquiring locks for 6376 operations 0212 16:29:26.611285 (+ 11865us) tablet.cc:426] PREPARE: locks acquired 0212 16:29:26.611286 (+ 1us) write_transaction.cc:126] PREPARE: finished. 0212 16:29:26.611389 (+ 103us) write_transaction.cc:136] Start() 0212 16:29:26.611392 (+ 3us) write_transaction.cc:141] Timestamp: P: 1550017766611388 usec, L: 0 0212 16:29:26.613188 (+ 1796us) log.cc:582] Serialized 10493083 byte log entry 0212 16:29:26.735023 (+121835us) write_transaction.cc:149] APPLY: Starting 0212 16:29:28.213010 (+1477987us) tablet_metrics.cc:365] ProbeStats: bloom_lookups=27143,key_file_lookups=5,delta_file_lookups=0,mrs_lookups=6376 0212 16:29:28.214317 (+ 1307us) log.cc:582] Serialized 38279 byte log entry 0212 16:29:28.214376 (+59us) write_transaction.cc:309] Releasing row and schema locks 0212 16:29:28.216505 (+ 2129us) write_transaction.cc:277] Released schema lock 0212 16:29:28.219357 (+ 2852us) write_transaction.cc:196] FINISH: updating metrics 0212 16:29:28.219709 (+ 352us) write_transaction.cc:309] Releasing row and schema locks 0212 16:29:28.219709 (+ 0us) write_transaction.cc:277] Released schema lock 0212 16:29:28.824261 (+604552us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegment4us8LW 0212 16:29:29.531241 (+706980us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentGJU550 0212 16:29:30.254932 (+723691us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmenteWEBF9 0212 16:29:31.005554 (+750622us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentsAIJzm 0212 16:29:31.695339 (+689785us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentsBimGD 0212 16:29:32.443351 (+748012us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentamea7Y 0212 16:29:33.180797 (+737446us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentSam2No 0212 16:29:33.905360 (+724563us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmenthfhQDS 0212 16:29:34.634994 (+729634us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentxDVwMq 0212 16:29:35.384800 (+749806us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentjMi862 0212 16:29:36.080099 (+695299us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentRgaDAJ 0212 16:29:36.822293 (+742194us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentwSXUHR 0212 16:29:37.540434 (+718141us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentCp3SMG 0212 16:29:38.289865 (+749431us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentyEttfA 0212 16:29:38.993878 (+704013us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegment7OTQ23 0212 16:29:39.759563 (+765685us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmentzk7y0x 0212 16:29:40.495284 (+735721us) log.cc:1006] Preallocating 8388608 byte segment in /data/15/kudu/wals/b5ecd0b8d23a4a529650e5871741f23e/.kudutmp.newsegmenttvkp8B 0212 16:29:41.289037 (+793753us) log.cc:1006] P
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 6:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@376
PS2, Line 376: public SecurityITest,
> external
Done
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@378
PS2, Line 378: };
> The initialization isn't necessary; getifaddrs() is going to overwrite 'ifa
Done
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@380
PS2, Line 380: INSTANTIATE_TEST_CASE_P(, AuthTokenIssuingTest,
::testing::ValuesIn(
> Would be cleaner if this could early-out rather than introduce a new nested
Done
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@382
PS2, Line 382: // The following 3 test cases cover passing authn token
over an
> Wrap, too long.
Done
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@383
PS2, Line 383: // encrypted loopback connection.
> Use C++-style casts here (static_cast or reinterpret_cast).
Done
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@388
PS2, Line 388:
> Probably clearer as:
Done
http://gerrit.cloudera.org:8080/#/c/12474/2/src/kudu/integration-tests/security-itest.cc@392
PS2, Line 392: { BindMode::LOOPBACK, AUTH_DISABLED,
RPC_ENCRYPTION_DISABLED,
> Use RAII principles: set up a SCOPED_CLEANUP that calls freeifaddrs() when
Done
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@325
PS6, Line 325: bool assignIPToClient(bool external) {
> Should use full camel-case: AssignIPToClient.
Done
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@337
PS6, Line 337: if (GetLocalNetworks(&local_networks).ok()) {
> In the context of a test, I think we should hard stop if we encounter an un
I think, effectively, this is a hard stop to the current test case. If
GetLocalNetworks fails, this function returns FALSE and the caller will issue a
warning and move on to the next test case.
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@342
PS6, Line 342: char s[INET_ADDRSTRLEN];
: inet_ntop(AF_INET, &addr, s, INET_ADDRSTRLEN);
> Could you encapsulate this into a new function inside Network? Seems like N
I wasn't sure if creating new members inside Network is warranted until there
is at least one non-test use case for them and until there is at least more
than one use case for them. In other words, wouldn't this be premature
optimization?
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@463
PS6, Line 463:
> Nit: extra space here.
Done
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 6
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 15 Feb 2019 00:35:28 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 6:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@325
PS6, Line 325: bool assignIPToClient(bool external) {
Should use full camel-case: AssignIPToClient.
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@337
PS6, Line 337: if (GetLocalNetworks(&local_networks).ok()) {
In the context of a test, I think we should hard stop if we encounter an
unexpected situation, such as GetLocalNetworks failing.
To that end, could you modify this function to return a Status, wrap this call
in RETURN_NOT_OK, and communicate the success or failure of assignment via a
bool* OUT parameter?
Alternatively, you could treat an OK return result as meaning that all calls
succeeded and a local IP was assigned, and a NotFound return result as a
failure in assignment.
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@338
PS6, Line 338: for (vector::iterator it = local_networks.begin();
: it != local_networks.end(); ++it) {
You should use a range-based for loop, new to C++11:
for (const auto& network : local_networks) {
https://en.cppreference.com/w/cpp/language/range-for has more info.
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@341
PS6, Line 341: if ((NetworkByteOrder::FromHost32(addr) >> 24) != 127) {
Likewise, this should be encapsulated in a new function in Network. Maybe
something like "IsLocalhost"?
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@342
PS6, Line 342: char s[INET_ADDRSTRLEN];
: inet_ntop(AF_INET, &addr, s, INET_ADDRSTRLEN);
Could you encapsulate this into a new function inside Network? Seems like
Network::ParseCIDRString is similar (though it does the opposite work).
http://gerrit.cloudera.org:8080/#/c/12474/6/src/kudu/integration-tests/security-itest.cc@463
PS6, Line 463:
Nit: extra space here.
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 6
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 15 Feb 2019 00:22:43 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2701 Fix compaction loop due to using wrong rowset size
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12488 ) Change subject: KUDU-2701 Fix compaction loop due to using wrong rowset size .. Patch Set 1: (1 comment) Have you tried putting this patch on the cluster you saw KUDU-2701 on? http://gerrit.cloudera.org:8080/#/c/12488/1/src/kudu/tablet/rowset_info.cc File src/kudu/tablet/rowset_info.cc: http://gerrit.cloudera.org:8080/#/c/12488/1/src/kudu/tablet/rowset_info.cc@474 PS1, Line 474: cdf_rs.value_ = ComputeRowsetValue(cdf_rs.width(), cdf_rs.extra_->size_bytes); Perhaps make a note of KUDU-2701 that the distinction between size_bytes and base_and_redos_size_mb_ is intentional and very important? Maybe a summarizing comment? -- To view, visit http://gerrit.cloudera.org:8080/12488 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 Gerrit-Change-Number: 12488 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 23:53:59 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12490 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 1: pardon, messed up Change-Id and created a separate review -- To view, visit http://gerrit.cloudera.org:8080/12490 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I74b41235af1d1d8da3506f8c7ac93f226d093040 Gerrit-Change-Number: 12490 Gerrit-PatchSet: 1 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 23:46:27 + Gerrit-HasComments: No
[kudu-CR] KUDU-1900: add loopback check and test
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12474 to look at the new patch set (#6). Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. This change adds a test case to AuthTokenIssuingTest which verifies that auth token is passed through unencrypted local connections. Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/socket.cc 2 files changed, 138 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12474/6 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 6 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has abandoned this change. ( http://gerrit.cloudera.org:8080/12490 ) Change subject: KUDU-1900: add loopback check and test .. Abandoned -- To view, visit http://gerrit.cloudera.org:8080/12490 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I74b41235af1d1d8da3506f8c7ac93f226d093040 Gerrit-Change-Number: 12490 Gerrit-PatchSet: 1 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12490 Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. This change adds a test case to AuthTokenIssuingTest which verifies that auth token is passed through unencrypted local connections. Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f address comments Change-Id: I74b41235af1d1d8da3506f8c7ac93f226d093040 --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/socket.cc 2 files changed, 138 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/12490/1 -- To view, visit http://gerrit.cloudera.org:8080/12490 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I74b41235af1d1d8da3506f8c7ac93f226d093040 Gerrit-Change-Number: 12490 Gerrit-PatchSet: 1 Gerrit-Owner: Greg Solovyev
[kudu-CR] KUDU-2701 Fix compaction loop due to using wrong rowset size
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12488 ) Change subject: KUDU-2701 Fix compaction loop due to using wrong rowset size .. Patch Set 1: (3 comments) Just passing through; hope you get http://gerrit.cloudera.org:8080/#/c/12488/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12488/1//COMMIT_MSG@14 PS1, Line 14: the size of the whole rowset should be used Can you be more explicit about which missing pieces are critical? Is it the bloom? The adhoc index (in the case of a composite primary key)? The UNDOs? http://gerrit.cloudera.org:8080/#/c/12488/1//COMMIT_MSG@15 PS1, Line 15: values Nit: this is used as a verb, right? Took a while to parse the sentence; maybe use a different verb that doesn't sound so much like a noun? http://gerrit.cloudera.org:8080/#/c/12488/1//COMMIT_MSG@19 PS1, Line 19: For example, I discovered a case where a tablet : had 8 rowsets of "size" 16MB. In reality, this size was the base data : size plus REDOs. Small rowset compaction policy determined that it would : be good to compact these 8 rowsets, believing that it would produce 4 : rowsets of size 32MB. However, the rowsets were actually 32MB in size, : and compacting them produced 8 rowsets of size 32MB identical to the : previous 8, and therefore 8 rowsets that appeared to be of size 16MB to : compaction policy. Thus these 8 were chosen to be compacted, and so : on... Would be nice to create a test case that models this and shows that compaction won't loop anymore. -- To view, visit http://gerrit.cloudera.org:8080/12488 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 Gerrit-Change-Number: 12488 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 23:33:14 + Gerrit-HasComments: Yes
[kudu-CR] Enable --rowset metadata store keys by default
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12489 ) Change subject: Enable --rowset_metadata_store_keys by default .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12489/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12489/1//COMMIT_MSG@12 PS1, Line 12: I think we : should try turning this flag off (at the beginning of the 1.10 release : cycle) to see if it would be a good idea to have it on by default If we do end up deciding to keep this enabled, we may also want to consider removing the flag altogether unless we think there's a good use case for disabling this feature. -- To view, visit http://gerrit.cloudera.org:8080/12489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2c4ac4b58845666cd10102fb125fc787c637e473 Gerrit-Change-Number: 12489 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 23:04:27 + Gerrit-HasComments: Yes
[kudu-CR] Enable --rowset metadata store keys by default
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12489 ) Change subject: Enable --rowset_metadata_store_keys by default .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12489/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12489/1//COMMIT_MSG@21 PS1, Line 21: Expected cost: Won't this also defer more lazy initialization work (i.e. initializing the key cfiles) to the first scan? Not a bad thing per se, just something to be aware of. -- To view, visit http://gerrit.cloudera.org:8080/12489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2c4ac4b58845666cd10102fb125fc787c637e473 Gerrit-Change-Number: 12489 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 23:01:50 + Gerrit-HasComments: Yes
[kudu-CR] [tools] Add tool to list data dirs for a tablet
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12417 ) Change subject: [tools] Add tool to list data dirs for a tablet .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/12417/3/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: http://gerrit.cloudera.org:8080/#/c/12417/3/src/kudu/tools/tool_action_local_replica.cc@692 PS3, Line 692: // Load the tablet meta to make sure the tablet's data directories are loaded : // into the manager. > Ah, so we would get that dir printed in the error message, and everyone can Agh sorry, it would successfully call Load(), but it wouldn't register a group because some of the UUIDs are missing. So the error I think would come from FindDataDirsByTabletId(), saying there is no data dir group for the tablet. IIRC there will be a warning logged about why this is the case, so I think this is fine. -- To view, visit http://gerrit.cloudera.org:8080/12417 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6c6de565e4061eb8248b453d2230847e064040ff Gerrit-Change-Number: 12417 Gerrit-PatchSet: 4 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 14 Feb 2019 22:56:44 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2672: [spark] Optionally repartition to match Kudu partitions
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12484 )
Change subject: KUDU-2672: [spark] Optionally repartition to match Kudu
partitions
..
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12484/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12484/1//COMMIT_MSG@12
PS1, Line 12:
: Repartitioning ensures that one task/client is only
: writing to a single tablet. This improves throughput
: by improving batching especially for tables with a large
: number of partitions.
Have you had a chance to try this on a real cluster? What sort of improvement
should one expect?
http://gerrit.cloudera.org:8080/#/c/12484/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
File
java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala:
http://gerrit.cloudera.org:8080/#/c/12484/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala@385
PS1, Line 385: tables
table's
http://gerrit.cloudera.org:8080/#/c/12484/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala@386
PS1, Line 386: val keyedRdd = rdd.mapPartitions { rows =>
Can we hoist the openTable and/or KuduPartitionerBuilder.build() out of the
loop? Each one involves sending RPCs, and I don't see why the results can't be
shared across iterations of the loop.
Indeed, in the case of the partitioner, we _want_ to share state because we
want the source of truth for partitioning to be as close to an "atomic
snapshot" as possible. Not that the partitioner guarantees that, but using
multiple partitioners is likely to increase the window wherein there may be
disagreement about how the table is partitioned.
http://gerrit.cloudera.org:8080/#/c/12484/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala@389
PS1, Line 389: val partitioner = new
KuduPartitioner.KuduPartitionerBuilder(table).build()
Can we satisfy getPartitionCount() above using the table and partitioner we
build here?
http://gerrit.cloudera.org:8080/#/c/12484/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala@407
PS1, Line 407: val shuffledRDD = if (writeOptions.repartitionSort) {
I think Impala will automatically partial sort if the number of rows being
inserted is large enough. Maybe we should apply a similar heuristic here too,
overrideable by user input?
http://gerrit.cloudera.org:8080/#/c/12484/1/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala
File
java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala:
http://gerrit.cloudera.org:8080/#/c/12484/1/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala@454
PS1, Line 454: new KuduWriteOptions(repartition = true, repartitionSort =
true))
Should we also test with repartitionSort=false?
--
To view, visit http://gerrit.cloudera.org:8080/12484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8763615997bccc08901235841149fc3bacb321e7
Gerrit-Change-Number: 12484
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Thu, 14 Feb 2019 22:51:22 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@335
PS5, Line 335: struct ifaddrs *ifap;
: if (getifaddrs(&ifap) > -1) {
: SCOPED_CLEANUP({
: freeifaddrs(ifap);
: });
: for (struct ifaddrs *ifa = ifap; ifa; ifa = ifa->ifa_next) {
: if (ifa->ifa_addr == nullptr || ifa->ifa_netmask == nullptr
: || ifa->ifa_addr->sa_family != AF_INET)
: continue;
:
: struct sockaddr_in *addr_in = reinterpret_cast(ifa->ifa_addr);
: if
((NetworkByteOrder::FromHost32(addr_in->sin_addr.s_addr) >> 24) != 127) {
: char s[INET_ADDRSTRLEN];
: inet_ntop(AF_INET, &(addr_in->sin_addr), s,
INET_ADDRSTRLEN);
: FLAGS_local_ip_for_outbound_sockets = string(s,
arraysize(s));
: // Found and assigned an external IP.
: return true;
: }
: }
: }
> I would not be too much concerned with efficiency in this particular case b
I didn't even realize we had existing code for this in net_util.{h,cc]. Yes, we
should _definitely_ reuse that code. Efficiency isn't a concern in an
integration test like this.
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 5
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 14 Feb 2019 22:38:56 +
Gerrit-HasComments: Yes
[kudu-CR] Enable --rowset metadata store keys by default
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12489 ) Change subject: Enable --rowset_metadata_store_keys by default .. Patch Set 1: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/12489/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12489/1//COMMIT_MSG@13 PS1, Line 13: off on http://gerrit.cloudera.org:8080/#/c/12489/1//COMMIT_MSG@22 PS1, Line 22: metdata metadata -- To view, visit http://gerrit.cloudera.org:8080/12489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2c4ac4b58845666cd10102fb125fc787c637e473 Gerrit-Change-Number: 12489 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 22:47:52 + Gerrit-HasComments: Yes
[kudu-CR] [tools] Add tablet data dirs to 'kudu remote replica list'
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12449 ) Change subject: [tools] Add tablet data dirs to 'kudu remote replica list' .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/12449/1/src/kudu/tablet/tablet.proto File src/kudu/tablet/tablet.proto: http://gerrit.cloudera.org:8080/#/c/12449/1/src/kudu/tablet/tablet.proto@104 PS1, Line 104: bytes > Directory and file names can be damn near anything in *nix, IIRC. Protobuf Ah, good to know. http://gerrit.cloudera.org:8080/#/c/12449/1/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/12449/1/src/kudu/tablet/tablet_replica.cc@448 PS1, Line 448: -- > ? Minor grammar point, I've usually seen "--" used as: "in this case -- the state" http://gerrit.cloudera.org:8080/#/c/12449/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/12449/2/src/kudu/tools/kudu-tool-test.cc@1940 PS2, Line 1940: JoinStrings(tablet_status.data_dirs(), ", " Hrm, surprised this is the case. I would have thought the tombstone would have no data and hence no data dir group. -- To view, visit http://gerrit.cloudera.org:8080/12449 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7e509b5c080a76a4110a32ecc073418c28aa0633 Gerrit-Change-Number: 12449 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 14 Feb 2019 22:46:44 + Gerrit-HasComments: Yes
[kudu-CR] Enable --rowset metadata store keys by default
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12489 Change subject: Enable --rowset_metadata_store_keys by default .. Enable --rowset_metadata_store_keys by default While starting up a cluster where each tablet server had ~4000 replicas, I noticed many replicas spent seconds opening. I traced this to having to read the rowset bounds for each rowset from disk. It appears to be the only I/O that occurs when opening the tablet replica. I think we should try turning this flag off (at the beginning of the 1.10 release cycle) to see if it would be a good idea to have it on by default. We can always revert this change if there are problems. Expected benefit: 1. Significantly decreased startup time when there are a lot of replicas. See c127477b52 for some numbers. Expected cost: 1. Bigger tablet metdata files, and therefore bigger flushes of tablet metadata. Note that tablet metadata size is already (in part) proportional to the number of rowsets, so the metadata will not grow differently. Also: Good: This is backwards compatible-- old servers will ignore the new field and read the bounds from disk. Good: This is forwards compatible-- new servers will read the bounds from disk if the new field is missing. Bad: This won't affect rowsets that are already on disk, so read-only tablets written in previous versions will never see any benefit from this patch. Change-Id: I2c4ac4b58845666cd10102fb125fc787c637e473 --- M src/kudu/tablet/diskrowset.cc 1 file changed, 1 insertion(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/12489/1 -- To view, visit http://gerrit.cloudera.org:8080/12489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I2c4ac4b58845666cd10102fb125fc787c637e473 Gerrit-Change-Number: 12489 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley
[kudu-CR] Rename DeadlineTracker to TimeoutTracker
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12373 ) Change subject: Rename DeadlineTracker to TimeoutTracker .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3f465c925856390ecf4747e84bdd5a67c51c69eb Gerrit-Change-Number: 12373 Gerrit-PatchSet: 4 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 14 Feb 2019 22:31:47 + Gerrit-HasComments: No
[kudu-CR] KUDU-1868: Part 1: Add timer-based RPC timeouts
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12338 ) Change subject: KUDU-1868: Part 1: Add timer-based RPC timeouts .. Patch Set 8: Let's just stick with null for now to keep it simple. -- To view, visit http://gerrit.cloudera.org:8080/12338 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8d823b63ac0a41cc5e42b63a7c19e0ef777e1dea Gerrit-Change-Number: 12338 Gerrit-PatchSet: 8 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 14 Feb 2019 22:31:28 + Gerrit-HasComments: No
[kudu-CR] KUDU-1868: Part 1: Add timer-based RPC timeouts
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12338 ) Change subject: KUDU-1868: Part 1: Add timer-based RPC timeouts .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12338 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8d823b63ac0a41cc5e42b63a7c19e0ef777e1dea Gerrit-Change-Number: 12338 Gerrit-PatchSet: 8 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 14 Feb 2019 22:31:16 + Gerrit-HasComments: No
[kudu-CR] Fix compilation warnings under debian8.9
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12294 ) Change subject: Fix compilation warnings under debian8.9 .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/12294/3/src/kudu/consensus/leader_election.cc File src/kudu/consensus/leader_election.cc: http://gerrit.cloudera.org:8080/#/c/12294/3/src/kudu/consensus/leader_election.cc@162 PS3, Line 162: #pragma GCC diagnostic push Nit: add a blank line just above this, and the comment "Suppress false positive about 'decision' used while uninitialized." http://gerrit.cloudera.org:8080/#/c/12294/3/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/12294/3/src/kudu/tablet/cfile_set.cc@324 PS3, Line 324: // Disable compilation warnings. Nit: change comment to "Suppress false positive about 'rowid' used when uninitialized." (or 'opt_rowid', if that's where the warning was). -- To view, visit http://gerrit.cloudera.org:8080/12294 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2464ff7938f79d090ebd1676da2108ce7dae4ca5 Gerrit-Change-Number: 12294 Gerrit-PatchSet: 3 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu Gerrit-Comment-Date: Thu, 14 Feb 2019 22:28:50 + Gerrit-HasComments: Yes
[kudu-CR] [tools] Add tablet data dirs to 'kudu remote replica list'
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12449 ) Change subject: [tools] Add tablet data dirs to 'kudu remote replica list' .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12449 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7e509b5c080a76a4110a32ecc073418c28aa0633 Gerrit-Change-Number: 12449 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 14 Feb 2019 22:17:00 + Gerrit-HasComments: No
[kudu-CR] [tools] Add tool to list data dirs for a tablet
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12417 ) Change subject: [tools] Add tool to list data dirs for a tablet .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12417 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6c6de565e4061eb8248b453d2230847e064040ff Gerrit-Change-Number: 12417 Gerrit-PatchSet: 4 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 14 Feb 2019 22:15:54 + Gerrit-HasComments: No
[kudu-CR] [tools] Add useful flags to 'kudu remote replica list'
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12450 ) Change subject: [tools] Add useful flags to 'kudu remote_replica list' .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12450 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I462515f1bc3e8487185aebb6cb99d1c5c00cea40 Gerrit-Change-Number: 12450 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 14 Feb 2019 22:15:26 + Gerrit-HasComments: No
[kudu-CR] [tools] Add tool to list data dirs for a tablet
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12417 ) Change subject: [tools] Add tool to list data dirs for a tablet .. Patch Set 3: (1 comment) Alexey says the IWYU thing is fixed once I rebase. http://gerrit.cloudera.org:8080/#/c/12417/3/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: http://gerrit.cloudera.org:8080/#/c/12417/3/src/kudu/tools/tool_action_local_replica.cc@692 PS3, Line 692: // Load the tablet meta to make sure the tablet's data directories are loaded : // into the manager. > Missing data dirs, like failed dirs, won't break the FsManager. Rather, the Ah, so we would get that dir printed in the error message, and everyone can be happy? -- To view, visit http://gerrit.cloudera.org:8080/12417 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6c6de565e4061eb8248b453d2230847e064040ff Gerrit-Change-Number: 12417 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 14 Feb 2019 22:14:33 + Gerrit-HasComments: Yes
[kudu-CR] [tests] fix running tests under super-user
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12486 )
Change subject: [tests] fix running tests under super-user
..
Patch Set 2:
(2 comments)
FWIW, I don't think this is goofy; it lowers the barrier to running tests (if
for some reason one's dev environment uses root).
http://gerrit.cloudera.org:8080/#/c/12486/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12486/2//COMMIT_MSG@9
PS2, Line 9: That's
Nit: just 'That'
http://gerrit.cloudera.org:8080/#/c/12486/2/src/kudu/rpc/negotiation-test.cc
File src/kudu/rpc/negotiation-test.cc:
http://gerrit.cloudera.org:8080/#/c/12486/2/src/kudu/rpc/negotiation-test.cc@1169
PS2, Line 1169: if (geteuid() == 0) {
: // The super-user can acess the 'inaccessible' keytab file
anyway.
: ASSERT_TRUE(s.ok()) << s.ToString();
: } else {
: ASSERT_FALSE(s.ok()) << s.ToString();
Maybe cleaner as a ternary?
ASSERT_TRUE(geteuid() == 0 ? s.ok() : !s.ok()) << s.ToString();
Might also apply to the other test fixes.
--
To view, visit http://gerrit.cloudera.org:8080/12486
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I05bf220ee8000209a3e36faa9f21fd43ab8bdc9f
Gerrit-Change-Number: 12486
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 14 Feb 2019 22:14:03 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1868: Part 1: Add timer-based RPC timeouts
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12338 ) Change subject: KUDU-1868: Part 1: Add timer-based RPC timeouts .. Patch Set 8: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/12338 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8d823b63ac0a41cc5e42b63a7c19e0ef777e1dea Gerrit-Change-Number: 12338 Gerrit-PatchSet: 8 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 14 Feb 2019 22:10:56 + Gerrit-HasComments: No
[kudu-CR] KUDU-1868 Part 2: Eliminate socket read timeout except for negotiation
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12363 ) Change subject: KUDU-1868 Part 2: Eliminate socket read timeout except for negotiation .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/12363/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12363/4//COMMIT_MSG@19 PS4, Line 19: so I set the timeout to 1ms, which is much larger than the default : for the C++ client > It's possible to raise the server-side timeout. So, without adding plumbing That doesn't answer my question though. Let me make sure I understood what you're saying: 1. The server negotiation timeout defaults to 3s, and is configurable. 2. The C++ client negotiation timeout is 3s, and is static. 3. In this patch, the Java client negotiation timeout is set to 10s, and is also static. I'm wondering what makes the Java client special that it shouldn't be exactly the same as the C++ client. If you want to allow for breathing room that's fine, but shouldn't the C++ client also get the same breathing room? -- To view, visit http://gerrit.cloudera.org:8080/12363 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I391374dd72b6f4a91a9f69cf34758703afbdc59e Gerrit-Change-Number: 12363 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 14 Feb 2019 22:10:14 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2701 Fix compaction loop due to using wrong rowset size
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12488 Change subject: KUDU-2701 Fix compaction loop due to using wrong rowset size .. KUDU-2701 Fix compaction loop due to using wrong rowset size Compaction policy originally evaluated the size of a rowset to be the size of the rowset's base data and its REDOs. This size is used to calculate the probability mass of the rowset and the weight of the rowset in the compaction knapsack problem. Mistakenly, it was also used as the size of a rowset for KUDU-1400 small rowset compaction policy. This is wrong- the size of the whole rowset should be used. The reason for this is the following: small rowset compaction values compacting rowsets when the number of rowsets is reduced. The number of rowsets produced depends on the total size of the data when written. If a partial size of the rowset is used, this can lead to bad decisions being made about compaction. For example, I discovered a case where a tablet had 8 rowsets of "size" 16MB. In reality, this size was the base data size plus REDOs. Small rowset compaction policy determined that it would be good to compact these 8 rowsets, believing that it would produce 4 rowsets of size 32MB. However, the rowsets were actually 32MB in size, and compacting them produced 8 rowsets of size 32MB identical to the previous 8, and therefore 8 rowsets that appeared to be of size 16MB to compaction policy. Thus these 8 were chosen to be compacted, and so on... This patch changes the small rowset compaction policy code to use the full size of the rowsets. All other uses of size remain the same. It's not totally clear to me why just base data and REDOs are used, but in any case changing it would change CDFs and change knapsack calculations, which could lead to pretty big differences in compaction behavior. Since, outside this bug, compaction is working fine, and since this patch is targeted to unblock 1.9, I opted to make a minimal change to fix the bug and not evaluate any larger chance to compaction policy. Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 --- M src/kudu/tablet/compaction_policy.cc M src/kudu/tablet/rowset_info.cc M src/kudu/tablet/rowset_info.h 3 files changed, 28 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/12488/1 -- To view, visit http://gerrit.cloudera.org:8080/12488 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 Gerrit-Change-Number: 12488 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley
[kudu-CR] [tools] Add tool to list data dirs for a tablet
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12417 ) Change subject: [tools] Add tool to list data dirs for a tablet .. Patch Set 4: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/12417/3/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: http://gerrit.cloudera.org:8080/#/c/12417/3/src/kudu/tools/tool_action_local_replica.cc@692 PS3, Line 692: // Load the tablet meta to make sure the tablet's data directories are loaded : // into the manager. > The purpose of the tool is to figure out which data dirs hold data for the Missing data dirs, like failed dirs, won't break the FsManager. Rather, the TabletMetadata would fail to Load upon realizing that one of the data dir UUIDs it expected is missing. -- To view, visit http://gerrit.cloudera.org:8080/12417 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6c6de565e4061eb8248b453d2230847e064040ff Gerrit-Change-Number: 12417 Gerrit-PatchSet: 4 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 14 Feb 2019 21:51:41 + Gerrit-HasComments: Yes
[kudu-CR] [build-support] updated iwyu-dependent target name
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12485 ) Change subject: [build-support] updated iwyu-dependent target name .. [build-support] updated iwyu-dependent target name This is a follow-up to 4e2451dbf18db1faf9545ac1f9663d378b9f5efe. Change-Id: Icb1750ce71ab4933e536cbd53d4b371412d8ce62 Reviewed-on: http://gerrit.cloudera.org:8080/12485 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo --- M build-support/iwyu.py 1 file changed, 2 insertions(+), 2 deletions(-) Approvals: Kudu Jenkins: Verified Adar Dembo: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/12485 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Icb1750ce71ab4933e536cbd53d4b371412d8ce62 Gerrit-Change-Number: 12485 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Rename DeadlineTracker to TimeoutTracker
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12373 ) Change subject: Rename DeadlineTracker to TimeoutTracker .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3f465c925856390ecf4747e84bdd5a67c51c69eb Gerrit-Change-Number: 12373 Gerrit-PatchSet: 4 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 14 Feb 2019 21:46:18 + Gerrit-HasComments: No
[kudu-CR] [build-support] updated iwyu-dependent target name
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12485 ) Change subject: [build-support] updated iwyu-dependent target name .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12485 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icb1750ce71ab4933e536cbd53d4b371412d8ce62 Gerrit-Change-Number: 12485 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 21:45:03 + Gerrit-HasComments: No
[kudu-CR] KUDU-1900: add loopback check and test
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@406 PS5, Line 406: true > What is the combination of rpc_encrypt_loopback_connections, rpc_authentica The idea behind passing or not passing authn token over the connection is simple: if the issuer (or just the holder) of the token knows that the connection is confidential, the token can be passed over the connection. Otherwise, the holder of the token should not pass the token over the connection. The 'confidential' means that there is no risk of capturing the token by sniffers over the wire. And we should assume that all loopback connections are confidential, I think. I suggest to leave this as is now, and maybe address in a separate patch. -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 5 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 20:49:26 + Gerrit-HasComments: Yes
[kudu-CR] [tools] Add tablet data dirs to 'kudu remote replica list'
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12449 to look at the new patch set (#2). Change subject: [tools] Add tablet data dirs to 'kudu remote replica list' .. [tools] Add tablet data dirs to 'kudu remote replica list' This is useful to find out the data dirs for a tablet on a remote server while the server is running. For example, it could be used to investigate performance issues related to the devices on which the data for the tablet is stored. Change-Id: I7e509b5c080a76a4110a32ecc073418c28aa0633 --- M src/kudu/tablet/tablet.proto M src/kudu/tablet/tablet_replica.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_remote_replica.cc 4 files changed, 68 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/12449/2 -- To view, visit http://gerrit.cloudera.org:8080/12449 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7e509b5c080a76a4110a32ecc073418c28aa0633 Gerrit-Change-Number: 12449 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] [tools] Add useful flags to 'kudu remote replica list'
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12450 to look at the new patch set (#2). Change subject: [tools] Add useful flags to 'kudu remote_replica list' .. [tools] Add useful flags to 'kudu remote_replica list' 1. --include_schema: The ability to exclude schema. 2. --table_name and --tablets: The ability to filter on tablet id, table name. Change-Id: I462515f1bc3e8487185aebb6cb99d1c5c00cea40 --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_remote_replica.cc 2 files changed, 88 insertions(+), 24 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/50/12450/2 -- To view, visit http://gerrit.cloudera.org:8080/12450 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I462515f1bc3e8487185aebb6cb99d1c5c00cea40 Gerrit-Change-Number: 12450 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] [tools] Add tool to list data dirs for a tablet
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12417 to look at the new patch set (#4). Change subject: [tools] Add tool to list data dirs for a tablet .. [tools] Add tool to list data dirs for a tablet It's hard to find out what the data dirs are for a tablet, given its tablet id, but it might be useful to do so. For example, if a user is trying to correlate disk utilization to busy tablets, it's useful to know which tablets will cause disk activity on which devices. Change-Id: I6c6de565e4061eb8248b453d2230847e064040ff --- M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_local_replica.cc 4 files changed, 100 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/12417/4 -- To view, visit http://gerrit.cloudera.org:8080/12417 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6c6de565e4061eb8248b453d2230847e064040ff Gerrit-Change-Number: 12417 Gerrit-PatchSet: 4 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] [tools] Add tablet data dirs to 'kudu remote replica list'
Will Berkeley has posted comments on this change. (
http://gerrit.cloudera.org:8080/12449 )
Change subject: [tools] Add tablet data dirs to 'kudu remote replica list'
..
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12449/1/src/kudu/tablet/tablet.proto
File src/kudu/tablet/tablet.proto:
http://gerrit.cloudera.org:8080/#/c/12449/1/src/kudu/tablet/tablet.proto@104
PS1, Line 104: bytes
> Curious, why bytes and not string?
Directory and file names can be damn near anything in *nix, IIRC. Protobuf
strings must be UTF-8.
http://gerrit.cloudera.org:8080/#/c/12449/1/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:
http://gerrit.cloudera.org:8080/#/c/12449/1/src/kudu/tablet/tablet_replica.cc@448
PS1, Line 448: --
> nit: prepend a space
?
http://gerrit.cloudera.org:8080/#/c/12449/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/12449/1/src/kudu/tools/kudu-tool-test.cc@1892
PS1, Line 1892: ExternalMiniClusterOptions opts;
> You can set num_data_dirs to something more interesting to test the multi-d
Done
http://gerrit.cloudera.org:8080/#/c/12449/1/src/kudu/tools/kudu-tool-test.cc@1910
PS1, Line 1910:
: // Some fields like state or estimated on disk size may vary.
Just check a
: // few whose values we should know exactly.
: ASSERT_STR_CONTAINS(stdout,
: Substitute("Tablet id: $0",
tablet_status.tablet_id()));
: ASSERT_STR_CONTAINS(stdout,
: Substitute("Table name: $0",
workload.table_name()));
: ASSERT_STR_CONTAINS(stdout,
: Substitute("Data dirs: $0",
JoinStrings(tablet_status.data_dirs(), ", ")));
> Could you also tombstone the replica and run this?
Done
--
To view, visit http://gerrit.cloudera.org:8080/12449
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e509b5c080a76a4110a32ecc073418c28aa0633
Gerrit-Change-Number: 12449
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Thu, 14 Feb 2019 20:34:55 +
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add to 'kudu remote replica list' the ability to exclude schema and to filter on tablet id, table name
Will Berkeley has posted comments on this change. (
http://gerrit.cloudera.org:8080/12450 )
Change subject: [tools] Add to 'kudu remote_replica list' the ability to
exclude schema and to filter on tablet id, table name
..
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12450/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12450/1//COMMIT_MSG@7
PS1, Line 7: [tools] Add to 'kudu remote_replica list' the ability to exclude
schema and to filter on tablet id, table name
> Nit: wrap/rewrite?
Fine.
http://gerrit.cloudera.org:8080/#/c/12450/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/12450/1/src/kudu/tools/kudu-tool-test.cc@1926
PS1, Line 1926: no_need_schema_info
> include_schema=false
Done
http://gerrit.cloudera.org:8080/#/c/12450/1/src/kudu/tools/kudu-tool-test.cc@1935
PS1, Line 1935: // Test we lose the tablet when matching on the wrong
tablet id or the wrong
: // table name.
: string stdout;
: NO_FATALS(RunActionStdoutString(
: Substitute("remote_replica list $0 --table_name=foo",
ts_addr),
: &stdout));
: ASSERT_STR_NOT_CONTAINS(stdout,
: Substitute("Tablet id: $0",
:
tablet_status.tablet_id()));
: stdout.clear();
: NO_FATALS(RunActionStdoutString(
: Substitute("remote_replica list $0 --tablets=foo",
ts_addr),
: &stdout));
: ASSERT_STR_NOT_CONTAINS(stdout,
: Substitute("Tablet id: $0",
:
tablet_status.tablet_id()));
> Think it's worth testing for the cross of the two? e.g. `--table_name=http://gerrit.cloudera.org:8080/#/c/12450/1/src/kudu/tools/tool_action_remote_replica.cc
File src/kudu/tools/tool_action_remote_replica.cc:
http://gerrit.cloudera.org:8080/#/c/12450/1/src/kudu/tools/tool_action_remote_replica.cc@188
PS1, Line 188: te
> to
Done
http://gerrit.cloudera.org:8080/#/c/12450/1/src/kudu/tools/tool_action_remote_replica.cc@294
PS1, Line 294: std::unordered_set tablet_ids =
strings::Split(FLAGS_tablets, ",");
> Nit: using
Done
http://gerrit.cloudera.org:8080/#/c/12450/1/src/kudu/tools/tool_action_remote_replica.cc@464
PS1, Line 464: string(""),
: string("Comma-separated list of
tablet IDs used to "
:"filter the list of
replicas"))
> Why can't these go in the DECLARE?
They are overriding the defaults from the DEFINE for this tool action,
specifically. That can't be done in the DECLARE.
--
To view, visit http://gerrit.cloudera.org:8080/12450
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I462515f1bc3e8487185aebb6cb99d1c5c00cea40
Gerrit-Change-Number: 12450
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Thu, 14 Feb 2019 20:35:00 +
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add tool to list data dirs for a tablet
Will Berkeley has posted comments on this change. (
http://gerrit.cloudera.org:8080/12417 )
Change subject: [tools] Add tool to list data dirs for a tablet
..
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12417/3/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/12417/3/src/kudu/tools/kudu-tool-test.cc@1193
PS3, Line 1193: TabletMetadata::CreateNew(&fs, kTestTablet, kTestTableName,
kTestTableId,
> Should ASSERT_OK on this.
Done
http://gerrit.cloudera.org:8080/#/c/12417/3/src/kudu/tools/kudu-tool-test.cc@1199
PS3, Line 1199: NO_FATALS(RunActionStdoutString(Substitute("local_replica
dump data_dirs $0 "
: "--fs_wal_dir=$1 "
:
"--fs_data_dirs=$2",
: kTestTablet,
kTestDir,
: kTestDir),
&stdout));
> Might be nice to run this on a tablet that doesn't exist if you haven't alr
There's an error about missing tablet metadata in that case.
http://gerrit.cloudera.org:8080/#/c/12417/3/src/kudu/tools/kudu-tool-test.cc@1204
PS3, Line 1204: const auto data_roots = fs.GetDataRootDirs();
: ASSERT_EQ(1, data_roots.size());
: ASSERT_EQ(data_roots[0], stdout);
> Could you test on multiple data directories?
Done
http://gerrit.cloudera.org:8080/#/c/12417/3/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
http://gerrit.cloudera.org:8080/#/c/12417/3/src/kudu/tools/tool_action_local_replica.cc@692
PS3, Line 692: // Load the tablet meta to make sure the tablet's data
directories are loaded
: // into the manager.
> Will it fail if a data dir is missing? Is that even desirable in this case?
The purpose of the tool is to figure out which data dirs hold data for the
tablet, so activity on data dirs can be correlated with activity on the tablet.
It's not a diagnostic tool to tell what's missing and why. I think the tool
would fail because it wouldn't be able to open the FsManager in that case?
--
To view, visit http://gerrit.cloudera.org:8080/12417
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6c6de565e4061eb8248b453d2230847e064040ff
Gerrit-Change-Number: 12417
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Thu, 14 Feb 2019 20:34:52 +
Gerrit-HasComments: Yes
[kudu-CR] [tests] fix running tests under super-user
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12486 Change subject: [tests] fix running tests under super-user .. [tests] fix running tests under super-user That's sounds goofy, but recently I found myself running Kudu tests as a super-user on a VM. A couple of test failed, and this patch fixes that. This is a test-only changelist, it doesn't change any functionality. Change-Id: I05bf220ee8000209a3e36faa9f21fd43ab8bdc9f --- M src/kudu/rpc/negotiation-test.cc M src/kudu/util/env-test.cc 2 files changed, 21 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/12486/1 -- To view, visit http://gerrit.cloudera.org:8080/12486 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I05bf220ee8000209a3e36faa9f21fd43ab8bdc9f Gerrit-Change-Number: 12486 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
[kudu-CR] [build-support] updated iwyu-dependent target name
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12485 Change subject: [build-support] updated iwyu-dependent target name .. [build-support] updated iwyu-dependent target name This is a follow-up to 4e2451dbf18db1faf9545ac1f9663d378b9f5efe. Change-Id: Icb1750ce71ab4933e536cbd53d4b371412d8ce62 --- M build-support/iwyu.py 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/12485/1 -- To view, visit http://gerrit.cloudera.org:8080/12485 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Icb1750ce71ab4933e536cbd53d4b371412d8ce62 Gerrit-Change-Number: 12485 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@406 PS5, Line 406: true > Something is fishy here: if in case of encrypted loopback connection betwee What is the combination of rpc_encrypt_loopback_connections, rpc_authentication and rpc_encryption flags that will result in authn token not being passed from local connection to local connection? -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 5 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 20:25:08 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1868: Part 1: Add timer-based RPC timeouts
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12338 ) Change subject: KUDU-1868: Part 1: Add timer-based RPC timeouts .. Patch Set 8: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/12338 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8d823b63ac0a41cc5e42b63a7c19e0ef777e1dea Gerrit-Change-Number: 12338 Gerrit-PatchSet: 8 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 14 Feb 2019 19:54:36 + Gerrit-HasComments: No
[kudu-CR] KUDU-1868: Part 1: Add timer-based RPC timeouts
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12338 ) Change subject: KUDU-1868: Part 1: Add timer-based RPC timeouts .. Patch Set 5: (1 comment) I think you need to rebase due to my KuduPartitioner patch. Apologies for that. http://gerrit.cloudera.org:8080/#/c/12338/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: http://gerrit.cloudera.org:8080/#/c/12338/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2377 PS5, Line 2377: return null; > We can't schedule a timeout on the timer, so we'd need to implement a Faile Yes, this is what I am advocating for. It's a small wrapper that would prevent future NPEs. If FailedTimeout already existed I suspect you would use it. The downside to adding this is about 30 lines of trivial code, the downside to not adding it is wrapping ever call to cancel in a null check and NPEs where we forget to do that. I feel strong enough about it that I am happy to do this in a follow up patch. -- To view, visit http://gerrit.cloudera.org:8080/12338 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8d823b63ac0a41cc5e42b63a7c19e0ef777e1dea Gerrit-Change-Number: 12338 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 14 Feb 2019 19:54:27 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@335
PS5, Line 335: struct ifaddrs *ifap;
: if (getifaddrs(&ifap) > -1) {
: SCOPED_CLEANUP({
: freeifaddrs(ifap);
: });
: for (struct ifaddrs *ifa = ifap; ifa; ifa = ifa->ifa_next) {
: if (ifa->ifa_addr == nullptr || ifa->ifa_netmask == nullptr
: || ifa->ifa_addr->sa_family != AF_INET)
: continue;
:
: struct sockaddr_in *addr_in = reinterpret_cast(ifa->ifa_addr);
: if
((NetworkByteOrder::FromHost32(addr_in->sin_addr.s_addr) >> 24) != 127) {
: char s[INET_ADDRSTRLEN];
: inet_ntop(AF_INET, &(addr_in->sin_addr), s,
INET_ADDRSTRLEN);
: FLAGS_local_ip_for_outbound_sockets = string(s,
arraysize(s));
: // Found and assigned an external IP.
: return true;
: }
: }
: }
> Yes, but it will be less efficient, because GetLocalNetworks will loop thro
I would not be too much concerned with efficiency in this particular case
because it's just a test. However, if you feel strong about this, maybe think
about making it more generic and moving into net_util.{h,cc}.
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@375
PS5, Line 375: encrypted
> So, that's something I would like to ask you back :) When we set --rpc_encr
Effectively, the application of the --rpc_encrypt_loopback_connections=true
flag is gated by the -rpc_encryption=disabled setting (see
shttps://github.com/apache/kudu/blob/4e2451dbf18db1faf9545ac1f9663d378b9f5efe/src/kudu/rpc/server_negotiation.cc#L509)
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@406
PS5, Line 406: true
> hm... yes, there is something wrong here. Token should not be passed here
Something is fishy here: if in case of encrypted loopback connection between
the client and the server the authz token is not present (as I can see it from
line 403), then it's a bit surprising that the token is present in this case.
At least, it would be nice to add some comment explaining why that's the case.
And it seems I know why:
https://github.com/apache/kudu/blob/4e2451dbf18db1faf9545ac1f9663d378b9f5efe/src/kudu/rpc/negotiation.cc#L279
Probably, that's a bug. Let's just add a comment and move further -- I think
it's worth addressing that issue in a separate changelist.
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 5
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 14 Feb 2019 19:48:22 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1868: Part 1: Add timer-based RPC timeouts
Hello Mike Percy, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12338 to look at the new patch set (#8). Change subject: KUDU-1868: Part 1: Add timer-based RPC timeouts .. KUDU-1868: Part 1: Add timer-based RPC timeouts Currently, the Java client requires some kind of event to detect the timeout of an RPC: either a response from the server in the chain of sub-RPCs or a socket read timeout on the connection. This patch adds a timer task to actively time out an RPC once it passes its deadline. Part 2 will eliminate socket read timeouts from the Java client, except possibly in the case of negotiation, which will fully resolve KUDU-1868. There is one test included, which checks that timeouts occur without an "outside stimulus" like a response from the server. This patch should not degrade the performance of the client. Even though every timer task holds a reference to its RPC, when the RPC completes it cancels the timer task, which will make the timer release it at the next tick. This means the RPC and its task should be available to be GC'd after the next tick of the timer. Change-Id: I8d823b63ac0a41cc5e42b63a7c19e0ef777e1dea --- M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java M java/kudu-client/src/main/java/org/apache/kudu/client/BatchResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToMasterRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/IsAlterTableDoneRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java M java/kudu-client/src/main/java/org/apache/kudu/client/ListTablesRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/ListTablesResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/ListTabletServersRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/ListTabletServersResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/ListTabletsRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/ListTabletsResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java M java/kudu-client/src/main/java/org/apache/kudu/client/OperationResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/PingRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java M java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java 33 files changed, 402 insertions(+), 181 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/12338/8 -- To view, visit http://gerrit.cloudera.org:8080/12338 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8d823b63ac0a41cc5e42b63a7c19e0ef777e1dea Gerrit-Change-Number: 12338 Gerrit-PatchSet: 8 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@406 PS5, Line 406: true > No, this is not a typo. This test passes. A token is passed over unencrypte hm... yes, there is something wrong here. Token should not be passed here -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 5 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 19:15:11 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1868: Part 1: Add timer-based RPC timeouts
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12338 ) Change subject: KUDU-1868: Part 1: Add timer-based RPC timeouts .. Patch Set 8: (5 comments) http://gerrit.cloudera.org:8080/#/c/12338/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: http://gerrit.cloudera.org:8080/#/c/12338/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2366 PS5, Line 2366: Preconditions.checkNotNull(timer); > Should passing a null timer be allowed? Done http://gerrit.cloudera.org:8080/#/c/12338/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2366 PS5, Line 2366: Preconditions.checkNotNull(timer); > PreConditions.checkNotNull() seems reasonable here Done http://gerrit.cloudera.org:8080/#/c/12338/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2377 PS5, Line 2377: > It sounds like the only time this could happen you don't expect the user to We can't schedule a timeout on the timer, so we'd need to implement a FailedTimeout class and provide an instance (maybe a singleton). Does that seem worth it? I still can't say I buy that returning null from here to indicate failure it such a problem. http://gerrit.cloudera.org:8080/#/c/12338/7/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java: http://gerrit.cloudera.org:8080/#/c/12338/7/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java@161 PS7, Line 161: this.timeoutTask = AsyncKuduClient.newTimeout(timer, : new RpcTimeout > Nit: indentation Done http://gerrit.cloudera.org:8080/#/c/12338/7/java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java: http://gerrit.cloudera.org:8080/#/c/12338/7/java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java@115 PS7, Line 115: : // The serve > Got an extra 'time' here. Also some of these lines may be too long; wrap? Done -- To view, visit http://gerrit.cloudera.org:8080/12338 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8d823b63ac0a41cc5e42b63a7c19e0ef777e1dea Gerrit-Change-Number: 12338 Gerrit-PatchSet: 8 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 14 Feb 2019 19:14:58 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1868 Part 2: Eliminate socket read timeout except for negotiation
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12363 ) Change subject: KUDU-1868 Part 2: Eliminate socket read timeout except for negotiation .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/12363/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12363/4//COMMIT_MSG@19 PS4, Line 19: so I set the timeout to 1ms, which is much larger than the default : for the C++ client > Not clear why you used 10s and not 3s if that's what the C++ client uses; w It's possible to raise the server-side timeout. So, without adding plumbing to allow people to raise the client-side timeout, leaving some breathing room for the timeout to be raised seemed reasonable. http://gerrit.cloudera.org:8080/#/c/12363/4/java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java File java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java: http://gerrit.cloudera.org:8080/#/c/12363/4/java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java@90 PS4, Line 90: table = harness.getClient().createTable(TABLE_NAME, getBasicSchema(), builder); : } : > Don't need this anymore; just use the client that's provided by the harness Done -- To view, visit http://gerrit.cloudera.org:8080/12363 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I391374dd72b6f4a91a9f69cf34758703afbdc59e Gerrit-Change-Number: 12363 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 14 Feb 2019 19:14:51 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1868 Part 2: Eliminate socket read timeout except for negotiation
Hello Mike Percy, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12363 to look at the new patch set (#5). Change subject: KUDU-1868 Part 2: Eliminate socket read timeout except for negotiation .. KUDU-1868 Part 2: Eliminate socket read timeout except for negotiation This removes all use of socket read timeouts from the Java client and deprecates all public APIs involving the socket read timeout. The Java client does not use socket read timeouts anymore except to time out negotiation. This is OK because negotiation is the only activity on the socket when negotiation occurs. Once negotiation succeeds, the socket read timeout handler is removed from the pipeline, and the timeout tasks from Part 1 handle RPC timeouts. The C++ client does not allow setting the negotiation timeout, and I chose not to expose it in the Java client because it didn't seem useful, so I set the timeout to 1ms, which is much larger than the default for the C++ client. The C++ client uses 3000ms, which is the server-side default. The server-side default is also 3000ms, so I left some room for the server-side default to rise. I wrote a test case and checked that negotiation did time out when the constant was lowered to a small number for the purposes of the test. Change-Id: I391374dd72b6f4a91a9f69cf34758703afbdc59e --- M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/CommandLineParser.java M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala 12 files changed, 74 insertions(+), 147 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/12363/5 -- To view, visit http://gerrit.cloudera.org:8080/12363 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I391374dd72b6f4a91a9f69cf34758703afbdc59e Gerrit-Change-Number: 12363 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] Rename DeadlineTracker to TimeoutTracker
Hello Mike Percy, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12373 to look at the new patch set (#4). Change subject: Rename DeadlineTracker to TimeoutTracker .. Rename DeadlineTracker to TimeoutTracker A deadline is the latest time by which something should be completed. It's an instant, like "next Tuesday at noon". A timeout is an interval of time allowed for some event to occur or be completed. It's a delta, like "15 minutes". The DeadlineTracker tracked a "relative deadline", relative to when the instance's "deadline" was set. That's actually a timeout. This patch harmonizes the names to the concepts. Change-Id: I3f465c925856390ecf4747e84bdd5a67c51c69eb --- M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java M java/kudu-client/src/main/java/org/apache/kudu/client/BatchResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableRequest.java D java/kudu-client/src/main/java/org/apache/kudu/client/DeadlineTracker.java M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/IsAlterTableDoneRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java M java/kudu-client/src/main/java/org/apache/kudu/client/ListTablesRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/ListTabletServersRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/ListTabletsRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java M java/kudu-client/src/main/java/org/apache/kudu/client/PingRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java A java/kudu-client/src/main/java/org/apache/kudu/client/TimeoutTracker.java M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java R java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeoutTracker.java M java/kudu-test-utils/src/main/java/org/apache/kudu/test/KuduTestHarness.java M java/kudu-test-utils/src/test/java/org/apache/kudu/test/TestMiniKuduCluster.java 25 files changed, 223 insertions(+), 224 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/12373/4 -- To view, visit http://gerrit.cloudera.org:8080/12373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3f465c925856390ecf4747e84bdd5a67c51c69eb Gerrit-Change-Number: 12373 Gerrit-PatchSet: 4 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] Rename DeadlineTracker to TimeoutTracker
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12373 ) Change subject: Rename DeadlineTracker to TimeoutTracker .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/12373/3/java/kudu-test-utils/src/main/java/org/apache/kudu/test/KuduTestHarness.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/KuduTestHarness.java: http://gerrit.cloudera.org:8080/#/c/12373/3/java/kudu-test-utils/src/main/java/org/apache/kudu/test/KuduTestHarness.java@232 PS3, Line 232: org.apache.kudu.client.TimeoutTracker timeoutTracker = new org.apache.kudu.client.TimeoutTracker(); > Import? Done http://gerrit.cloudera.org:8080/#/c/12373/3/java/kudu-test-utils/src/test/java/org/apache/kudu/test/TestMiniKuduCluster.java File java/kudu-test-utils/src/test/java/org/apache/kudu/test/TestMiniKuduCluster.java: http://gerrit.cloudera.org:8080/#/c/12373/3/java/kudu-test-utils/src/test/java/org/apache/kudu/test/TestMiniKuduCluster.java@118 PS3, Line 118: org.apache.kudu.client.TimeoutTracker tracker = new org.apache.kudu.client.TimeoutTracker(); > Import. Done -- To view, visit http://gerrit.cloudera.org:8080/12373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3f465c925856390ecf4747e84bdd5a67c51c69eb Gerrit-Change-Number: 12373 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 14 Feb 2019 19:14:41 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@406 PS5, Line 406: true > Is this typo? No, this is not a typo. This test passes. A token is passed over unencrypted local connection from 127.0.0.0 (client) to 127.x.x.x (mini cluster) -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 5 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 18:36:57 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@375
PS5, Line 375: encrypted
> Wait, but what about { BindMode::LOOPBACK, "disabled", "disabled", true, f
So, that's something I would like to ask you back :) When we set
--rpc_encryption=disabled and --rpc_encrypt_loopback_connections=true will a
loopback connection be encrypted?
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 5
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 14 Feb 2019 18:33:04 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/5/src/kudu/integration-tests/security-itest.cc@335
PS5, Line 335: struct ifaddrs *ifap;
: if (getifaddrs(&ifap) > -1) {
: SCOPED_CLEANUP({
: freeifaddrs(ifap);
: });
: for (struct ifaddrs *ifa = ifap; ifa; ifa = ifa->ifa_next) {
: if (ifa->ifa_addr == nullptr || ifa->ifa_netmask == nullptr
: || ifa->ifa_addr->sa_family != AF_INET)
: continue;
:
: struct sockaddr_in *addr_in = reinterpret_cast(ifa->ifa_addr);
: if
((NetworkByteOrder::FromHost32(addr_in->sin_addr.s_addr) >> 24) != 127) {
: char s[INET_ADDRSTRLEN];
: inet_ntop(AF_INET, &(addr_in->sin_addr), s,
INET_ADDRSTRLEN);
: FLAGS_local_ip_for_outbound_sockets = string(s,
arraysize(s));
: // Found and assigned an external IP.
: return true;
: }
: }
: }
> Is it possible to call kudu::GetLocalNetworks() and then just work with the
Yes, but it will be less efficient, because GetLocalNetworks will loop through
all local interfaces and put them into an array, then this test will have to
loop through the array and find non-loopback interfaces. At the same time, we
are talking about the efficiency of 2-3 repetitions, so I don't have a strong
opinion here
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 5
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 14 Feb 2019 18:30:19 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2672: Spark write to kudu, too many machines write to one tserver.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12341 ) Change subject: KUDU-2672: Spark write to kudu, too many machines write to one tserver. .. Patch Set 1: Also a word of warning. I found a spark bug while working on my patch and needed to fix our use of InternalRow to prevent data corruption/loss. You may want to check if this code has that issue: https://issues.apache.org/jira/browse/SPARK-26880 -- To view, visit http://gerrit.cloudera.org:8080/12341 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic6e0df2d43e93dc60d0b5c68daf91eb75b8d3bc7 Gerrit-Change-Number: 12341 Gerrit-PatchSet: 1 Gerrit-Owner: yangz Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 18:20:33 + Gerrit-HasComments: No
[kudu-CR] KUDU-2672: Spark write to kudu, too many machines write to one tserver.
Grant Henke has removed a vote on this change. Change subject: KUDU-2672: Spark write to kudu, too many machines write to one tserver. .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/12341 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Ic6e0df2d43e93dc60d0b5c68daf91eb75b8d3bc7 Gerrit-Change-Number: 12341 Gerrit-PatchSet: 1 Gerrit-Owner: yangz Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2672: Spark write to kudu, too many machines write to one tserver.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12341 ) Change subject: KUDU-2672: Spark write to kudu, too many machines write to one tserver. .. Patch Set 1: I hope you don't mind. Given the KuduPartitioner patch was committed and I haven't received any response on my comments, I implemented the repartitioning part of this patch here: https://gerrit.cloudera.org/#/c/12484/ -- To view, visit http://gerrit.cloudera.org:8080/12341 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic6e0df2d43e93dc60d0b5c68daf91eb75b8d3bc7 Gerrit-Change-Number: 12341 Gerrit-PatchSet: 1 Gerrit-Owner: yangz Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Feb 2019 18:18:17 + Gerrit-HasComments: No
[kudu-CR] KUDU-2672: [spark] Optionally repartition to match Kudu partitions
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12484 Change subject: KUDU-2672: [spark] Optionally repartition to match Kudu partitions .. KUDU-2672: [spark] Optionally repartition to match Kudu partitions Adds a write option to repartition the data to match the target Kudu partitions. Additionally provides the option to sort while repartitioning. Repartitioning ensures that one task/client is only writing to a single tablet. This improves throughput by improving batching especially for tables with a large number of partitions. Additionally sorting before writing to Kudu reduces the amount of compactions needed and can improve sustained throughput. Change-Id: I8763615997bccc08901235841149fc3bacb321e7 --- M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduWriteOptions.scala M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/RowConverter.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala 5 files changed, 183 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/12484/1 -- To view, visit http://gerrit.cloudera.org:8080/12484 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I8763615997bccc08901235841149fc3bacb321e7 Gerrit-Change-Number: 12484 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] Fix compilation warnings under debian8.9
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12294 to look at the new patch set (#3). Change subject: Fix compilation warnings under debian8.9 .. Fix compilation warnings under debian8.9 It seems there are some compilation warnings under debian8.9, with gcc 4.9.2/5.5.0, and the warning info is formatted as follows: "warning: ‘xxx’ may be used uninitialized in this function" Change-Id: I2464ff7938f79d090ebd1676da2108ce7dae4ca5 --- M src/kudu/cfile/bshuf_block.cc M src/kudu/cfile/rle_block.h M src/kudu/common/row_operations.cc M src/kudu/common/table_util.cc M src/kudu/consensus/leader_election.cc M src/kudu/consensus/log-test.cc M src/kudu/fs/log_block_manager-test-util.cc M src/kudu/hms/hms_client-test.cc M src/kudu/integration-tests/log_verifier.cc M src/kudu/rpc/server_negotiation.cc M src/kudu/server/tracing_path_handlers.cc M src/kudu/tablet/cfile_set.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/tablet-test-util.h M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_perf.cc M src/kudu/tserver/mini_tablet_server-test.cc M src/kudu/util/debug/trace_event_impl.cc M src/kudu/util/net/sockaddr.cc M src/kudu/util/pb_util.cc M src/kudu/util/status.cc 22 files changed, 43 insertions(+), 24 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/12294/3 -- To view, visit http://gerrit.cloudera.org:8080/12294 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2464ff7938f79d090ebd1676da2108ce7dae4ca5 Gerrit-Change-Number: 12294 Gerrit-PatchSet: 3 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu
[kudu-CR] Fix compilation warnings under debian8.9
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12294 ) Change subject: Fix compilation warnings under debian8.9 .. Patch Set 2: "and the same to the other files" means: src/kudu/consensus/log-test.cc src/kudu/rpc/server_negotiation.cc -- To view, visit http://gerrit.cloudera.org:8080/12294 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2464ff7938f79d090ebd1676da2108ce7dae4ca5 Gerrit-Change-Number: 12294 Gerrit-PatchSet: 2 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu Gerrit-Comment-Date: Thu, 14 Feb 2019 11:05:48 + Gerrit-HasComments: No
[kudu-CR] Fix compilation warnings under debian8.9
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12294 ) Change subject: Fix compilation warnings under debian8.9 .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/cfile/bshuf_block.cc File src/kudu/cfile/bshuf_block.cc: http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/cfile/bshuf_block.cc@99 PS1, Line 99: uint32_t mid_key = 0; > I don't think this has been addressed yet. sorry, it's my mistake. i modified it locally but not uploaded to the gerrit after branch switching, and the same to the other files :( http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/consensus/leader_election.cc File src/kudu/consensus/leader_election.cc: http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/consensus/leader_election.cc@304 PS1, Line 304: CHECK_OK(vote_counter_->GetDecision(&decision)); > PS2 still shows a default value of VOTE_DENIED instead of suppression of th Done http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/12294/1/src/kudu/tablet/cfile_set.cc@320 PS1, Line 320: boost::optional opt_rowid; > Why not add the suppression though? Using this: Done -- To view, visit http://gerrit.cloudera.org:8080/12294 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2464ff7938f79d090ebd1676da2108ce7dae4ca5 Gerrit-Change-Number: 12294 Gerrit-PatchSet: 2 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu Gerrit-Comment-Date: Thu, 14 Feb 2019 11:04:30 + Gerrit-HasComments: Yes
