[kudu-CR] [docs]: Delete invalid link
Adar Dembo has removed a vote on this change. Change subject: [docs]: Delete invalid link .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/13881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Ic61282e41eb2cd6ee4371d0ca2c7e42b9ccb4ee4 Gerrit-Change-Number: 13881 Gerrit-PatchSet: 1 Gerrit-Owner: XiaokaiWang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: XiaokaiWang Gerrit-Reviewer: Yao Xu
[kudu-CR] [docs]: Delete invalid link
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13881 ) Change subject: [docs]: Delete invalid link .. Patch Set 1: Verified+1 Code-Review+2 > Hmm, this will return back to 'Dictionary encoding' title, but actually that > is where we are reading. Is this necessary? Agreed; a link referring back to the same section isn't useful. Overriding Jenkins; the ASAN failure is unrelated to this patch. -- To view, visit http://gerrit.cloudera.org:8080/13881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic61282e41eb2cd6ee4371d0ca2c7e42b9ccb4ee4 Gerrit-Change-Number: 13881 Gerrit-PatchSet: 1 Gerrit-Owner: XiaokaiWang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: XiaokaiWang Gerrit-Reviewer: Yao Xu Gerrit-Comment-Date: Tue, 23 Jul 2019 03:24:50 + Gerrit-HasComments: No
[kudu-CR] WIP KUDU-2780: create thread for auto-rebalancing (1/n)
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/13891 )
Change subject: WIP KUDU-2780: create thread for auto-rebalancing (1/n)
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer-test.cc
File src/kudu/master/autorebalancer-test.cc:
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer-test.cc@69
PS1, Line 69: TEST_F(AutoRebalancerTest, ViewReports) {
> This test keeps a cluster around for at least a couple polling intervals, a
For initial POC testing that approach might be OK. However, in the long run
the tests should provide coverage to automatically verify the required
functionality: nobody is going to peer at the output of those tests unless
something is reported to be broken.
Probably, we want to represent auto-relabancer stats as a set of metrics which
are programmatically retrievable from master. As for the latter, in Kudu there
is a standardized way to do so: take a look at so-called metric counters and
gauges: grep for METRIC_DEFINE_counter and METRIC_DEFINE_gauge_int64 in the
code and see how it's used there and in corresponding tests (grep for
METRIC_DECLARE_counter macros and FindOrCreateCounter). From what I remember,
I recently worked with those somewhere in src/kudu/util/ttl_cache.h and
src/kudu/util/ttl_cache-test.cc
In addition to be able to retrieve the metrics from within a process, those are
exposed via debug web server as well, so it's possible to retrieve them in JSON
format via HTTP.
--
To view, visit http://gerrit.cloudera.org:8080/13891
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc829d14dc8636287bf3a4faa4ba76e86ab6885c
Gerrit-Change-Number: 13891
Gerrit-PatchSet: 1
Gerrit-Owner: Hannah Nguyen
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Hannah Nguyen
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 23 Jul 2019 02:55:27 +
Gerrit-HasComments: Yes
[kudu-CR] WIP KUDU-2780: create thread for auto-rebalancing (1/n)
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13891 to look at the new patch set (#4). Change subject: WIP KUDU-2780: create thread for auto-rebalancing (1/n) .. WIP KUDU-2780: create thread for auto-rebalancing (1/n) AutoRebalancerTask is a thread in the master with a configurable polling interval. Every interval, the thread logs the current skew of the cluster if auto-rebalancing is enabled. Code that is modified/copied directly from the CLI rebalancing tool in kudu/tools is marked. The task currently calls the 'ksck' tool to determine skew, but this will change in later patches when rebalancing is implemented and the task retrieves information directly from the master. Change-Id: Ibc829d14dc8636287bf3a4faa4ba76e86ab6885c --- M src/kudu/benchmarks/tpch/tpch1.cc M src/kudu/master/CMakeLists.txt A src/kudu/master/autorebalancer-test.cc A src/kudu/master/autorebalancer.cc A src/kudu/master/autorebalancer.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h 7 files changed, 1,753 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/13891/4 -- To view, visit http://gerrit.cloudera.org:8080/13891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibc829d14dc8636287bf3a4faa4ba76e86ab6885c Gerrit-Change-Number: 13891 Gerrit-PatchSet: 4 Gerrit-Owner: Hannah Nguyen Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hannah Nguyen Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] WIP KUDU-2780: create thread for auto-rebalancing (1/n)
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13891 to look at the new patch set (#3). Change subject: WIP KUDU-2780: create thread for auto-rebalancing (1/n) .. WIP KUDU-2780: create thread for auto-rebalancing (1/n) AutoRebalancerTask is a thread in the master with a configurable polling interval. Every interval, the thread logs the current skew of the cluster if auto-rebalancing is enabled. Code that is modified/copied directly from the CLI rebalancing tool in kudu/tools is marked. The task currently calls the 'ksck' tool to determine skew, but this will change in later patches when rebalancing is implemented and the task retrieves information directly from the master. Change-Id: Ibc829d14dc8636287bf3a4faa4ba76e86ab6885c --- M src/kudu/benchmarks/tpch/tpch1.cc M src/kudu/master/CMakeLists.txt A src/kudu/master/autorebalancer-test.cc A src/kudu/master/autorebalancer.cc A src/kudu/master/autorebalancer.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h 7 files changed, 1,753 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/13891/3 -- To view, visit http://gerrit.cloudera.org:8080/13891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibc829d14dc8636287bf3a4faa4ba76e86ab6885c Gerrit-Change-Number: 13891 Gerrit-PatchSet: 3 Gerrit-Owner: Hannah Nguyen Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hannah Nguyen Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] WIP KUDU-2780: create thread for auto-rebalancing (1/n)
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13891 to look at the new patch set (#2). Change subject: WIP KUDU-2780: create thread for auto-rebalancing (1/n) .. WIP KUDU-2780: create thread for auto-rebalancing (1/n) AutoRebalancerTask is a thread in the master with a configurable polling interval. Every interval, the thread logs the current skew of the cluster if auto-rebalancing is enabled. Code that is modified/copied directly from the CLI rebalancing tool in kudu/tools is marked. The task currently calls the 'ksck' tool to determine skew, but this will change in later patches when rebalancing is implemented and the task retrieves information directly from the master. Change-Id: Ibc829d14dc8636287bf3a4faa4ba76e86ab6885c --- M src/kudu/benchmarks/tpch/tpch1.cc M src/kudu/master/CMakeLists.txt A src/kudu/master/autorebalancer-test.cc A src/kudu/master/autorebalancer.cc A src/kudu/master/autorebalancer.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h 7 files changed, 1,753 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/13891/2 -- To view, visit http://gerrit.cloudera.org:8080/13891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibc829d14dc8636287bf3a4faa4ba76e86ab6885c Gerrit-Change-Number: 13891 Gerrit-PatchSet: 2 Gerrit-Owner: Hannah Nguyen Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hannah Nguyen Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2780: create thread for auto-rebalancing (1/n)
Hannah Nguyen has posted comments on this change. (
http://gerrit.cloudera.org:8080/13891 )
Change subject: KUDU-2780: create thread for auto-rebalancing (1/n)
..
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13891/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13891/1//COMMIT_MSG@13
PS1, Line 13: Code that is modified/copied directly from the CLI rebalancing
: tool in kudu/tools is marked.
> Did you consider separating the rebalancing piece that doesn't depend on th
Hmmm, the point of this patch was mostly to lay the infrastructure (create the
Task in the master) by implementing some code that would log the current skew
of the cluster.
I don't think this patch will be merge-able, but the next one (which will
migrate any common code into the master directory, and retrieve information
from the master and not call 'ksck' at all) should be.
Will clarify this in the message.
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/CMakeLists.txt
File src/kudu/master/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/CMakeLists.txt@61
PS1, Line 61: ksck
> This introduce a cycling dependency which is a no-no:
Will refactor the auto-rebalancer to not call 'ksck' at all.
Then moving common code into the master should also be ok, since tools already
depends on the master directory.
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer-test.cc
File src/kudu/master/autorebalancer-test.cc:
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer-test.cc@55
PS1, Line 55: TEST_F(AutoRebalancerTest, NoReports) {
> We have many tests which start master and does some work before shutting it
Agreed, this test isn't necessary.
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer-test.cc@69
PS1, Line 69: TEST_F(AutoRebalancerTest, ViewReports) {
> What does this test tries to verify?
This test keeps a cluster around for at least a couple polling intervals, and
since the auto-rebalancer is currently just logging the skew of the cluster,
running the test should print some cluster reports of skew.
I just looked at the output on my terminal manually. Is there a better way to
write the test to check that it appears?
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer.h
File src/kudu/master/autorebalancer.h:
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer.h@74
PS1, Line 74:
> nit: we use spaces, not tabs in Kudu C++ for indentation.
Done
--
To view, visit http://gerrit.cloudera.org:8080/13891
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc829d14dc8636287bf3a4faa4ba76e86ab6885c
Gerrit-Change-Number: 13891
Gerrit-PatchSet: 1
Gerrit-Owner: Hannah Nguyen
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Hannah Nguyen
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 23 Jul 2019 01:16:07 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2780: create thread for auto-rebalancing (1/n)
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/13891 )
Change subject: KUDU-2780: create thread for auto-rebalancing (1/n)
..
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13891/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13891/1//COMMIT_MSG@13
PS1, Line 13: Code that is modified/copied directly from the CLI rebalancing
: tool in kudu/tools is marked.
Did you consider separating the rebalancing piece that doesn't depend on the
ksck structures per se into a library that might be used from both master and
the ksck CLI tool? Another option would be moving the whole rebalancing code
from the tools directory under master directory, and introducing an RPC
end-point in master that the CLI tool would simply call.
I'm not sure it's a good idea to keep duplicate code around.
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/CMakeLists.txt
File src/kudu/master/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/CMakeLists.txt@61
PS1, Line 61: ksck
This introduce a cycling dependency which is a no-no:
12:59:16 CMake Error: The inter-target dependency graph contains the following
strongly connected component (cycle):
12:59:16 "master" of type SHARED_LIBRARY
12:59:16 depends on "ksck" (weak)
12:59:16 "ksck" of type SHARED_LIBRARY
12:59:16 depends on "master" (weak)
BTW, why does master need the ksck library as the link dependecney if the
rebalancer is being run as a simple out-of-the box binary?
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer-test.cc
File src/kudu/master/autorebalancer-test.cc:
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer-test.cc@55
PS1, Line 55: TEST_F(AutoRebalancerTest, NoReports) {
We have many tests which start master and does some work before shutting it
down. Given this scenario doesn't do much aside from that, I think we
automatically get coverage for this simple functionality already, no?
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer-test.cc@69
PS1, Line 69: TEST_F(AutoRebalancerTest, ViewReports) {
What does this test tries to verify?
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer.h
File src/kudu/master/autorebalancer.h:
http://gerrit.cloudera.org:8080/#/c/13891/1/src/kudu/master/autorebalancer.h@74
PS1, Line 74:
nit: we use spaces, not tabs in Kudu C++ for indentation.
--
To view, visit http://gerrit.cloudera.org:8080/13891
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc829d14dc8636287bf3a4faa4ba76e86ab6885c
Gerrit-Change-Number: 13891
Gerrit-PatchSet: 1
Gerrit-Owner: Hannah Nguyen
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 23 Jul 2019 00:56:44 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2797 p2: the master aggregates tablet statistics
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/13426 )
Change subject: KUDU-2797 p2: the master aggregates tablet statistics
..
Patch Set 24:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/13426/24/src/kudu/integration-tests/ts_tablet_manager-itest.cc
File src/kudu/integration-tests/ts_tablet_manager-itest.cc:
http://gerrit.cloudera.org:8080/#/c/13426/24/src/kudu/integration-tests/ts_tablet_manager-itest.cc@589
PS24, Line 589: const auto GetLeaderMaster = [&] (Master** master) {
I wonder if this would be more useful as a lambda that took an std::function as
an argument and:
1. Acquired/verified the ScopedLeaderSharedLock.
2. Ran the std::function
3. Released the ScopedLeaderSharedLock.
http://gerrit.cloudera.org:8080/#/c/13426/24/src/kudu/integration-tests/ts_tablet_manager-itest.cc@631
PS24, Line 631: ASSERT_EQ(kNumMasters, cluster_->num_masters());
: ASSERT_EQ(kNumTservers, cluster_->num_tablet_servers());
Not necessary.
http://gerrit.cloudera.org:8080/#/c/13426/24/src/kudu/integration-tests/ts_tablet_manager-itest.cc@678
PS24, Line 678: ASSERT_OK(con->EmulateElection());
You sure this is safe? The function comment says:
// This is NOT safe to use in a distributed configuration with failure
detection
// enabled, as it could result in a split-brain scenario.
How about using LeaderStepDown instead?
http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.cc@370
PS20, Line 370:
table->RegisterMetrics(catalog_manager_->master_->metric_registry(),
metadata.name());
> I think the table name in the metadata should be a normalized string alread
Hmm, why do we normalize the table name on L354 then?
http://gerrit.cloudera.org:8080/#/c/13426/24/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/13426/24/src/kudu/master/catalog_manager.cc@5515
PS24, Line 5515: std::
Don't need.
http://gerrit.cloudera.org:8080/#/c/13426/24/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:
http://gerrit.cloudera.org:8080/#/c/13426/24/src/kudu/tablet/tablet_replica.cc@824
PS24, Line 824: if (RUNNING == state()) {
To avoid deep nesting, invert this:
if (RUNNING != state()) {
return;
}
...
--
To view, visit http://gerrit.cloudera.org:8080/13426
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I74406ab7cca7c22fda455c328b8ee9989a6b2d99
Gerrit-Change-Number: 13426
Gerrit-PatchSet: 24
Gerrit-Owner: helifu
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: helifu
Gerrit-Comment-Date: Mon, 22 Jul 2019 22:51:46 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/13864 )
Change subject: KUDU-2625: Support per-row error check in prepare stage
..
Patch Set 3:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/client/client-test.cc@2732
PS3, Line 2732: ASSERT_EQ(R"((int32 key=1, int32 int_val=1, string
string_val="One", int32 non_null_with_default=12345))", rows[0]);
: ASSERT_EQ(R"((int32 key=2, int32 int_val=22, string
string_val="Two", int32 non_null_with_default=12345))", rows[1]);
These lines look too long, could you wrap?
Below too.
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/client/client-test.cc@2789
PS3, Line 2789: CHECK(!overflow);
: CHECK_EQ(2, errors.size());
Use ASSERTs here.
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/client/client-test.cc@2798
PS3, Line 2798: ScanTableToStrings(client_table_.get(), );
Need to wrap these calls in NO_FATALS.
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc@449
PS3, Line 449: continue;
Looking over DecodeUpdateOrDelete, it looks like the "nothing to consume, move
on to the next column" case is less common than the "we need to consume the
value in order to properly validate subsequent rows" case. As such, could you
add a comment here explaining why we're avoiding ReadColumn?
Alternatively, you could augment "First process the key columns" to explain the
overall flow and help readers understand why, in the event of a key column
validation failure, we can't just short circuit out.
We also need equivalent breadcrumbs in the comments below, dealing with non-key
columns.
Finally, could you add some multi-row tests to row_operations-test, where a
validation error in the first row doesn't mess up the subsequent rows? Or do we
have such coverage already?
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc@527
PS3, Line 527: ReadColumn(col, scratch);
Wrap in RETURN_NOT_OK? Or should this be wrapped in ignore_result?
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/common/row_operations.cc@616
PS3, Line 616: if (!ops->empty() && all_error) {
: return Status::InvalidArgument("all row operations decoded
error");
: }
Why this short-circuit? Shouldn't we surface a batch of 10/10 decoding errors
using per-row errors?
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/tablet/transactions/write_transaction.cc
File src/kudu/tablet/transactions/write_transaction.cc:
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/tablet/transactions/write_transaction.cc@169
PS3, Line 169: UpdatePerRowErrors();
Is this here just to accommodate the new "if all ops failed to decode, fail the
entire write" codepath?
http://gerrit.cloudera.org:8080/#/c/13864/3/src/kudu/tablet/transactions/write_transaction.cc@220
PS3, Line 220: i++;
Nit: not your fault (since you're just moving code), but this would probably be
clearer if you used a for loop iterating over i and set up a local const RowOp*
in the body of the loop.
--
To view, visit http://gerrit.cloudera.org:8080/13864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0
Gerrit-Change-Number: 13864
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <405403...@qq.com>
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
Gerrit-Comment-Date: Mon, 22 Jul 2019 21:25:38 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2780: create thread for auto-rebalancing (1/n)
Hannah Nguyen has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13891 Change subject: KUDU-2780: create thread for auto-rebalancing (1/n) .. KUDU-2780: create thread for auto-rebalancing (1/n) AutoRebalancerTask is a thread in the master with a configurable polling interval. Every interval, the thread logs the current skew of the cluster if auto-rebalancing is enabled. Code that is modified/copied directly from the CLI rebalancing tool in kudu/tools is marked. The task currently calls the 'ksck' tool to determine skew, but this may change in later patches when rebalancing is implemented and the task retrieves information directly from the master. Change-Id: Ibc829d14dc8636287bf3a4faa4ba76e86ab6885c --- M src/kudu/benchmarks/tpch/tpch1.cc M src/kudu/master/CMakeLists.txt A src/kudu/master/autorebalancer-test.cc A src/kudu/master/autorebalancer.cc A src/kudu/master/autorebalancer.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h 7 files changed, 1,769 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/13891/1 -- To view, visit http://gerrit.cloudera.org:8080/13891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ibc829d14dc8636287bf3a4faa4ba76e86ab6885c Gerrit-Change-Number: 13891 Gerrit-PatchSet: 1 Gerrit-Owner: Hannah Nguyen
[kudu-CR] KUDU-2823 [java client] Support setting dimension for the newly created tablet
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13875 ) Change subject: KUDU-2823 [java client] Support setting dimension for the newly created tablet .. KUDU-2823 [java client] Support setting dimension for the newly created tablet Change-Id: I5fc7797011df578000544147938bd15a6027f4c8 Reviewed-on: http://gerrit.cloudera.org:8080/13875 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo --- M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.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/CreateTableOptions.java M java/kudu-client/src/main/java/org/apache/kudu/client/LocatedTablet.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto 7 files changed, 111 insertions(+), 1 deletion(-) Approvals: Kudu Jenkins: Verified Adar Dembo: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/13875 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I5fc7797011df578000544147938bd15a6027f4c8 Gerrit-Change-Number: 13875 Gerrit-PatchSet: 6 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yao Xu
[kudu-CR] KUDU-2823 [java client] Support setting dimension for the newly created tablet
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13875 ) Change subject: KUDU-2823 [java client] Support setting dimension for the newly created tablet .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13875 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5fc7797011df578000544147938bd15a6027f4c8 Gerrit-Change-Number: 13875 Gerrit-PatchSet: 5 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yao Xu Gerrit-Comment-Date: Mon, 22 Jul 2019 19:10:44 + Gerrit-HasComments: No
[kudu-CR] KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13869 ) Change subject: KUDU-1938 Add CHAR/VARCHAR to C++ client pt 2 .. Patch Set 11: (5 comments) http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/client/predicate-test.cc File src/kudu/client/predicate-test.cc: http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/client/predicate-test.cc@1145 PS11, Line 1145: TestStringPredicates Nit: maybe rename this to something more generic now that it's not just string predicates? Like, these are all predicates whose values are stored in indirect data, right? http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/client/predicate-test.cc@1165 PS11, Line 1165: default: Nit: I would actually reserve the default case for a LOG(FATAL) or some such, since we wouldn't want anyone to extend the list in ::testing::Values() without also adding a new case here. http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/integration-tests/char-itest.cc File src/kudu/integration-tests/char-itest.cc: PS11: This is fine, though extending client-test would have been fine too (fewer LOC I suspect). http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/integration-tests/char-itest.cc@68 PS11, Line 68: // Insert Row May want to explain what's special about the row we're inserting (i.e. what we're trying to test). http://gerrit.cloudera.org:8080/#/c/13869/11/src/kudu/integration-tests/char-itest.cc@85 PS11, Line 85: CHECK_OK ASSERT_OK -- To view, visit http://gerrit.cloudera.org:8080/13869 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifaf609565b0a0a87d6e645cd3ac14c0965af5ba8 Gerrit-Change-Number: 13869 Gerrit-PatchSet: 11 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 22 Jul 2019 19:09:49 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 ) Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1 .. Patch Set 23: (1 comment) http://gerrit.cloudera.org:8080/#/c/13760/23/src/kudu/common/partial_row.h File src/kudu/common/partial_row.h: http://gerrit.cloudera.org:8080/#/c/13760/23/src/kudu/common/partial_row.h@153 PS23, Line 153: /// @note The copying behavior is new for these methods starting Kudu 0.10. : /// Prior to Kudu 0.10, these methods behaved like : /// KuduPartialRow::SetStringNoCopy() and KuduPartialRow::SetBinaryNoCopy() : /// correspondingly. This @note doesn't apply to the new methods you added (below too). Perhaps we should split out Char and Varchar from the setters here into a separate section to avoid the confusion? On a related note, do you think we should add NoCopy variants of the new functions? -- To view, visit http://gerrit.cloudera.org:8080/13760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54 Gerrit-Change-Number: 13760 Gerrit-PatchSet: 23 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 22 Jul 2019 19:09:37 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1938 Add support for CHAR/VARCHAR pt 1
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 ) Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1 .. Patch Set 23: (1 comment) > Patch Set 22: > > Due to the variable length nature of UTF8 and the columnar format I believe > it makes most sense to implement it the same way as Impala did, only wanted > to bring your attention to this discrepancy. This is all super useful design trade off information. Could you try to summarize it into the commit message? http://gerrit.cloudera.org:8080/#/c/13760/23//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13760/23//COMMIT_MSG@23 PS23, Line 23: 65,353 Did you mean 65535 here? -- To view, visit http://gerrit.cloudera.org:8080/13760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54 Gerrit-Change-Number: 13760 Gerrit-PatchSet: 23 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 22 Jul 2019 19:01:55 + Gerrit-HasComments: Yes
[kudu-CR] WIP: [KUDU-442] Add Kudu support for Hive
Grant Henke has abandoned this change. ( http://gerrit.cloudera.org:8080/13483 ) Change subject: WIP: [KUDU-442] Add Kudu support for Hive .. Abandoned This is being contributed to Apache Hive here: https://issues.apache.org/jira/browse/HIVE-12971 -- To view, visit http://gerrit.cloudera.org:8080/13483 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: Ifdcc72c4099dc40a7a2efa59c0c62f63902733d4 Gerrit-Change-Number: 13483 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
honeyhexin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13833 ) Change subject: KUDU-2881 Support create/drop range partition by command line .. Patch Set 7: If the range partition now is unbounded, we can't add range partition for it directly, since it will explict with the existing range partition: UNBOUNDED. However, we can drop the unbounded partition at first and then add for it by the command line. -- To view, visit http://gerrit.cloudera.org:8080/13833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 7 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin Gerrit-Comment-Date: Mon, 22 Jul 2019 07:04:24 + Gerrit-HasComments: No
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#7). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the range partition is unbounded. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 593 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/7 -- To view, visit http://gerrit.cloudera.org:8080/13833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 7 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
