[kudu-CR] KUDU-1861: add range-partitions to loadgen tables
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10633 to look at the new patch set (#8). Change subject: KUDU-1861: add range-partitions to loadgen tables .. KUDU-1861: add range-partitions to loadgen tables This patch adds the ability to generate a range-partitioned table with the loadgen tool. The range partitioning schema is designed such that the non-random write workload will insert sequentially on the primary key, provided the number of threads is equal to the number of tablets. This sequential workload per tablet both reduces the number of compactions and avoids bloom filter lookups. This patch also renames --table_num_buckets to --table_num_hash_partitions, which can be combined with --table_num_range_partitions if desired. I tested this out on a singler-tserver cluster and verified via the metrics logs that the number of bloom lookups for a non-random workload where the number of insert threads and the number of tablets were equal stayed at zero. When the number of threads was not a factor of the number of buckets, the number of bloom lookups was non-zero. See TestNonRandomWorkloadLoadgen in kudu-tool-test.cc for more details. Change-Id: If4f552a4c73dc82f3b7934082769522557ee5013 --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_perf.cc 2 files changed, 244 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/10633/8 -- To view, visit http://gerrit.cloudera.org:8080/10633 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If4f552a4c73dc82f3b7934082769522557ee5013 Gerrit-Change-Number: 10633 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1861: add range-partitions to loadgen tables
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/10633 )
Change subject: KUDU-1861: add range-partitions to loadgen tables
..
Patch Set 7:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1487
PS5, Line 1487:
: // Keep the tab
> Thank you for the explanation. Could you add those details into the commen
Done
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1512
PS5, Line 1512: args = base_args;
: args.emplace_back("--table_num_range_partiti
> I'm not sure I understand. My comment was about reducing
Ah my bad, I misunderstood. Done
http://gerrit.cloudera.org:8080/#/c/10633/6/src/kudu/tools/tool_action_perf.cc
File src/kudu/tools/tool_action_perf.cc:
http://gerrit.cloudera.org:8080/#/c/10633/6/src/kudu/tools/tool_action_perf.cc@197
PS6, Line 197: #include "kudu/util/flag_validators.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/10633/6/src/kudu/tools/tool_action_perf.cc@325
PS6, Line 325:
> nit: why do you need the 'static' specifier for the function within an anon
Done
http://gerrit.cloudera.org:8080/#/c/10633/6/src/kudu/tools/tool_action_perf.cc@325
PS6, Line 325:
> warning: 'ValidatePartitionFlags' is a static definition in anonymous names
Done
--
To view, visit http://gerrit.cloudera.org:8080/10633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If4f552a4c73dc82f3b7934082769522557ee5013
Gerrit-Change-Number: 10633
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sat, 23 Jun 2018 05:29:01 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1861: add range-partitions to loadgen tables
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10633 to look at the new patch set (#7). Change subject: KUDU-1861: add range-partitions to loadgen tables .. KUDU-1861: add range-partitions to loadgen tables This patch adds the ability to generate a range-partitioned table with the loadgen tool. The range partitioning schema is designed such that the non-random write workload will insert sequentially on the primary key, provided the number of threads is equal to the number of tablets. This sequential workload per tablet both reduces the number of compactions and avoids bloom filter lookups. This patch also renames --table_num_buckets to --table_num_hash_partitions, which can be combined with --table_num_range_partitions if desired. I tested this out on a singler-tserver cluster and verified via the metrics logs that the number of bloom lookups for a non-random workload where the number of insert threads and the number of tablets were equal stayed at zero. When the number of threads was not a factor of the number of buckets, the number of bloom lookups was non-zero. See TestNonRandomWorkloadLoadgen in kudu-tool-test.cc for more details. Change-Id: If4f552a4c73dc82f3b7934082769522557ee5013 --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_perf.cc 2 files changed, 242 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/10633/7 -- To view, visit http://gerrit.cloudera.org:8080/10633 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If4f552a4c73dc82f3b7934082769522557ee5013 Gerrit-Change-Number: 10633 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1861: add range-partitions to loadgen tables
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/10633 )
Change subject: KUDU-1861: add range-partitions to loadgen tables
..
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1487
PS5, Line 1487:
: // Keep the tab
> Actually I did mean to use `use_random` to make sure nothing break _even if
Thank you for the explanation. Could you add those details into the comment
itself? I think it would be clearer for the reader than '... nothing funny
happens ...'.
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1512
PS5, Line 1512: args.emplace_back("--table_num_range_partitions=2");
: args.emplace_back("--table_num_hash_partitio
> We're not deleting the tables as we go so we can do this check. That also m
I'm not sure I understand. My comment was about reducing
ASSERT_OK(WaitForNumTabletsOnTS(ts, expected_tablets, kTimeout, ));
ASSERT_EQ(expected_tablets, tablets.size());
to
SSERT_OK(WaitForNumTabletsOnTS(ts, expected_tablets, kTimeout));
The idea is that you don't need the second assert because of the way how
WaitForNumTabletsOnTS() works.
http://gerrit.cloudera.org:8080/#/c/10633/6/src/kudu/tools/tool_action_perf.cc
File src/kudu/tools/tool_action_perf.cc:
http://gerrit.cloudera.org:8080/#/c/10633/6/src/kudu/tools/tool_action_perf.cc@325
PS6, Line 325: static
nit: why do you need the 'static' specifier for the function within an
anonymous namespace?
--
To view, visit http://gerrit.cloudera.org:8080/10633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If4f552a4c73dc82f3b7934082769522557ee5013
Gerrit-Change-Number: 10633
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sat, 23 Jun 2018 04:14:07 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1861: add range-partitions to loadgen tables
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/10633 )
Change subject: KUDU-1861: add range-partitions to loadgen tables
..
Patch Set 6:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/10633/4/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/10633/4/src/kudu/tools/kudu-tool-test.cc@1584
PS4, Line 1584: args.emplace_back("--table_num_hash_partitions=1");
> Done, the new revision passed 1000/1000 times
I only tested in DEBUG mode, and it's surprisingly very flaky in TSAN mode. I
don't think this is quite as important to test, so I'm going to remove this
hashing.
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1473
PS5, Line 1473: {
: ExternalMiniClusterOptions opts;
: opts.num_tablet_servers = 1;
> nit: since std::move() is used for 'opts' which invalidates 'opts' for the
Done
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1476
PS5, Line 1476: NO_FATALS(StartExternalMiniCluster(std::move(opts)));
> nit: const ?
Done
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1479
PS5, Line 1479: "perf", "loadgen",
> Why default 1000 is not good enough here?
Ah, good points, neither the size of the workload nor the flushing behavior in
this case are quite as important as in the other test. I'll switch to use the
defaults.
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1480
PS5, Line 1480: cluster_->master()->bou
> Could you add a comment explaining why manual flushing per row is essential
See other comment
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1481
PS5, Line 1481: // Use a number of threa
> It's 1 by default, so this option can be omitted.
Done
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1482
PS5, Line 1482: // so the insertio
> Why default is not good enough? If it's not indeed, I think it's worth add
Done
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1487
PS5, Line 1487:
: // Keep the tab
> By my experience, funny things happen when using random :) Maybe, you mean
Actually I did mean to use `use_random` to make sure nothing break _even if_
using random inserts. If we didn't use `use_random`, it wouldn't insert across
the entire keyspace, and instead be limited by the number of rows.
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1512
PS5, Line 1512:
> Is it stable enough for TSAN/ASAN builds with 10ms timeout?
Yep, ran with TSAN and it passed 500/500 times, ASAN 300/300
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1512
PS5, Line 1512: args.emplace_back("--table_num_range_partitions=2");
: args.emplace_back("--table_num_hash_partitio
> Here and below: it seems these two lines of code can be reduced to
We're not deleting the tables as we go so we can do this check. That also means
that we have to take into account the previous tables created during this test.
I'll add a comment to that effect.
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1534
PS5, Line 1534: able_num_range_
> nit: 'workloads result' or 'workload results'
Done
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1556
PS5, Line 1556: opts.extra_tserver_flags = {
: "--flush_threshold_mb=1",
: "--flush_threshold_secs=1",
: };
> I think that can be dropped because that are the defaults values for those
Done
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1568
PS5, Line 1568: // each thread will be writing
> I think you could use '--string_len=32768' in addition if you want to gener
Done
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/tool_action_perf.cc
File src/kudu/tools/tool_action_perf.cc:
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/tool_action_perf.cc@303
PS5, Line 303: "the existing table nor its data is never
dropped/deleted.");
: DEFINE_int32(table_num_hash_partitions, 8,
: "The number of hash partitions to create when this
tool creates "
: "a new table. Note: The total number of partitions
must be "
: "greater than 1.");
: DEFINE_int32(table_num_range_partitions, 1,
: "The number of range partitions to create when this
tool creates "
: "a new table. A range partitioning schema will be
determined to "
:
[kudu-CR] KUDU-1861: add range-partitions to loadgen tables
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10633 to look at the new patch set (#6). Change subject: KUDU-1861: add range-partitions to loadgen tables .. KUDU-1861: add range-partitions to loadgen tables This patch adds the ability to generate a range-partitioned table with the loadgen tool. The range partitioning schema is designed such that the non-random write workload will insert sequentially on the primary key, provided the number of threads is equal to the number of tablets. This sequential workload per tablet both reduces the number of compactions and avoids bloom filter lookups. This patch also renames --table_num_buckets to --table_num_hash_partitions, which can be combined with --table_num_range_partitions if desired. I tested this out on a singler-tserver cluster and verified via the metrics logs that the number of bloom lookups for a non-random workload where the number of insert threads and the number of tablets were equal stayed at zero. When the number of threads was not a factor of the number of buckets, the number of bloom lookups was non-zero. See TestNonRandomWorkloadLoadgen in kudu-tool-test.cc for more details. Change-Id: If4f552a4c73dc82f3b7934082769522557ee5013 --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_perf.cc 2 files changed, 247 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/10633/6 -- To view, visit http://gerrit.cloudera.org:8080/10633 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If4f552a4c73dc82f3b7934082769522557ee5013 Gerrit-Change-Number: 10633 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] dmsiterator: remove unused field
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10801 ) Change subject: dmsiterator: remove unused field .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10801 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I877685b59a9353dcc5b9a4b385e5c1c7749154f8 Gerrit-Change-Number: 10801 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Sat, 23 Jun 2018 02:09:34 + Gerrit-HasComments: No
[kudu-CR] [rebalancer] 'auto' mode for RF=1 tablet movements
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/10612 )
Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements
..
Patch Set 8:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc
File src/kudu/tools/tool_action_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@101
PS7, Line 101: 'disabled'. The value of 'auto' means "
: "turn it on/off depending on the replica m
> This can be implied using the right flag tag (perhaps advanced or experimen
Yep, it could be implied using the flag tags, but I'm not sure it's applicable
in case of the flag for the 'kudu' utility. I think we can just drop this
ominous sentence.
http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@173
PS7, Line 173:
client->default_admin_operation_timeout(),
> Nit: add an inline arg to explain what this nullptr means.
Done
http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@180
PS7, Line 180: Version v;
: if (!ParseVersion(version_str, ).ok()) {
> Nit: I think this would be simpler as a single DCHECK on "disabled", on L18
Done
http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@220
PS7, Line 220: summaries }) {
: for (const auto& summary : summaries) {
: if (summary.version) {
: if (!VersionSupportsRF1Movement(*summary.version)) {
> I'll be honest; I don't understand this explanation. Could you clarify furt
Indeed: the comment does not make much sense. Updated.
http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@224
PS7, Line 224: LOG(INFO) << "found Kudu server of version '" <<
*summary.version
> Nit: what's the point of this extra scope?
Done
http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/tools/tool_action_cluster.cc@226
PS7, Line 226:
> Don't we have to test this too? If not, why bother setting it to non-null?
Indeed -- it should return false if it's 3-2-3. It seems like a typo.
http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/util/version_util.h
File src/kudu/util/version_util.h:
http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/util/version_util.h@60
PS7, Line 60:
> I don't think this belongs here. This is util code, so one would expect the
Done
http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/util/version_util.cc
File src/kudu/util/version_util.cc:
http://gerrit.cloudera.org:8080/#/c/10612/7/src/kudu/util/version_util.cc@85
PS7, Line 85:
> Nit: drop
Done
--
To view, visit http://gerrit.cloudera.org:8080/10612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e
Gerrit-Change-Number: 10612
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Sat, 23 Jun 2018 01:58:52 +
Gerrit-HasComments: Yes
[kudu-CR] [rebalancer] 'auto' mode for RF=1 tablet movements
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10612 to look at the new patch set (#8). Change subject: [rebalancer] 'auto' mode for RF=1 tablet movements .. [rebalancer] 'auto' mode for RF=1 tablet movements Added logic to automatically determine whether the tool should move replicas of tablets with RF=1. This is determined by examining the version of Kudu and the replica management schema in the cluster, which indicates whether the fix for KUDU-2443 applies. The fix applies whenever the version is larger than 1.7.0. Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e --- M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_cluster.cc 2 files changed, 172 insertions(+), 15 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/10612/8 -- To view, visit http://gerrit.cloudera.org:8080/10612 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id5a06b137cb34d9351f7e2fb819ed52790e2ee7e Gerrit-Change-Number: 10612 Gerrit-PatchSet: 8 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2191: HMS inconsistent metadata fix tool
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/10303 ) Change subject: KUDU-2191: HMS inconsistent metadata fix tool .. Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/10303/4/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/10303/4/src/kudu/tools/tool_action_hms.cc@502 PS4, Line 502: // Fail the fix process as encountered unfixable errors. > Can you not just alter the HMS entry directly through the HMS catalog? If Done http://gerrit.cloudera.org:8080/#/c/10303/5/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/10303/5/src/kudu/tools/tool_action_hms.cc@73 PS5, Line 73: Hive > s/hive/Hive Done -- To view, visit http://gerrit.cloudera.org:8080/10303 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I63c694b5d9877cfbd218139f2e9ecf05fd69c997 Gerrit-Change-Number: 10303 Gerrit-PatchSet: 6 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Sat, 23 Jun 2018 00:50:50 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191: HMS Metadata Consistency Check Tool
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/10217 )
Change subject: KUDU-2191: HMS Metadata Consistency Check Tool
..
Patch Set 13:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10217/10/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/10217/10/src/kudu/tools/tool_action_hms.cc@394
PS10, Line 394:
> Ah, I missed that. However I think this should also cause the tool to fail
Done
http://gerrit.cloudera.org:8080/#/c/10217/12/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/10217/12/src/kudu/tools/tool_action_hms.cc@372
PS12, Line 372: 0.$1 in
> upgrade
Done
http://gerrit.cloudera.org:8080/#/c/10217/12/src/kudu/tools/tool_action_hms.cc@372
PS12, Line 372: LOG(WARNING) << Substitute("Found legacy table $0.$1 in
the Hive Metastore, "
> add to the end the exact kudu tool invocation command
Done
http://gerrit.cloudera.org:8080/#/c/10217/12/src/kudu/tools/tool_action_hms.cc@404
PS12, Line 404: unsynced_tables_map, cout),
> This failure should cause the tool to return a non-0 exit status. I imagin
Done
--
To view, visit http://gerrit.cloudera.org:8080/10217
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3495e9c755571b85beaecca849f83457b592ee90
Gerrit-Change-Number: 10217
Gerrit-PatchSet: 13
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sat, 23 Jun 2018 00:50:40 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191: HMS Metadata Consistency Check Tool
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10217 to look at the new patch set (#13). Change subject: KUDU-2191: HMS Metadata Consistency Check Tool .. KUDU-2191: HMS Metadata Consistency Check Tool This commit introduces a metadata consistency check CLI tool for Hive Metastore integration. It checks metadata (table ID, table name, Kudu master addresses, and column name and type) between Kudu and HMS, and reports inconsistent metadata to the user if any. It adds test case using external mini cluster to ensure the tool works as expected. Change-Id: I3495e9c755571b85beaecca849f83457b592ee90 --- M src/kudu/client/client.h M src/kudu/hms/hms_catalog.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 4 files changed, 382 insertions(+), 46 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/10217/13 -- To view, visit http://gerrit.cloudera.org:8080/10217 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3495e9c755571b85beaecca849f83457b592ee90 Gerrit-Change-Number: 10217 Gerrit-PatchSet: 13 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2191: HMS inconsistent metadata fix tool
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10303 to look at the new patch set (#6). Change subject: KUDU-2191: HMS inconsistent metadata fix tool .. KUDU-2191: HMS inconsistent metadata fix tool This commit introduces a CLI tool that fixes inconsistent metadata for Hive Metastore integration. It corrects the table name based on the users input. And it fixes the master addresses property, column name and type based on the values in Kudu. Test case is added using external mini cluster to ensure the tool works as expected. Change-Id: I63c694b5d9877cfbd218139f2e9ecf05fd69c997 --- M src/kudu/client/client.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 3 files changed, 246 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/03/10303/6 -- To view, visit http://gerrit.cloudera.org:8080/10303 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I63c694b5d9877cfbd218139f2e9ecf05fd69c997 Gerrit-Change-Number: 10303 Gerrit-PatchSet: 6 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] Synchronizer::WaitFor thread-safety
Dan Burkert has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10783 ) Change subject: Synchronizer::WaitFor thread-safety .. Synchronizer::WaitFor thread-safety WaitFor's implementation previously was not thread safe in the case that the waiter deallocated the Synchronizer after receiving a timeout. Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a Reviewed-on: http://gerrit.cloudera.org:8080/10783 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M src/kudu/util/CMakeLists.txt A src/kudu/util/async_util-test.cc M src/kudu/util/async_util.h 3 files changed, 170 insertions(+), 31 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/10783 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a Gerrit-Change-Number: 10783 Gerrit-PatchSet: 9 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tablet: encapsulate common iterator options
Hello Mike Percy, Grant Henke, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/10802 to review the following change. Change subject: tablet: encapsulate common iterator options .. tablet: encapsulate common iterator options I intend to introduce an additional option or two as part of the "diff scan" API. To that end, this patch knocks the plumbing out of the way by moving all of the common iterator options into a new struct. Unfortunately, I can't use it in Tablet::NewRowIterator because while nearly every iterator expects a pointer to the projection, Tablet::Iterator stores a copy of the projection itself. Change-Id: I7232d163436e6bba75ed66756d2a86c5a959 --- M src/kudu/tablet/compaction.cc M src/kudu/tablet/delta_compaction-test.cc M src/kudu/tablet/delta_iterator_merger.cc M src/kudu/tablet/delta_iterator_merger.h M src/kudu/tablet/delta_store.h M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/deltafile-test.cc M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/deltamemstore-test.cc M src/kudu/tablet/deltamemstore.cc M src/kudu/tablet/deltamemstore.h M src/kudu/tablet/diskrowset-test-base.h M src/kudu/tablet/diskrowset-test.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/mt-rowset_delta_compaction-test.cc M src/kudu/tablet/rowset.cc M src/kudu/tablet/rowset.h M src/kudu/tablet/tablet-test-util.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h 27 files changed, 259 insertions(+), 241 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/10802/1 -- To view, visit http://gerrit.cloudera.org:8080/10802 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I7232d163436e6bba75ed66756d2a86c5a959 Gerrit-Change-Number: 10802 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] dmsiterator: remove unused field
Hello Mike Percy, Grant Henke, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/10801 to review the following change. Change subject: dmsiterator: remove unused field .. dmsiterator: remove unused field Change-Id: I877685b59a9353dcc5b9a4b385e5c1c7749154f8 --- M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltamemstore.h 2 files changed, 2 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/10801/1 -- To view, visit http://gerrit.cloudera.org:8080/10801 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I877685b59a9353dcc5b9a4b385e5c1c7749154f8 Gerrit-Change-Number: 10801 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1861: add range-partitions to loadgen tables
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/10633 )
Change subject: KUDU-1861: add range-partitions to loadgen tables
..
Patch Set 5:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1473
PS5, Line 1473: ExternalMiniClusterOptions opts;
: opts.num_tablet_servers = 1;
: NO_FATALS(StartExternalMiniCluster(std::move(opts)));
nit: since std::move() is used for 'opts' which invalidates 'opts' for the rest
of the scope, consider placing this code into a separate scope.
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1476
PS5, Line 1476: vector base_args = {
nit: const ?
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1479
PS5, Line 1479: "--num_rows_per_thread=2048",
Why default 1000 is not good enough here?
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1480
PS5, Line 1480: "--flush_per_n_rows=1",
Could you add a comment explaining why manual flushing per row is essential in
this case?
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1481
PS5, Line 1481: "--table_num_replicas=1"
It's 1 by default, so this option can be omitted.
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1482
PS5, Line 1482: "--num_threads=3",
Why default is not good enough? If it's not indeed, I think it's worth adding
a comment about that.
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1487
PS5, Line 1487: // Let's ensure nothing funny happens inserting across the
entire keyspace.
: "--use_random",
By my experience, funny things happen when using random :) Maybe, you meant to
not using random here?
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1512
PS5, Line 1512: kTimeout
Is it stable enough for TSAN/ASAN builds with 10ms timeout?
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1512
PS5, Line 1512: ASSERT_OK(WaitForNumTabletsOnTS(ts, expected_tablets,
kTimeout, ));
: ASSERT_EQ(expected_tablets, tablets.size());
Here and below: it seems these two lines of code can be reduced to
ASSERT_OK(WaitForNumTabletsOnTS(ts, expected_tablets, kTimeout));
So, the 'tablets' variable can be dropped.
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1534
PS5, Line 1534: workload result
nit: 'workloads result' or 'workload results'
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1556
PS5, Line 1556: "--table_num_replicas=1",
: "--keep_auto_table=false",
: "--run_scan=false",
: "--use_random=false",
I think that can be dropped because that are the defaults values for those
parameters.
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/kudu-tool-test.cc@1568
PS5, Line 1568: "--num_rows_per_thread=1",
I think you could use '--string_len=32768' in addition if you want to generate
'heavier' rows to reach the flush threshold sooner.
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/tool_action_perf.cc
File src/kudu/tools/tool_action_perf.cc:
http://gerrit.cloudera.org:8080/#/c/10633/5/src/kudu/tools/tool_action_perf.cc@303
PS5, Line 303: DEFINE_int32(table_num_hash_partitions, 8,
: "The number of hash partitions to create when this
tool creates "
: "a new table. Note: The total number of partitions
must be "
: "greater than 1.");
: DEFINE_int32(table_num_range_partitions, 1,
: "The number of range partitions to create when this
tool creates "
: "a new table. A range partitioning schema will be
determined to "
: "evenly split a sequential workload across ranges,
leaving "
: "the outermost ranges unbounded to ensure coverage
of the entire "
: "keyspace. Note: The total number of partitions
must be greater "
: "than 1.");
Maybe, it's worth adding a group validator to verify that the total number of
partitions is greater than 1?
--
To view, visit http://gerrit.cloudera.org:8080/10633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If4f552a4c73dc82f3b7934082769522557ee5013
Gerrit-Change-Number: 10633
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu
[kudu-CR] Synchronizer::WaitFor thread-safety
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10783 ) Change subject: Synchronizer::WaitFor thread-safety .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10783 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a Gerrit-Change-Number: 10783 Gerrit-PatchSet: 8 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 22 Jun 2018 21:31:58 + Gerrit-HasComments: No
[kudu-CR] Synchronizer::WaitFor thread-safety
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10783 to look at the new patch set (#8). Change subject: Synchronizer::WaitFor thread-safety .. Synchronizer::WaitFor thread-safety WaitFor's implementation previously was not thread safe in the case that the waiter deallocated the Synchronizer after receiving a timeout. Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a --- M src/kudu/util/CMakeLists.txt A src/kudu/util/async_util-test.cc M src/kudu/util/async_util.h 3 files changed, 170 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/10783/8 -- To view, visit http://gerrit.cloudera.org:8080/10783 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a Gerrit-Change-Number: 10783 Gerrit-PatchSet: 8 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Synchronizer::WaitFor thread-safety
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10783 ) Change subject: Synchronizer::WaitFor thread-safety .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10783 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a Gerrit-Change-Number: 10783 Gerrit-PatchSet: 7 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 22 Jun 2018 20:55:12 + Gerrit-HasComments: No
[kudu-CR] Synchronizer::WaitFor thread-safety
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10783 to look at the new patch set (#7). Change subject: Synchronizer::WaitFor thread-safety .. Synchronizer::WaitFor thread-safety WaitFor's implementation previously was not thread safe in the case that the waiter deallocated the Synchronizer after receiving a timeout. Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a --- M src/kudu/util/CMakeLists.txt A src/kudu/util/async_util-test.cc M src/kudu/util/async_util.h 3 files changed, 171 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/10783/7 -- To view, visit http://gerrit.cloudera.org:8080/10783 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a Gerrit-Change-Number: 10783 Gerrit-PatchSet: 7 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Synchronizer::WaitFor thread-safety
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10783 ) Change subject: Synchronizer::WaitFor thread-safety .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/10783/6/src/kudu/util/async_util-test.cc File src/kudu/util/async_util-test.cc: http://gerrit.cloudera.org:8080/#/c/10783/6/src/kudu/util/async_util-test.cc@48 PS6, Line 48: alar > alarm Done -- To view, visit http://gerrit.cloudera.org:8080/10783 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a Gerrit-Change-Number: 10783 Gerrit-PatchSet: 7 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 22 Jun 2018 20:50:28 + Gerrit-HasComments: Yes
[kudu-CR] Synchronizer::WaitFor thread-safety
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10783 ) Change subject: Synchronizer::WaitFor thread-safety .. Patch Set 6: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/10783/6/src/kudu/util/async_util-test.cc File src/kudu/util/async_util-test.cc: http://gerrit.cloudera.org:8080/#/c/10783/6/src/kudu/util/async_util-test.cc@48 PS6, Line 48: alar alarm -- To view, visit http://gerrit.cloudera.org:8080/10783 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a Gerrit-Change-Number: 10783 Gerrit-PatchSet: 6 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 22 Jun 2018 20:48:15 + Gerrit-HasComments: Yes
[kudu-CR] Synchronizer::WaitFor thread-safety
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10783 to look at the new patch set (#6). Change subject: Synchronizer::WaitFor thread-safety .. Synchronizer::WaitFor thread-safety WaitFor's implementation previously was not thread safe in the case that the waiter deallocated the Synchronizer after receiving a timeout. Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a --- M src/kudu/util/CMakeLists.txt A src/kudu/util/async_util-test.cc M src/kudu/util/async_util.h 3 files changed, 171 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/10783/6 -- To view, visit http://gerrit.cloudera.org:8080/10783 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a Gerrit-Change-Number: 10783 Gerrit-PatchSet: 6 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Synchronizer::WaitFor thread-safety
Dan Burkert has posted comments on this change. (
http://gerrit.cloudera.org:8080/10783 )
Change subject: Synchronizer::WaitFor thread-safety
..
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h
File src/kudu/util/async_util.h:
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h@44
PS2, Line 44: class Synchronizer {
> I would prefer that, yes. Only one place where CountDown() is used, and if
Done
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 22 Jun 2018 20:33:57 +
Gerrit-HasComments: Yes
[kudu-CR] Synchronizer::WaitFor thread-safety
Dan Burkert has posted comments on this change. (
http://gerrit.cloudera.org:8080/10783 )
Change subject: Synchronizer::WaitFor thread-safety
..
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc
File src/kudu/util/async_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@41
PS2, Line 41: class AsyncUtilTest : public KuduTest {
> Looks good, but why not add it to the test fixture's constructor/destructor
Done
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@46
PS2, Line 46: }
> Oh, I see. I missed that in the first pass. Could you add DISALLOW_COPY_AND
Good idea, done.
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 22 Jun 2018 20:32:41 +
Gerrit-HasComments: Yes
[kudu-CR] Synchronizer::WaitFor thread-safety
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10783 to look at the new patch set (#5). Change subject: Synchronizer::WaitFor thread-safety .. Synchronizer::WaitFor thread-safety WaitFor's implementation previously was not thread safe in the case that the waiter deallocated the Synchronizer after receiving a timeout. Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a --- M src/kudu/util/CMakeLists.txt A src/kudu/util/async_util-test.cc M src/kudu/util/async_util.h 3 files changed, 172 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/10783/5 -- To view, visit http://gerrit.cloudera.org:8080/10783 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a Gerrit-Change-Number: 10783 Gerrit-PatchSet: 5 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add a simple metric for cluster skew
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10787 ) Change subject: Add a simple metric for cluster skew .. Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/10787/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10787/2//COMMIT_MSG@31 PS2, Line 31: theshold threshold http://gerrit.cloudera.org:8080/#/c/10787/2//COMMIT_MSG@31 PS2, Line 31: - nit: add space or replace with a colon? http://gerrit.cloudera.org:8080/#/c/10787/2//COMMIT_MSG@33 PS2, Line 33: external force like unbalanced re-replication or the addition of a tablet nit: it would be nice to have a line with 72 chars or less of length for the commit message. http://gerrit.cloudera.org:8080/#/c/10787/2/src/kudu/master/ts_manager.cc File src/kudu/master/ts_manager.cc: http://gerrit.cloudera.org:8080/#/c/10787/2/src/kudu/master/ts_manager.cc@36 PS2, Line 36: cluster_skew Nit: would it make sense to include 'replica' or 'tablet' in the name of this metric (e.g. 'tablet_replicas_cluster_skew')? Or it already has some implicit namespacing? http://gerrit.cloudera.org:8080/#/c/10787/2/src/kudu/master/ts_manager.cc@145 PS2, Line 145: int& nit: why a reference, not just a copy of an integer value? http://gerrit.cloudera.org:8080/#/c/10787/2/src/kudu/scripts/max_skew_estimate.py File src/kudu/scripts/max_skew_estimate.py: http://gerrit.cloudera.org:8080/#/c/10787/2/src/kudu/scripts/max_skew_estimate.py@20 PS2, Line 20: This The http://gerrit.cloudera.org:8080/#/c/10787/2/src/kudu/scripts/max_skew_estimate.py@20 PS2, Line 20: aximum maximum http://gerrit.cloudera.org:8080/#/c/10787/2/src/kudu/scripts/max_skew_estimate.py@21 PS2, Line 21: nit: extra space http://gerrit.cloudera.org:8080/#/c/10787/2/src/kudu/scripts/max_skew_estimate.py@22 PS2, Line 22: is drop -- To view, visit http://gerrit.cloudera.org:8080/10787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I107256de604998cbf9206a8fccb3a43de86f81a8 Gerrit-Change-Number: 10787 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Fri, 22 Jun 2018 20:19:31 + Gerrit-HasComments: Yes
[kudu-CR] Synchronizer::WaitFor thread-safety
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/10783 )
Change subject: Synchronizer::WaitFor thread-safety
..
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc
File src/kudu/util/async_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@41
PS2, Line 41: class AsyncUtilTest : public KuduTest {
> Done
Looks good, but why not add it to the test fixture's constructor/destructor?
Then you wouldn't have to repeat yourself?
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@46
PS2, Line 46: // Set up an alarm to fail the test in case of deadlock.
> Correct. I removed the DISALLOW_COPY_AND_ASSIGN call on Synchronizer since
Oh, I see. I missed that in the first pass. Could you add
DISALLOW_COPY_AND_ASSIGN to Synchronizer::Data, since that really shouldn't be
copied (and probably can't?).
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h
File src/kudu/util/async_util.h:
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h@44
PS2, Line 44: class Synchronizer {
> It's used in a test, and it's part of the public API and seems reasonably u
I would prefer that, yes. Only one place where CountDown() is used, and if the
actual data ever changed to be more than just the status, only one place to
make that change.
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 22 Jun 2018 20:08:00 +
Gerrit-HasComments: Yes
[kudu-CR] Synchronizer::WaitFor thread-safety
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10783 to look at the new patch set (#4). Change subject: Synchronizer::WaitFor thread-safety .. Synchronizer::WaitFor thread-safety WaitFor's implementation previously was not thread safe in the case that the waiter deallocated the Synchronizer after receiving a timeout. Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a --- M src/kudu/util/CMakeLists.txt A src/kudu/util/async_util-test.cc M src/kudu/util/async_util.h 3 files changed, 180 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/10783/4 -- To view, visit http://gerrit.cloudera.org:8080/10783 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a Gerrit-Change-Number: 10783 Gerrit-PatchSet: 4 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Synchronizer::WaitFor thread-safety
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10783 ) Change subject: Synchronizer::WaitFor thread-safety .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc File src/kudu/util/async_util-test.cc: http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@41 PS2, Line 41: > If the hypothetical failure mode for this test is a deadlock or other test Done -- To view, visit http://gerrit.cloudera.org:8080/10783 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a Gerrit-Change-Number: 10783 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 22 Jun 2018 19:28:25 + Gerrit-HasComments: Yes
[kudu-CR] Synchronizer::WaitFor thread-safety
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10783 to look at the new patch set (#3). Change subject: Synchronizer::WaitFor thread-safety .. Synchronizer::WaitFor thread-safety WaitFor's implementation previously was not thread safe in the case that the waiter deallocated the Synchronizer after receiving a timeout. Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a --- M src/kudu/util/CMakeLists.txt A src/kudu/util/async_util-test.cc M src/kudu/util/async_util.h 3 files changed, 178 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/10783/3 -- To view, visit http://gerrit.cloudera.org:8080/10783 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a Gerrit-Change-Number: 10783 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Synchronizer::WaitFor thread-safety
Dan Burkert has posted comments on this change. (
http://gerrit.cloudera.org:8080/10783 )
Change subject: Synchronizer::WaitFor thread-safety
..
Patch Set 2:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc
File src/kudu/util/async_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@38
PS2, Line 38: public:
> Not needed?
Done
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@46
PS2, Line 46: auto waiter = thread([sync] {
> Aren't these captures by copy? Isn't that disallowed? How does this work?
Correct. I removed the DISALLOW_COPY_AND_ASSIGN call on Synchronizer since
it's no longer necessary for correctness. This test uses the API somewhat
unusually in that it waits on the synchronizer from the background thread and
completes the callback on the original thread, but I don't think that's really
a problem. The more conventional orientation is tested in
TestSynchronizerTimedWait.
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@47
PS2, Line 47: auto s = sync.Wait();
> You're not using 's'; could you just omit the LHS? Or wrap the call in igno
Done
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@96
PS2, Line 96: waiter = thread([cb] {
> Capture by ref here and below?
That would be a use after free in the second case, since the callback would be
cleaned up before the sleep is complete. In the first case I think it's more
typical for the callback to be moved into the background thread.
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@100
PS2, Line 100: CHECK_OK
> ASSERT_OK?
Done
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@111
PS2, Line 111: CHECK
> ASSERT
Done
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h
File src/kudu/util/async_util.h:
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h@33
PS2, Line 33: // Simple class which can be used to make async methods
synchronous.
> Can you update this to describe how the lifecycle of the callbacks is decou
Done
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h@44
PS2, Line 44: void StatusCB(const Status& status) {
> No longer used? If it is used (i.e. for non-callback invocations on the syn
It's used in a test, and it's part of the public API and seems reasonably
useful so I didn't want to remove it. Having them share an implementation
would be pretty awkward:
void StatusCB(const Status& status) {
Data::Callback(std::weak_ptr(data_), status);
}
But if that seems better to you then I can make the change.
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h@71
PS2, Line 71: DCHECK_EQ(data_.use_count(), 1);
> Are you sure that weak refs count towards use_count()? I don't think they d
Yeah you're right. I changed this initially because it smelled like a
Java-style memory leak where a field is failed to be reset to null, but in
practice it won't matter as you point out.
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 22 Jun 2018 19:27:38 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191: HMS inconsistent metadata fix tool
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10303 ) Change subject: KUDU-2191: HMS inconsistent metadata fix tool .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/10303/4/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/10303/4/src/kudu/tools/tool_action_hms.cc@502 PS4, Line 502: > It is to trigger the correction of the master addr, column name, etc in the Can you not just alter the HMS entry directly through the HMS catalog? If not maybe add a doc note because this is a bit confusing. http://gerrit.cloudera.org:8080/#/c/10303/5/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/10303/5/src/kudu/tools/tool_action_hms.cc@73 PS5, Line 73: hive s/hive/Hive -- To view, visit http://gerrit.cloudera.org:8080/10303 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I63c694b5d9877cfbd218139f2e9ecf05fd69c997 Gerrit-Change-Number: 10303 Gerrit-PatchSet: 5 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 22 Jun 2018 19:02:00 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191: HMS Metadata Consistency Check Tool
Dan Burkert has posted comments on this change. (
http://gerrit.cloudera.org:8080/10217 )
Change subject: KUDU-2191: HMS Metadata Consistency Check Tool
..
Patch Set 12:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10217/10/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/10217/10/src/kudu/tools/tool_action_hms.cc@394
PS10, Line 394: if (unsynced_tables_map.empty()) {
> Hmm, right. L392 is logging a warning. You think it is not sufficient?
Ah, I missed that. However I think this should also cause the tool to fail
(return non-0 exist status), but perhaps not immediately if avoidable. Check
out how ksck aggregates error messages.
http://gerrit.cloudera.org:8080/#/c/10217/12/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/10217/12/src/kudu/tools/tool_action_hms.cc@372
PS12, Line 372: "try to use metadata updrade
tool first",
add to the end the exact kudu tool invocation command
http://gerrit.cloudera.org:8080/#/c/10217/12/src/kudu/tools/tool_action_hms.cc@372
PS12, Line 372: updrade
upgrade
http://gerrit.cloudera.org:8080/#/c/10217/12/src/kudu/tools/tool_action_hms.cc@404
PS12, Line 404: return Status::OK();
This failure should cause the tool to return a non-0 exit status. I imagine
that requires returning a non-OK exist status, right?
--
To view, visit http://gerrit.cloudera.org:8080/10217
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3495e9c755571b85beaecca849f83457b592ee90
Gerrit-Change-Number: 10217
Gerrit-PatchSet: 12
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 22 Jun 2018 18:57:21 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2437 Split a tablet into primary key ranges by size
Dan Burkert has posted comments on this change. (
http://gerrit.cloudera.org:8080/10406 )
Change subject: KUDU-2437 Split a tablet into primary key ranges by size
..
Patch Set 14:
(21 comments)
Nice! Really like this optimization.
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/common/key_range.h
File src/kudu/common/key_range.h:
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/common/key_range.h@17
PS14, Line 17: #ifndef KUDU_COMMON_KEY_RANGE_H
Prefer using '#pragma once' for new header files.
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/common/key_range.h@30
PS14, Line 30: const std::string& start_key,
:const std::string& stop_key,
Take the keys by value and move them into the field. This allows the caller to
potentially save a copy if they move the key into the new range.
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/tablet/cfile_set.h
File src/kudu/tablet/cfile_set.h:
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/tablet/cfile_set.h@100
PS14, Line 100: // Excludes the ad hoc index and bloomfiles.
I think this note about the index and bloomfiles being excluded is obvious in
this case, so consider removing it.
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/tablet/rowset.h
File src/kudu/tablet/rowset.h:
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/tablet/rowset.h@140
PS14, Line 140: virtual uint64_t OnDiskBaseDataColumnSize(const ColumnId&
col_id) const = 0;
Given that ColumnId is only 4-bytes and trivially copyable its probably better
for these APIs to take the ColumnId by value (here and elsewhere).
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/tablet/rowset.cc
File src/kudu/tablet/rowset.cc:
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/tablet/rowset.cc@230
PS14, Line 230: // The actual value of this doesn't matter, since it won't be
selected
This comment probably doesn't apply.
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/tablet/rowset_info.h
File src/kudu/tablet/rowset_info.h:
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/tablet/rowset_info.h@53
PS14, Line 53: static void SplitKeyRange(const RowSetTree& tree,
Add doc
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/tablet/rowset_info.cc
File src/kudu/tablet/rowset_info.cc:
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/tablet/rowset_info.cc@151
PS14, Line 151: double WidthByDataSize(const Slice& prev, const Slice& next,
Add a comment, such as:
Computes the the "width" of an interval as above, for the provided columns
in the rowsets.
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/tablet/rowset_info.cc@152
PS14, Line 152:const vector& col_ids,
Consider adding this parameter last, so that the parameter list of the original
method is just a prefix of this ones.
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/tablet/rowset_info.cc@158
PS14, Line 158: const auto& col_id
copy by value here as well
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/tablet/rowset_info.cc@276
PS14, Line 276: &
Keep the & with the type
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/tablet/rowset_info.cc@279
PS14, Line 279: // if start_key greater than stop_key then return without
split
Does this happen in practice? Maybe we should make this an invariant that's
validated at a higher level and put a CHECK here.
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/tablet/rowset_info.cc@283
PS14, Line 283:
It would be great to add a doc here about the algorithm being used, similar to
lines 198 - 212.
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/tablet/rowset_info.cc@311
PS14, Line 311: if (interval_size / 2 > target_chunk_size - chunk_size
&&
I expect that in most cases the target chunk size will be much greater than the
DRS size (e.g. I'd expect hundreds of MiB or more chunk sizes, and DRSs are
capped at 32MiB).
Given that, maybe it makes sense to keep this simple and reset before the cap
is exceeded (with the exception that empty chunks). That way I think you can
make this conditional check just 'last_bound != prev'.
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/tablet/tablet-test.cc
File src/kudu/tablet/tablet-test.cc:
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/tablet/tablet-test.cc@1159
PS14, Line 1159: RowSetVector new_rowset = {
It'd be good to add tests for a few more tablet layout scenarios, namely:
- Tablet with 0 rowsets
- Tablet with 1 rowset
- Tablet with non-overlapping rowsets (with and without gaps between)
- Tablet with rowset whose start is the minimum value
http://gerrit.cloudera.org:8080/#/c/10406/14/src/kudu/tablet/tablet-test.cc@1176
PS14, Line 1176: ASSERT_EQ(result.size(), range.size());
: for (size_t idx = 0; idx < result.size(); ++idx) {
:
[kudu-CR] [consensus] update UnsafeChangeConfig() signature
Alexey Serbin has submitted this change and it was merged. (
http://gerrit.cloudera.org:8080/10141 )
Change subject: [consensus] update UnsafeChangeConfig() signature
..
[consensus] update UnsafeChangeConfig() signature
While testing various scenarios involving kudu CLI's
'remote_replica unsafe_change_config' sub-command, it turned out that
tserver might end up trying to respond with invalid value of
TabletServerErrorPB::Code. In that case, a tserver built in DEBUG
configuration would crash with the following stack trace:
../nptl/sysdeps/unix/sysv/linux/raise.c:64
assertion=0x7fb4eab7c4f8
"::kudu::tserver::TabletServerErrorPB_Code_IsValid(value)",
file=0x7fb4eab7c531 "src/kudu/tserver/tserver.pb.h",
line=, function=) at assert.c:96
assertion=0x7fb4eab7c4f8
"::kudu::tserver::TabletServerErrorPB_Code_IsValid(value)",
file=0x7fb4eab7c531 "src/kudu/tserver/tserver.pb.h", line=2623,
function=0x7fb4eab7c586 "void kudu::tserver::TabletServerErrorPB::set_code(
::kudu::tserver::TabletServerErrorPB_Code)") at assert.c:105
kudu::tserver::TabletServerErrorPB_Code) ()
at src/kudu/tserver/tserver.pb.h:2623
kudu::tserver::TabletServerErrorPB*,
kudu::Status const&, kudu::tserver::TabletServerErrorPB_Code,
kudu::rpc::RpcContext*) ()
at src/kudu/tserver/tablet_service.cc:370
kudu::consensus::UnsafeChangeConfigRequestPB const*,
kudu::consensus::UnsafeChangeConfigResponsePB*,
kudu::rpc::RpcContext*) () at src/kudu/tserver/tablet_service.cc:1060
This changelist fixes the issue, 'unifying' the signatures of the
RaftConsensus::{ChangeConfig,BulkChangeConfig,UpdateChangeConfig}()
methods.
Change-Id: Id266dd72ef5e44e9b4e2ce9df10694bce8cfe2fb
Reviewed-on: http://gerrit.cloudera.org:8080/10141
Tested-by: Alexey Serbin
Reviewed-by: Mike Percy
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/tserver/tablet_service.cc
3 files changed, 17 insertions(+), 17 deletions(-)
Approvals:
Alexey Serbin: Verified
Mike Percy: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/10141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id266dd72ef5e44e9b4e2ce9df10694bce8cfe2fb
Gerrit-Change-Number: 10141
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10777 ) Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash .. KUDU-2260: Log block manager should handle null bytes in metadata on crash On ext4 with data=ordered (the default), it's possible for a write to persist an increase to the filesize without persisting the actual data. In this case, the file will contain null bytes at the end. In the LBM, we considered this case to be corruption if there were enough null bytes (>= 8) for a PB container record length and length checksum. However, it's safe to call this an incomplete record and truncate the file at the end of the last complete record. This patch changes the P container reader code to return Status::Incomplete if it encounters this situation. This will cause the LBM to repair the container metadata appropriately. Two regression tests, at the PB container file and LBM layers, are included. Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b Reviewed-on: http://gerrit.cloudera.org:8080/10777 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo --- M src/kudu/consensus/log_util.cc M src/kudu/fs/log_block_manager-test.cc M src/kudu/util/pb_util-test.cc M src/kudu/util/pb_util.cc M src/kudu/util/slice.cc M src/kudu/util/slice.h 6 files changed, 134 insertions(+), 51 deletions(-) Approvals: Kudu Jenkins: Verified Adar Dembo: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/10777 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b Gerrit-Change-Number: 10777 Gerrit-PatchSet: 6 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10777 ) Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10777 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b Gerrit-Change-Number: 10777 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Fri, 22 Jun 2018 17:17:52 + Gerrit-HasComments: No
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10777 ) Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/10777/4/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/10777/4/src/kudu/fs/log_block_manager-test.cc@625 PS4, Line 625: some extra bytes > some extra null bytes Done -- To view, visit http://gerrit.cloudera.org:8080/10777 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b Gerrit-Change-Number: 10777 Gerrit-PatchSet: 4 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Fri, 22 Jun 2018 15:31:25 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2260: Log block manager should handle null bytes in metadata on crash
Hello Tidy Bot, Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10777 to look at the new patch set (#5). Change subject: KUDU-2260: Log block manager should handle null bytes in metadata on crash .. KUDU-2260: Log block manager should handle null bytes in metadata on crash On ext4 with data=ordered (the default), it's possible for a write to persist an increase to the filesize without persisting the actual data. In this case, the file will contain null bytes at the end. In the LBM, we considered this case to be corruption if there were enough null bytes (>= 8) for a PB container record length and length checksum. However, it's safe to call this an incomplete record and truncate the file at the end of the last complete record. This patch changes the P container reader code to return Status::Incomplete if it encounters this situation. This will cause the LBM to repair the container metadata appropriately. Two regression tests, at the PB container file and LBM layers, are included. Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b --- M src/kudu/consensus/log_util.cc M src/kudu/fs/log_block_manager-test.cc M src/kudu/util/pb_util-test.cc M src/kudu/util/pb_util.cc M src/kudu/util/slice.cc M src/kudu/util/slice.h 6 files changed, 134 insertions(+), 51 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/10777/5 -- To view, visit http://gerrit.cloudera.org:8080/10777 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0af5c9dbbe28afe7a179595ad20392b99cde2a1b Gerrit-Change-Number: 10777 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2382 - [examples] Add Kudu Basic Python Example
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10794 ) Change subject: KUDU-2382 - [examples] Add Kudu Basic Python Example .. Patch Set 1: (12 comments) Is there something used in the python world to specify dependencies and any build process? It'd be ideal if the basic example was self-contained, so if a user took the folder, copied it elsewhere, and started to use it as a basis for a Python application it'd be as easy as possible to get it running. http://gerrit.cloudera.org:8080/#/c/10794/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10794/1//COMMIT_MSG@7 PS1, Line 7: KUDU-2382 - [examples] nit: [examples] KUDU-2382: ... http://gerrit.cloudera.org:8080/#/c/10794/1//COMMIT_MSG@10 PS1, Line 10: nit: is http://gerrit.cloudera.org:8080/#/c/10794/1/examples/python/basic-python-example/README.md File examples/python/basic-python-example/README.md: http://gerrit.cloudera.org:8080/#/c/10794/1/examples/python/basic-python-example/README.md@2 PS1, Line 2: Licensed under the Apache License, Version 2.0 Why does this license header differ from our standard one? Because it's not source code? http://gerrit.cloudera.org:8080/#/c/10794/1/examples/python/basic-python-example/basic_example.py File examples/python/basic-python-example/basic_example.py: http://gerrit.cloudera.org:8080/#/c/10794/1/examples/python/basic-python-example/basic_example.py@23 PS1, Line 23: kudu.master This needs to be configurable so the example is runnable. http://gerrit.cloudera.org:8080/#/c/10794/1/examples/python/basic-python-example/basic_example.py@25 PS1, Line 25: # Define a schema for a new table nit: End comments with a . here and below. http://gerrit.cloudera.org:8080/#/c/10794/1/examples/python/basic-python-example/basic_example.py@31 PS1, Line 31: nit: add "the" or "its" http://gerrit.cloudera.org:8080/#/c/10794/1/examples/python/basic-python-example/basic_example.py@34 PS1, Line 34: nit: a http://gerrit.cloudera.org:8080/#/c/10794/1/examples/python/basic-python-example/basic_example.py@51 PS1, Line 51: Updating nit: Update http://gerrit.cloudera.org:8080/#/c/10794/1/examples/python/basic-python-example/basic_example.py@59 PS1, Line 59: capture nit: extra word http://gerrit.cloudera.org:8080/#/c/10794/1/examples/python/basic-python-example/basic_example.py@62 PS1, Line 62: e Is there anything worth printing in this object? http://gerrit.cloudera.org:8080/#/c/10794/1/examples/python/basic-python-example/basic_example.py@69 PS1, Line 69: Scanner nit: scanner http://gerrit.cloudera.org:8080/#/c/10794/1/examples/python/basic-python-example/basic_example.py@71 PS1, Line 71: result = scanner.open().read_all_tuples() We should print and/or verify the results are as expected. -- To view, visit http://gerrit.cloudera.org:8080/10794 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I12016a7c9b1f4a1e20556297654c7abfa7b673cb Gerrit-Change-Number: 10794 Gerrit-PatchSet: 1 Gerrit-Owner: Jordan Birdsell Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Fri, 22 Jun 2018 15:29:47 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2382 - [examples] Add Kudu Basic Python Example
Jordan Birdsell has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10794 Change subject: KUDU-2382 - [examples] Add Kudu Basic Python Example .. KUDU-2382 - [examples] Add Kudu Basic Python Example This patch adds a basic Kudu Python Client example to the code base. This example also listed on the website. Change-Id: I12016a7c9b1f4a1e20556297654c7abfa7b673cb --- A examples/python/basic-python-example/README.md A examples/python/basic-python-example/basic_example.py 2 files changed, 88 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/10794/1 -- To view, visit http://gerrit.cloudera.org:8080/10794 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I12016a7c9b1f4a1e20556297654c7abfa7b673cb Gerrit-Change-Number: 10794 Gerrit-PatchSet: 1 Gerrit-Owner: Jordan Birdsell
