[kudu-CR] Fix RAT warnings
Hello Grant Henke, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/12533 to review the following change. Change subject: Fix RAT warnings .. Fix RAT warnings Change-Id: Icd9f79f19312982e56ad9e4b5aff9c5fe1f39bec --- M build-support/release/rat_exclude_files.txt 1 file changed, 7 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/12533/1 -- To view, visit http://gerrit.cloudera.org:8080/12533 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Icd9f79f19312982e56ad9e4b5aff9c5fe1f39bec Gerrit-Change-Number: 12533 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Grant Henke
[kudu-CR] [tools] Support starting master and tablet server via the kudu binary
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/12517 )
Change subject: [tools] Support starting master and tablet server via the kudu
binary
..
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12517/4/src/kudu/tools/tool_action_master.cc
File src/kudu/tools/tool_action_master.cc:
http://gerrit.cloudera.org:8080/#/c/12517/4/src/kudu/tools/tool_action_master.cc@218
PS4, Line 218: ActionBuilder("start", &MasterStart)
Should the action be "run" or "exec" to better indicate that this will continue
to run until interrupted?
--
To view, visit http://gerrit.cloudera.org:8080/12517
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3717cbac930b3506a76f7a51388c64afbcbb480e
Gerrit-Change-Number: 12517
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Wed, 20 Feb 2019 05:32:25 +
Gerrit-HasComments: Yes
[kudu-CR](branch-1.9.x) KUDU-2701 Fix compaction loop due to using wrong rowset size
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12532 ) Change subject: KUDU-2701 Fix compaction loop due to using wrong rowset size .. KUDU-2701 Fix compaction loop due to using wrong rowset size Compaction policy originally evaluated the size of a rowset to be the size of the rowset's base data and its REDOs. This size is used to calculate the probability mass of the rowset and the weight of the rowset in the compaction knapsack problem. Mistakenly, it was also used as the size of a rowset for KUDU-1400 small rowset compaction policy. This is wrong- the size of the whole rowset should be used. The reason for this is the following: small rowset compaction compacts rowsets when the number of rowsets is reduced. The number of rowsets produced depends on the total size of the data when written. If a partial size of the rowset is used, this can lead to bad decisions being made about compaction. For example, I discovered a case where a tablet had 8 rowsets of "size" 16MB. In reality, this size was the base data size plus REDOs. Small rowset compaction policy determined that it would be good to compact these 8 rowsets, believing that it would produce 4 rowsets of size 32MB. However, the rowsets were actually 32MB in size, and compacting them produced 8 rowsets of size 32MB identical to the previous 8, and therefore 8 rowsets that appeared to be of size 16MB to compaction policy. Thus these 8 were chosen to be compacted, and so on... This patch changes the small rowset compaction policy code to use the full size of the rowsets. All other uses of size remain the same. It's not totally clear to me why just base data and REDOs are used, but in any case changing it would change CDFs and change knapsack calculations, which could lead to pretty big differences in compaction behavior. Since, outside this bug, compaction is working fine, and since this patch is targeted to unblock 1.9, I opted to make a minimal change to fix the bug and not evaluate any larger chance to compaction policy. I tested this code on the cluster where I originally encountered the compaction loop. The loop did not reoccur and the updated compaction policy successfully compacted small rowsets together for several hours without becoming stuck. It finally converged to a stable end state. I also ran a number of sequential and random insert workloads on this cluster and saw no problems with compactions looping or getting stuck, even under high load or when insertions ended and the tablet was free to catch up on compactions. Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 Reviewed-on: http://gerrit.cloudera.org:8080/12488 Reviewed-by: Andrew Wong Tested-by: Andrew Wong Reviewed-by: Alexey Serbin Reviewed-on: http://gerrit.cloudera.org:8080/12532 Tested-by: Kudu Jenkins --- M src/kudu/tablet/compaction_policy.cc M src/kudu/tablet/rowset_info.cc M src/kudu/tablet/rowset_info.h 3 files changed, 28 insertions(+), 17 deletions(-) Approvals: Alexey Serbin: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/12532 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.9.x Gerrit-MessageType: merged Gerrit-Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 Gerrit-Change-Number: 12532 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [tools] Support starting master and tablet server via the kudu binary
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/12517 )
Change subject: [tools] Support starting master and tablet server via the kudu
binary
..
Patch Set 4:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/12517/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12517/3//COMMIT_MSG@13
PS3, Line 13:
: This means we can ship a single binary in the kudu
: docker image or potentially the kudu-binary jar
: reducing the size by appoximately 66%.
> Does this also inflate the size of the `kudu` binary? If so, by how much?
There is no size or dependency change in the kudu binary because it already
depended on the master and tserver.
http://gerrit.cloudera.org:8080/#/c/12517/3//COMMIT_MSG@22
PS3, Line 22: ng
> by
Done
http://gerrit.cloudera.org:8080/#/c/12517/3//COMMIT_MSG@25
PS3, Line 25: ossible.
> How does this work wrt the log_dir flag? Does it just allow users to specif
This is an existing flag. It defines the name of the file/s created in the log
dir. I will rework this patch to make this no longer a concern. See other
comments.
http://gerrit.cloudera.org:8080/#/c/12517/3//COMMIT_MSG@29
PS3, Line 29: ensure f
> related
Done
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/master/master_main.cc
File src/kudu/master/master_main.cc:
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/master/master_main.cc@38
PS3, Line 38:
: namespace kudu {
> Can be dropped?
Done
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_master.cc
File src/kudu/tools/tool_action_master.cc:
PS3:
> Would strongly prefer not to duplicate master_main.cc here. Could we make M
I will rework this to ensure the code only lives in one place.
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_master.cc@257
PS3, Line 257:"include in output.\nPossible
values: uuid, "
> Should be probably explain here (or in ExtraDescription) that:
Done
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_master.cc@263
PS3, Line 263:
> I agree (the rest of the CLI treats these that way) but if you're going to
Done
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_master.cc@268
PS3, Line 268: .AddAction(std::move(set_flag))
: .AddAction(std::move(start))
> Impala consolidated all their binaries; it's worth checking out how they di
I may be able to do something clever by deferring the call to
kudu::InitGoogleLoggingSafe(prog_name); in tool_main.cc so that I have an
opportunity to set the log_filename before it is set in logging.cc.
I like the idea of not only shipping a single binary, but not having multiple
files just for the sake of argv[0] if I can help it.
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_master.cc@270
PS3, Line 270: .AddAction(std::move(status))
> BTW, why don't we just change the default value to 'kudu-master' for 'kudu
See my comment above. The initialization of logging was happening before I had
a chance to set the correct default. Originally I wasn't sure it was worth the
effort to change, but now I think I will make this patch handle the
log_filename.
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_tserver.cc@251
PS3, Line 251: unique_ptr list_tservers =
> Indentation.
Done
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_tserver.cc@261
PS3, Line 261: .AddOptionalParameter("timeout_ms")
: .Build();
> I'd drop these; keeping them in makes this read more and more like a list o
I decided to put this here because it matches our documentation here:
https://kudu.apache.org/docs/configuration.html#_configuring_tablet_servers
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/util/flags.h
File src/kudu/util/flags.h:
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/util/flags.h@75
PS3, Line 75: GFlagsMap GetFlagsMap();
:
: enum class TriStateFlag {
> Probably easiest to document one of these two variants as "Like the above [
I found I could remove the old version.
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/util/flags.cc@578
PS3, Line 578: flags_by_name.emplace(flag.name, std::move(flag));
> Should be possible to share the implementation with the existing variant.
I found with all the changes I could remove the old version.
--
To view, visit http://gerrit.cloudera.org:8080/12517
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3717cb
[kudu-CR] [tools] Support starting master and tablet server via the kudu binary
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12517 to look at the new patch set (#4). Change subject: [tools] Support starting master and tablet server via the kudu binary .. [tools] Support starting master and tablet server via the kudu binary Adds commands to the kudu tools to run the master and tablet server: - `kudu master start ...` - `kudu tserver start …` This means we can ship a single binary in the kudu docker image or potentially the kudu-binary jar reducing the size by appoximately 66%. Though follow up changes may be needed in the kudu-binary case. The behavior is the same as running the kudu-master and kudu-tserver binaries. The logging initialization was updated to ensure that the logging filenames are the same as when running via the dedicated binaries. A few logging fixes are inclued to ensure this capability is possible. This patch also contains a few related fixes for default flags ensuring that they can be overwritten when neccessary. These changes also ensure flags are not incorrectly reported as non-default. Change-Id: I3717cbac930b3506a76f7a51388c64afbcbb480e --- M src/kudu/master/CMakeLists.txt M src/kudu/master/master_main.cc A src/kudu/master/master_main.h M src/kudu/tools/tool_action.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_master.cc M src/kudu/tools/tool_action_tserver.cc M src/kudu/tools/tool_main.cc M src/kudu/tserver/CMakeLists.txt M src/kudu/tserver/tablet_server.h M src/kudu/tserver/tablet_server_main.cc A src/kudu/tserver/tablet_server_main.h M src/kudu/util/flags-test.cc M src/kudu/util/flags.cc M src/kudu/util/flags.h M src/kudu/util/logging.cc M src/kudu/util/minidump.cc M src/kudu/util/rolling_log.cc M src/kudu/util/rolling_log.h 19 files changed, 276 insertions(+), 73 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/12517/4 -- To view, visit http://gerrit.cloudera.org:8080/12517 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3717cbac930b3506a76f7a51388c64afbcbb480e Gerrit-Change-Number: 12517 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR](branch-1.9.x) KUDU-2701 Fix compaction loop due to using wrong rowset size
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12532 ) Change subject: KUDU-2701 Fix compaction loop due to using wrong rowset size .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12532 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.9.x Gerrit-MessageType: comment Gerrit-Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 Gerrit-Change-Number: 12532 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 20 Feb 2019 04:30:47 + Gerrit-HasComments: No
[kudu-CR](branch-1.9.x) KUDU-2701 Fix compaction loop due to using wrong rowset size
Hello Alexey Serbin, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/12532 to review the following change. Change subject: KUDU-2701 Fix compaction loop due to using wrong rowset size .. KUDU-2701 Fix compaction loop due to using wrong rowset size Compaction policy originally evaluated the size of a rowset to be the size of the rowset's base data and its REDOs. This size is used to calculate the probability mass of the rowset and the weight of the rowset in the compaction knapsack problem. Mistakenly, it was also used as the size of a rowset for KUDU-1400 small rowset compaction policy. This is wrong- the size of the whole rowset should be used. The reason for this is the following: small rowset compaction compacts rowsets when the number of rowsets is reduced. The number of rowsets produced depends on the total size of the data when written. If a partial size of the rowset is used, this can lead to bad decisions being made about compaction. For example, I discovered a case where a tablet had 8 rowsets of "size" 16MB. In reality, this size was the base data size plus REDOs. Small rowset compaction policy determined that it would be good to compact these 8 rowsets, believing that it would produce 4 rowsets of size 32MB. However, the rowsets were actually 32MB in size, and compacting them produced 8 rowsets of size 32MB identical to the previous 8, and therefore 8 rowsets that appeared to be of size 16MB to compaction policy. Thus these 8 were chosen to be compacted, and so on... This patch changes the small rowset compaction policy code to use the full size of the rowsets. All other uses of size remain the same. It's not totally clear to me why just base data and REDOs are used, but in any case changing it would change CDFs and change knapsack calculations, which could lead to pretty big differences in compaction behavior. Since, outside this bug, compaction is working fine, and since this patch is targeted to unblock 1.9, I opted to make a minimal change to fix the bug and not evaluate any larger chance to compaction policy. I tested this code on the cluster where I originally encountered the compaction loop. The loop did not reoccur and the updated compaction policy successfully compacted small rowsets together for several hours without becoming stuck. It finally converged to a stable end state. I also ran a number of sequential and random insert workloads on this cluster and saw no problems with compactions looping or getting stuck, even under high load or when insertions ended and the tablet was free to catch up on compactions. Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 Reviewed-on: http://gerrit.cloudera.org:8080/12488 Reviewed-by: Andrew Wong Tested-by: Andrew Wong Reviewed-by: Alexey Serbin --- M src/kudu/tablet/compaction_policy.cc M src/kudu/tablet/rowset_info.cc M src/kudu/tablet/rowset_info.h 3 files changed, 28 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/12532/1 -- To view, visit http://gerrit.cloudera.org:8080/12532 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.9.x Gerrit-MessageType: newchange Gerrit-Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 Gerrit-Change-Number: 12532 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin
[kudu-CR] Fix some compilation warnings
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12522 to look at the new patch set (#4). Change subject: Fix some compilation warnings .. Fix some compilation warnings Fix compilation warnings on CentOS 7.3 with gcc 4.8.5, and Ubuntu 18.04 with gcc 7.3.0 Change-Id: Ia11e3a3ec1b81332aac0b1c841a78d9fb145c33d --- M src/kudu/common/row_changelist.cc M src/kudu/common/row_changelist.h M src/kudu/consensus/consensus_queue.cc M src/kudu/consensus/log_util.cc M src/kudu/tablet/deltafile.cc M src/kudu/util/env_posix.cc M src/kudu/util/logging.cc 7 files changed, 12 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/12522/4 -- To view, visit http://gerrit.cloudera.org:8080/12522 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia11e3a3ec1b81332aac0b1c841a78d9fb145c33d Gerrit-Change-Number: 12522 Gerrit-PatchSet: 4 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
[kudu-CR] KUDU-2701 Fix compaction loop due to using wrong rowset size
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12488 ) Change subject: KUDU-2701 Fix compaction loop due to using wrong rowset size .. KUDU-2701 Fix compaction loop due to using wrong rowset size Compaction policy originally evaluated the size of a rowset to be the size of the rowset's base data and its REDOs. This size is used to calculate the probability mass of the rowset and the weight of the rowset in the compaction knapsack problem. Mistakenly, it was also used as the size of a rowset for KUDU-1400 small rowset compaction policy. This is wrong- the size of the whole rowset should be used. The reason for this is the following: small rowset compaction compacts rowsets when the number of rowsets is reduced. The number of rowsets produced depends on the total size of the data when written. If a partial size of the rowset is used, this can lead to bad decisions being made about compaction. For example, I discovered a case where a tablet had 8 rowsets of "size" 16MB. In reality, this size was the base data size plus REDOs. Small rowset compaction policy determined that it would be good to compact these 8 rowsets, believing that it would produce 4 rowsets of size 32MB. However, the rowsets were actually 32MB in size, and compacting them produced 8 rowsets of size 32MB identical to the previous 8, and therefore 8 rowsets that appeared to be of size 16MB to compaction policy. Thus these 8 were chosen to be compacted, and so on... This patch changes the small rowset compaction policy code to use the full size of the rowsets. All other uses of size remain the same. It's not totally clear to me why just base data and REDOs are used, but in any case changing it would change CDFs and change knapsack calculations, which could lead to pretty big differences in compaction behavior. Since, outside this bug, compaction is working fine, and since this patch is targeted to unblock 1.9, I opted to make a minimal change to fix the bug and not evaluate any larger chance to compaction policy. I tested this code on the cluster where I originally encountered the compaction loop. The loop did not reoccur and the updated compaction policy successfully compacted small rowsets together for several hours without becoming stuck. It finally converged to a stable end state. I also ran a number of sequential and random insert workloads on this cluster and saw no problems with compactions looping or getting stuck, even under high load or when insertions ended and the tablet was free to catch up on compactions. Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 Reviewed-on: http://gerrit.cloudera.org:8080/12488 Reviewed-by: Andrew Wong Tested-by: Andrew Wong Reviewed-by: Alexey Serbin --- M src/kudu/tablet/compaction_policy.cc M src/kudu/tablet/rowset_info.cc M src/kudu/tablet/rowset_info.h 3 files changed, 28 insertions(+), 17 deletions(-) Approvals: Andrew Wong: Looks good to me, approved; Verified Alexey Serbin: Looks good to me, but someone else must approve -- To view, visit http://gerrit.cloudera.org:8080/12488 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 Gerrit-Change-Number: 12488 Gerrit-PatchSet: 4 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2701 Fix compaction loop due to using wrong rowset size
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12488 ) Change subject: KUDU-2701 Fix compaction loop due to using wrong rowset size .. Patch Set 3: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/12488 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 Gerrit-Change-Number: 12488 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 20 Feb 2019 01:51:18 + Gerrit-HasComments: No
[kudu-CR] KUDU-2038: Support bitmap index
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11722 ) Change subject: KUDU-2038: Support bitmap index .. Patch Set 6: I was recently reading a survey on LSM trees (https://arxiv.org/pdf/1812.07527.pdf), which is relevant to us because every tablet in a Kudu table is an independent LSM tree. Beginning on page 16 there's a discussion on how LSM trees can optimize secondary indexes. The "index maintenance" section (3.7.2) caught my eye; it looks like this patch implements a form of Deferred Lightweight Indexing (DELI), in which secondary indexes are only rewritten during merge operations. The paper as a whole is interesting, but you may find the secondary indexes section to be particularly relevant to your work here. If you have time please take a look; I'd be curious to hear your thoughts. -- To view, visit http://gerrit.cloudera.org:8080/11722 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0edaa0ef1dba2dbce85ebf15f0a731e4939a7860 Gerrit-Change-Number: 11722 Gerrit-PatchSet: 6 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: helifu Gerrit-Comment-Date: Wed, 20 Feb 2019 00:58:27 + Gerrit-HasComments: No
[kudu-CR] KUDU-2690: don't roll log schema on failed alter
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12462 ) Change subject: KUDU-2690: don't roll log schema on failed alter .. Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/12462/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12462/6//COMMIT_MSG@25 PS6, Line 25: use > uses Done http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/tablet/tablet_bootstrap.cc File src/kudu/tablet/tablet_bootstrap.cc: http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/tablet/tablet_bootstrap.cc@1458 PS6, Line 1458: // 3. The commit message contains a 'result', which should only happen if the : //alter resulted in a failure. Exit out without attempting the alter. > AlterSchemas aren't very common, so maybe we should follow the convention o Clarifying the behavior of writes: - if there is an error, record it - if not, record the memstore that the operation affected AFAICT this is done out of necessity; I don't think there is a need to do so for alters. >do we do the same for ChangeConfig? No, the commit message is untouched during replay for ChangeConfig and NoOps, and AFAICT they don't have any results. http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/tablet/tablet_replica-test.cc File src/kudu/tablet/tablet_replica-test.cc: http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/tablet/tablet_replica-test.cc@137 PS6, Line 137: scoped_refptr cmeta_manager( > Could we store this so that your new test needn't recreate it as part of th Done -- To view, visit http://gerrit.cloudera.org:8080/12462 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id761851741297e29a4666bec0c34fc4f7285f715 Gerrit-Change-Number: 12462 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Feb 2019 00:17:50 + Gerrit-HasComments: Yes
[kudu-CR] [tools] Support starting master and tablet server via the kudu binary
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12517 ) Change subject: [tools] Support starting master and tablet server via the kudu binary .. Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/12517/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12517/3//COMMIT_MSG@13 PS3, Line 13: : This means we can ship a single binary in the kudu : docker image or potentially the kudu-binary jar : reducing the size by appoximately 66%. Does this also inflate the size of the `kudu` binary? If so, by how much? http://gerrit.cloudera.org:8080/#/c/12517/3//COMMIT_MSG@22 PS3, Line 22: be by http://gerrit.cloudera.org:8080/#/c/12517/3//COMMIT_MSG@25 PS3, Line 25: log_filename How does this work wrt the log_dir flag? Does it just allow users to specify the prefix? If so, maybe name it log_file_prefix or something? Particularly since the logs will be suffixed by the date and the 'diagnostics' tag. http://gerrit.cloudera.org:8080/#/c/12517/3//COMMIT_MSG@29 PS3, Line 29: releated related http://gerrit.cloudera.org:8080/#/c/12517/2/src/kudu/util/flags.h File src/kudu/util/flags.h: http://gerrit.cloudera.org:8080/#/c/12517/2/src/kudu/util/flags.h@75 PS2, Line 75: // Get all the flags different their defaults. The output is a nicely : // formatted string with --flag=value pairs per line. Redact any flags that : // are tagged as sensitive, if redaction is enabled. nit: it wasn't clear to me how this was different from above; perhaps rephrase like, "Like above, but infers the default values of flags"? -- To view, visit http://gerrit.cloudera.org:8080/12517 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3717cbac930b3506a76f7a51388c64afbcbb480e Gerrit-Change-Number: 12517 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Feb 2019 00:27:26 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2690: don't roll log schema on failed alter
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12462 ) Change subject: KUDU-2690: don't roll log schema on failed alter .. Patch Set 7: Code-Review+1 Looks good to me but would still prefer a review from someone more familiar with the AlterSchema code. -- To view, visit http://gerrit.cloudera.org:8080/12462 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id761851741297e29a4666bec0c34fc4f7285f715 Gerrit-Change-Number: 12462 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Feb 2019 00:26:54 + Gerrit-HasComments: No
[kudu-CR] Fix some compilation warnings
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12522 ) Change subject: Fix some compilation warnings .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12522 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia11e3a3ec1b81332aac0b1c841a78d9fb145c33d Gerrit-Change-Number: 12522 Gerrit-PatchSet: 3 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Wed, 20 Feb 2019 00:27:31 + Gerrit-HasComments: No
[kudu-CR] Fix some compilation warnings
Yingchun Lai has posted comments on this change. (
http://gerrit.cloudera.org:8080/12522 )
Change subject: Fix some compilation warnings
..
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12522/2/src/kudu/util/logging.cc
File src/kudu/util/logging.cc:
http://gerrit.cloudera.org:8080/#/c/12522/2/src/kudu/util/logging.cc@177
PS2, Line 177: if (write(STDERR_FILENO, msg, arraysize(msg)) < 0) {
: // Ignore errors.
: }
> Would ignore_result(write(...)) work? See gutil/basictypes.h.
thanks for your advice
--
To view, visit http://gerrit.cloudera.org:8080/12522
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia11e3a3ec1b81332aac0b1c841a78d9fb145c33d
Gerrit-Change-Number: 12522
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <405403...@qq.com>
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
Gerrit-Comment-Date: Wed, 20 Feb 2019 00:24:26 +
Gerrit-HasComments: Yes
[kudu-CR] Fix some compilation warnings
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12522 to look at the new patch set (#3). Change subject: Fix some compilation warnings .. Fix some compilation warnings Fix compilation warnings on CentOS 7.3 with gcc 4.8.5, and Ubuntu 18.04 with gcc 7.3.0 Change-Id: Ia11e3a3ec1b81332aac0b1c841a78d9fb145c33d --- M src/kudu/common/row_changelist.cc M src/kudu/common/row_changelist.h M src/kudu/consensus/consensus_queue.cc M src/kudu/consensus/log_util.cc M src/kudu/tablet/deltafile.cc M src/kudu/util/env_posix.cc M src/kudu/util/logging.cc 7 files changed, 11 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/12522/3 -- To view, visit http://gerrit.cloudera.org:8080/12522 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia11e3a3ec1b81332aac0b1c841a78d9fb145c33d Gerrit-Change-Number: 12522 Gerrit-PatchSet: 3 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2701 Fix compaction loop due to using wrong rowset size
Andrew Wong has removed a vote on this change. Change subject: KUDU-2701 Fix compaction loop due to using wrong rowset size .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/12488 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 Gerrit-Change-Number: 12488 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2701 Fix compaction loop due to using wrong rowset size
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12488 ) Change subject: KUDU-2701 Fix compaction loop due to using wrong rowset size .. Patch Set 3: Verified+1 Unrelated failure: MultiThreadedTabletTest/1.DeleteAndReinsert -- To view, visit http://gerrit.cloudera.org:8080/12488 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 Gerrit-Change-Number: 12488 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 20 Feb 2019 00:18:45 + Gerrit-HasComments: No
[kudu-CR] KUDU-2690: don't roll log schema on failed alter
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12462 to look at the new patch set (#7). Change subject: KUDU-2690: don't roll log schema on failed alter .. KUDU-2690: don't roll log schema on failed alter It is possible to update the log segment header schema version based on an AlterSchema operation that failed. This is because an alter operation that didn't succeed (e.g. because of a schema version mismatch) is treated as a successful transaction (similar to how we treat a duplicated insert transaction as a successful transaction), and can thus mistakenly lead to updating the log segment schema version, even on failure. This can lead to a mismatch of schemas between the log segment headers and the write ops in those log segments, which can lead to a failure to bootstrap. This patch addresses this by: 1. making the tablet no-op if it sees that an alter didn't go through, 2. storing the error in the commit message, so further bootstraps will skip over the op (this is KUDU-860). Only the former is necessary to prevent the issue, and though the latter uses extra space, it may be helpful for added visibility and debugging. This patch adds a unit test that reproduced the scenario at the TabletReplica level. A more end-to-end test that arrives at the reported state by working around master-level table locking can be found here[1]. [1] https://gist.github.com/andrwng/3a049bb038680cc0254c5ba52b9a7507 Change-Id: Id761851741297e29a4666bec0c34fc4f7285f715 --- M src/kudu/consensus/log.cc M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tablet/tablet_replica-test.cc M src/kudu/tablet/transactions/alter_schema_transaction.cc M src/kudu/tablet/transactions/alter_schema_transaction.h 6 files changed, 241 insertions(+), 82 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/12462/7 -- To view, visit http://gerrit.cloudera.org:8080/12462 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id761851741297e29a4666bec0c34fc4f7285f715 Gerrit-Change-Number: 12462 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2701 Fix compaction loop due to using wrong rowset size
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12488 ) Change subject: KUDU-2701 Fix compaction loop due to using wrong rowset size .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/12488/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12488/1//COMMIT_MSG@14 PS1, Line 14: the size of the whole rowset should be used > Can you be more explicit about which missing pieces are critical? Is it the It's actually the blooms: table id |tablet id | rowset id | block type| size --+--+---+-+ 72e0935bd65a488ca45ab81f99d84dce | 39aba63834b441e0b26d7aa5949f92ae | 2877 | c0 (key)| 8.56M 72e0935bd65a488ca45ab81f99d84dce | 39aba63834b441e0b26d7aa5949f92ae | 2877 | c1 (int_val)| 8.10M 72e0935bd65a488ca45ab81f99d84dce | 39aba63834b441e0b26d7aa5949f92ae | 2877 | c2 (string_val) | 42.6K 72e0935bd65a488ca45ab81f99d84dce | 39aba63834b441e0b26d7aa5949f92ae | 2877 | REDO| 0B 72e0935bd65a488ca45ab81f99d84dce | 39aba63834b441e0b26d7aa5949f92ae | 2877 | UNDO| 0B 72e0935bd65a488ca45ab81f99d84dce | 39aba63834b441e0b26d7aa5949f92ae | 2877 | BLOOM | 15.38M Sorry if that prints like crap. http://gerrit.cloudera.org:8080/#/c/12488/1//COMMIT_MSG@15 PS1, Line 15: values > Nit: this is used as a verb, right? Took a while to parse the sentence; may Done -- To view, visit http://gerrit.cloudera.org:8080/12488 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 Gerrit-Change-Number: 12488 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 19 Feb 2019 23:43:31 + Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 ) Change subject: [sentry] add privilege scope validation to SentryAuthzProvider .. Patch Set 2: (3 comments) Like Andrew and Alexey, I'm also kind of perplexed by this, though it's probably because I don't know enough about Sentry. It's unclear from the commit message why this is a problem that needs to be solved within Kudu and not within Sentry. http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@12 PS2, Line 12: a privilege implies another when, Nit: if all of these conditions must be met, consider rephrasing as: a privilege implies another when: 1. ..., and 2. ..., and 3. ... That makes it more clear that ALL conditions are required rather than ANY condition. http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@24 PS2, Line 24: with Nit: drop 'with' http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@39 PS2, Line 39: privilge privilege -- To view, visit http://gerrit.cloudera.org:8080/12500 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a Gerrit-Change-Number: 12500 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 23:40:03 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2701 Fix compaction loop due to using wrong rowset size
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12488 ) Change subject: KUDU-2701 Fix compaction loop due to using wrong rowset size .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12488 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 Gerrit-Change-Number: 12488 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 23:27:26 + Gerrit-HasComments: No
[kudu-CR] KUDU-2701 Fix compaction loop due to using wrong rowset size
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12488 to look at the new patch set (#3). Change subject: KUDU-2701 Fix compaction loop due to using wrong rowset size .. KUDU-2701 Fix compaction loop due to using wrong rowset size Compaction policy originally evaluated the size of a rowset to be the size of the rowset's base data and its REDOs. This size is used to calculate the probability mass of the rowset and the weight of the rowset in the compaction knapsack problem. Mistakenly, it was also used as the size of a rowset for KUDU-1400 small rowset compaction policy. This is wrong- the size of the whole rowset should be used. The reason for this is the following: small rowset compaction compacts rowsets when the number of rowsets is reduced. The number of rowsets produced depends on the total size of the data when written. If a partial size of the rowset is used, this can lead to bad decisions being made about compaction. For example, I discovered a case where a tablet had 8 rowsets of "size" 16MB. In reality, this size was the base data size plus REDOs. Small rowset compaction policy determined that it would be good to compact these 8 rowsets, believing that it would produce 4 rowsets of size 32MB. However, the rowsets were actually 32MB in size, and compacting them produced 8 rowsets of size 32MB identical to the previous 8, and therefore 8 rowsets that appeared to be of size 16MB to compaction policy. Thus these 8 were chosen to be compacted, and so on... This patch changes the small rowset compaction policy code to use the full size of the rowsets. All other uses of size remain the same. It's not totally clear to me why just base data and REDOs are used, but in any case changing it would change CDFs and change knapsack calculations, which could lead to pretty big differences in compaction behavior. Since, outside this bug, compaction is working fine, and since this patch is targeted to unblock 1.9, I opted to make a minimal change to fix the bug and not evaluate any larger chance to compaction policy. I tested this code on the cluster where I originally encountered the compaction loop. The loop did not reoccur and the updated compaction policy successfully compacted small rowsets together for several hours without becoming stuck. It finally converged to a stable end state. I also ran a number of sequential and random insert workloads on this cluster and saw no problems with compactions looping or getting stuck, even under high load or when insertions ended and the tablet was free to catch up on compactions. Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 --- M src/kudu/tablet/compaction_policy.cc M src/kudu/tablet/rowset_info.cc M src/kudu/tablet/rowset_info.h 3 files changed, 28 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/12488/3 -- To view, visit http://gerrit.cloudera.org:8080/12488 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 Gerrit-Change-Number: 12488 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [tools] Support starting master and tablet server via the kudu binary
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12517 )
Change subject: [tools] Support starting master and tablet server via the kudu
binary
..
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_master.cc
File src/kudu/tools/tool_action_master.cc:
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_master.cc@270
PS3, Line 270: .AddOptionalParameter("log_filename")
BTW, why don't we just change the default value to 'kudu-master' for 'kudu
master start' and 'kudu-tserver' for 'kudu tserver start'? We needn't even
expose it as an optional parameter (because most users won't care to ever
override it); could just manipulate gflags directly.
--
To view, visit http://gerrit.cloudera.org:8080/12517
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3717cbac930b3506a76f7a51388c64afbcbb480e
Gerrit-Change-Number: 12517
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 19 Feb 2019 23:16:33 +
Gerrit-HasComments: Yes
[kudu-CR] [test] Use the kudu binary in the external mini cluster
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12523 )
Change subject: [test] Use the kudu binary in the external mini cluster
..
Patch Set 4:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12523/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12523/4//COMMIT_MSG@7
PS4, Line 7: [test] Use the kudu binary in the external mini cluster
I think you could also change dist_test.py to ship just the 'kudu' binary
rather than 'kudu-tserver' and 'kudu-master'. Could you check?
http://gerrit.cloudera.org:8080/#/c/12523/4//COMMIT_MSG@9
PS4, Line 9: externa
external
http://gerrit.cloudera.org:8080/#/c/12523/4//COMMIT_MSG@10
PS4, Line 10: it’s
its
http://gerrit.cloudera.org:8080/#/c/12523/4/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
PS4:
It's unfortunate that JAR files (i.e. ZIP archives) don't allow symlinks;
otherwise you wouldn't need this patch at all.
http://gerrit.cloudera.org:8080/#/c/12523/4/src/kudu/mini-cluster/external_mini_cluster.cc@1358
PS4, Line 1358: Status ExternalTabletServer::Start() {
Could you set up log_filename correctly here and for ExternalMaster::Start too?
Then some of the other test changes won't be necessary.
http://gerrit.cloudera.org:8080/#/c/12523/4/src/kudu/tserver/tablet_server_main.cc
File src/kudu/tserver/tablet_server_main.cc:
PS4:
If you parameterize and reuse the TabletServerMain function as I suggested in
the first patch, you won't need the fault_before_start change.
--
To view, visit http://gerrit.cloudera.org:8080/12523
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie963f5d50571f3d5ce55e40fcded245bf29b1b55
Gerrit-Change-Number: 12523
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 19 Feb 2019 23:03:43 +
Gerrit-HasComments: Yes
[kudu-CR] [docker] Use only the kudu binary in the kudu image
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12518 ) Change subject: [docker] Use only the kudu binary in the kudu image .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/12518/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12518/2//COMMIT_MSG@13 PS2, Line 13: donwloaded downloaded http://gerrit.cloudera.org:8080/#/c/12518/2/docker/Dockerfile File docker/Dockerfile: http://gerrit.cloudera.org:8080/#/c/12518/2/docker/Dockerfile@233 PS2, Line 233: COPY --from=build /kudu/build/latest/bin/kudu ./ Consider adding symlinks for kudu-master and kudu-tserver for folks used to the old binaries, and to solve the log_filename problem. Actually then you wouldn't need the entrypoint change at all. -- To view, visit http://gerrit.cloudera.org:8080/12518 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I406d18790b17307e9207a5fa67bd733d4689470d Gerrit-Change-Number: 12518 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 19 Feb 2019 22:57:46 + Gerrit-HasComments: Yes
[kudu-CR] [tools] Support starting master and tablet server via the kudu binary
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12517 )
Change subject: [tools] Support starting master and tablet server via the kudu
binary
..
Patch Set 3:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/12517/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12517/3//COMMIT_MSG@29
PS3, Line 29: releated
related
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/master/master_main.cc
File src/kudu/master/master_main.cc:
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/master/master_main.cc@38
PS3, Line 38: DECLARE_int32(webserver_port);
: DECLARE_string(rpc_bind_addresses);
Can be dropped?
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_master.cc
File src/kudu/tools/tool_action_master.cc:
PS3:
Would strongly prefer not to duplicate master_main.cc here. Could we make
MasterMain from master_main.cc non-static and call it from the tool? I see
there are some differences (i.e. whether to parse gflags and initialize
logging) but surely we could parameterize that?
Same for tserver.
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_master.cc@257
PS3, Line 257: .Description("Start a Kudu Master")
Should be probably explain here (or in ExtraDescription) that:
1. The CLI tool will run until interrupted.
2. The Master will run on this machine (i.e. not somewhere remote).
Same for the tserver variant.
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_master.cc@263
PS3, Line 263: // Even though fs_wal_dir is required, we don't want it to
be positional argument.
I agree (the rest of the CLI treats these that way) but if you're going to add
this comment, it'd be good to also explain why.
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_master.cc@268
PS3, Line 268: // In logging.cc the `log_filename` flag is set to the
binary name (`kudu`)
: // by default. It may be common for users to override this.
Impala consolidated all their binaries; it's worth checking out how they did
that. I believe they now use symlinks (i.e. 'impalad' is the real binary and
'catalogd' -> 'impalad'). This affects argv[0] and could be a nice way to
switch log_filename based on the binary being used:
$ cat test.c
#include
int main(int argc, char* argv[]) {
printf("%s\n", argv[0]);
return 0;
}
$ gcc -o test test.c
$ ln -s test foo
$ ./test
./test
$ ./foo
./foo
In the basic CLI case it's not going to help (i.e. when all you have is a
'kudu' binary). But in any packaged scenario where you can maintain symlinks
(i.e. system packages, Docker images, kudu-binary JAR), this could be a simpler
solution rather than forcing users to specify log_filename.
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_tserver.cc@251
PS3, Line 251: .Description("Start a Kudu Tablet Server")
Indentation.
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/tools/tool_action_tserver.cc@261
PS3, Line 261: .AddOptionalParameter("block_cache_capacity_mb")
: .AddOptionalParameter("memory_limit_hard_bytes")
I'd drop these; keeping them in makes this read more and more like a list of
all stable params, and it'll be annoying to remember to update this list when
stabilizing other params elsewhere.
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/util/flags.h
File src/kudu/util/flags.h:
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/util/flags.h@75
PS3, Line 75: // Get all the flags different their defaults. The output is a
nicely
: // formatted string with --flag=value pairs per line. Redact any
flags that
: // are tagged as sensitive, if redaction is enabled.
Probably easiest to document one of these two variants as "Like the above [or
like the below or whatever], but...".
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:
http://gerrit.cloudera.org:8080/#/c/12517/3/src/kudu/util/flags.cc@578
PS3, Line 578: string GetNonDefaultFlags() {
Should be possible to share the implementation with the existing variant.
--
To view, visit http://gerrit.cloudera.org:8080/12517
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3717cbac930b3506a76f7a51388c64afbcbb480e
Gerrit-Change-Number: 12517
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 19 Feb 2019 22:57:50 +
Gerrit-HasComments: Yes
[kudu-CR](branch-1.9.x) [minicluster] Fix building with python 2.6
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12527 ) Change subject: [minicluster] Fix building with python 2.6 .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12527 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.9.x Gerrit-MessageType: comment Gerrit-Change-Id: Id9e7ce9a1fd7ac813866678d6ec804fdf91ea729 Gerrit-Change-Number: 12527 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 22:49:39 + Gerrit-HasComments: No
[kudu-CR] [minicluster] Fix building with python 2.6
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12526 ) Change subject: [minicluster] Fix building with python 2.6 .. [minicluster] Fix building with python 2.6 This patch fixes building the minicluster binary jar with python 2.6. This is imporant because python 2.6 is the default version on centos 6 which is the primary linux OS that the kudu-binary jar is built on. Because I used Docker to test this, the related Docker build fixes are included too. Change-Id: Id9e7ce9a1fd7ac813866678d6ec804fdf91ea729 Reviewed-on: http://gerrit.cloudera.org:8080/12526 Reviewed-by: Adar Dembo Reviewed-by: Andrew Wong Tested-by: Kudu Jenkins --- M .dockerignore M build-support/mini-cluster/relocate_binaries_for_mini_cluster.py M docker/bootstrap-dev-env.sh 3 files changed, 8 insertions(+), 1 deletion(-) Approvals: Adar Dembo: Looks good to me, approved Andrew Wong: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/12526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Id9e7ce9a1fd7ac813866678d6ec804fdf91ea729 Gerrit-Change-Number: 12526 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR](branch-1.9.x) [minicluster] Fix building with python 2.6
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12527 ) Change subject: [minicluster] Fix building with python 2.6 .. [minicluster] Fix building with python 2.6 This patch fixes building the minicluster binary jar with python 2.6. This is imporant because python 2.6 is the default version on centos 6 which is the primary linux OS that the kudu-binary jar is built on. Because I used Docker to test this, the related Docker build fixes are included too. Change-Id: Id9e7ce9a1fd7ac813866678d6ec804fdf91ea729 Reviewed-on: http://gerrit.cloudera.org:8080/12527 Reviewed-by: Adar Dembo Reviewed-by: Andrew Wong Tested-by: Grant Henke --- M .dockerignore M build-support/mini-cluster/relocate_binaries_for_mini_cluster.py M docker/bootstrap-dev-env.sh 3 files changed, 8 insertions(+), 1 deletion(-) Approvals: Adar Dembo: Looks good to me, approved Andrew Wong: Looks good to me, approved Grant Henke: Verified -- To view, visit http://gerrit.cloudera.org:8080/12527 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.9.x Gerrit-MessageType: merged Gerrit-Change-Id: Id9e7ce9a1fd7ac813866678d6ec804fdf91ea729 Gerrit-Change-Number: 12527 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2701 Fix compaction loop due to using wrong rowset size
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12488 ) Change subject: KUDU-2701 Fix compaction loop due to using wrong rowset size .. Patch Set 2: Verified+1 Code-Review+1 The failure was of: rebalancer_tool-test.7 Seems unrelated. -- To view, visit http://gerrit.cloudera.org:8080/12488 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 Gerrit-Change-Number: 12488 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 22:40:30 + Gerrit-HasComments: No
[kudu-CR] KUDU-2701 Fix compaction loop due to using wrong rowset size
Andrew Wong has removed a vote on this change. Change subject: KUDU-2701 Fix compaction loop due to using wrong rowset size .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/12488 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 Gerrit-Change-Number: 12488 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR](branch-1.9.x) [minicluster] Fix building with python 2.6
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12527 ) Change subject: [minicluster] Fix building with python 2.6 .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12527 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.9.x Gerrit-MessageType: comment Gerrit-Change-Id: Id9e7ce9a1fd7ac813866678d6ec804fdf91ea729 Gerrit-Change-Number: 12527 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 22:36:18 + Gerrit-HasComments: No
[kudu-CR] [minicluster] Fix building with python 2.6
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12526 ) Change subject: [minicluster] Fix building with python 2.6 .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9e7ce9a1fd7ac813866678d6ec804fdf91ea729 Gerrit-Change-Number: 12526 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 22:36:12 + Gerrit-HasComments: No
[kudu-CR] [minicluster] Fix building with python 2.6
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12526 ) Change subject: [minicluster] Fix building with python 2.6 .. Patch Set 1: (1 comment) > Patch Set 1: Code-Review+2 > > (1 comment) http://gerrit.cloudera.org:8080/#/c/12526/1/build-support/mini-cluster/relocate_binaries_for_mini_cluster.py File build-support/mini-cluster/relocate_binaries_for_mini_cluster.py: http://gerrit.cloudera.org:8080/#/c/12526/1/build-support/mini-cluster/relocate_binaries_for_mini_cluster.py@119 PS1, Line 119: check_output > To add more color: Gotcha, thanks for the explanations. -- To view, visit http://gerrit.cloudera.org:8080/12526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9e7ce9a1fd7ac813866678d6ec804fdf91ea729 Gerrit-Change-Number: 12526 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 22:36:40 + Gerrit-HasComments: Yes
[kudu-CR] [minicluster] Fix building with python 2.6
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12526 ) Change subject: [minicluster] Fix building with python 2.6 .. Patch Set 1: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/12526/1/build-support/mini-cluster/relocate_binaries_for_mini_cluster.py File build-support/mini-cluster/relocate_binaries_for_mini_cluster.py: http://gerrit.cloudera.org:8080/#/c/12526/1/build-support/mini-cluster/relocate_binaries_for_mini_cluster.py@119 PS1, Line 119: check_output > We import check_output from kudu_util above which is python 2.6 compatible. To add more color: 1. subprocess.check_output is new in Python 2.7. 2. kudu_util.check_output is a carbon copy of Python 2.7's subprocess.check_output. 3. This script already imported kudu_util.check_output to use in one place, but didn't use it here, which is why the script stopped working on Python 2.6. -- To view, visit http://gerrit.cloudera.org:8080/12526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9e7ce9a1fd7ac813866678d6ec804fdf91ea729 Gerrit-Change-Number: 12526 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 22:18:27 + Gerrit-HasComments: Yes
[kudu-CR](branch-1.9.x) [minicluster] Fix building with python 2.6
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12527 Change subject: [minicluster] Fix building with python 2.6 .. [minicluster] Fix building with python 2.6 This patch fixes building the minicluster binary jar with python 2.6. This is imporant because python 2.6 is the default version on centos 6 which is the primary linux OS that the kudu-binary jar is built on. Because I used Docker to test this, the related Docker build fixes are included too. Change-Id: Id9e7ce9a1fd7ac813866678d6ec804fdf91ea729 --- M .dockerignore M build-support/mini-cluster/relocate_binaries_for_mini_cluster.py M docker/bootstrap-dev-env.sh 3 files changed, 8 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/12527/1 -- To view, visit http://gerrit.cloudera.org:8080/12527 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.9.x Gerrit-MessageType: newchange Gerrit-Change-Id: Id9e7ce9a1fd7ac813866678d6ec804fdf91ea729 Gerrit-Change-Number: 12527 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR](branch-1.9.x) [minicluster] Fix building with python 2.6
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12527 ) Change subject: [minicluster] Fix building with python 2.6 .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12527 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.9.x Gerrit-MessageType: comment Gerrit-Change-Id: Id9e7ce9a1fd7ac813866678d6ec804fdf91ea729 Gerrit-Change-Number: 12527 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 22:19:42 + Gerrit-HasComments: No
[kudu-CR] [minicluster] Fix building with python 2.6
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12526 ) Change subject: [minicluster] Fix building with python 2.6 .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/12526/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12526/1//COMMIT_MSG@14 PS1, Line 14: Because I used Docker to test this, the related Docker > Is the chrpath change related to the python change? Or is it just a change chrpath is used in build-support/mini-cluster/relocate_binaries_for_mini_cluster.py and not installed in the docker images by default. Without it I couldn't build the jar in the docker images. http://gerrit.cloudera.org:8080/#/c/12526/1/build-support/mini-cluster/relocate_binaries_for_mini_cluster.py File build-support/mini-cluster/relocate_binaries_for_mini_cluster.py: http://gerrit.cloudera.org:8080/#/c/12526/1/build-support/mini-cluster/relocate_binaries_for_mini_cluster.py@119 PS1, Line 119: check_output > I'm confused by this change; I thought you could only use this syntax if yo We import check_output from kudu_util above which is python 2.6 compatible. It's used elsewhere in this script already but was missed here. -- To view, visit http://gerrit.cloudera.org:8080/12526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9e7ce9a1fd7ac813866678d6ec804fdf91ea729 Gerrit-Change-Number: 12526 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 22:04:00 + Gerrit-HasComments: Yes
[kudu-CR] [minicluster] Fix building with python 2.6
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12526 ) Change subject: [minicluster] Fix building with python 2.6 .. Patch Set 1: Code-Review+1 (2 comments) LGTM, just want to make sure I understand the change http://gerrit.cloudera.org:8080/#/c/12526/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12526/1//COMMIT_MSG@14 PS1, Line 14: Because I used Docker to test this, the related Docker Is the chrpath change related to the python change? Or is it just a change that happened to be necessary for centos6? (and ubuntu, it seems?) http://gerrit.cloudera.org:8080/#/c/12526/1/build-support/mini-cluster/relocate_binaries_for_mini_cluster.py File build-support/mini-cluster/relocate_binaries_for_mini_cluster.py: http://gerrit.cloudera.org:8080/#/c/12526/1/build-support/mini-cluster/relocate_binaries_for_mini_cluster.py@119 PS1, Line 119: check_output I'm confused by this change; I thought you could only use this syntax if you did: from subprocess import check_output or somesuch -- To view, visit http://gerrit.cloudera.org:8080/12526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9e7ce9a1fd7ac813866678d6ec804fdf91ea729 Gerrit-Change-Number: 12526 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 21:58:52 + Gerrit-HasComments: Yes
[kudu-CR] [minicluster] Fix building with python 2.6
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12526 Change subject: [minicluster] Fix building with python 2.6 .. [minicluster] Fix building with python 2.6 This patch fixes building the minicluster binary jar with python 2.6. This is imporant because python 2.6 is the default version on centos 6 which is the primary linux OS that the kudu-binary jar is built on. Because I used Docker to test this, the related Docker build fixes are included too. Change-Id: Id9e7ce9a1fd7ac813866678d6ec804fdf91ea729 --- M .dockerignore M build-support/mini-cluster/relocate_binaries_for_mini_cluster.py M docker/bootstrap-dev-env.sh 3 files changed, 8 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/12526/1 -- To view, visit http://gerrit.cloudera.org:8080/12526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Id9e7ce9a1fd7ac813866678d6ec804fdf91ea729 Gerrit-Change-Number: 12526 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] Rename DeadlineTracker to TimeoutTracker
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12373 ) Change subject: Rename DeadlineTracker to TimeoutTracker .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3f465c925856390ecf4747e84bdd5a67c51c69eb Gerrit-Change-Number: 12373 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 19 Feb 2019 21:38:40 + Gerrit-HasComments: No
[kudu-CR] KUDU-1900: add loopback check and test
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 15: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 15 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 21:36:33 + Gerrit-HasComments: No
[kudu-CR] [client] extra validation on rows upon Session::Apply()
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12512 ) Change subject: [client] extra validation on rows upon Session::Apply() .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/12512/1/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/12512/1/src/kudu/client/client-test.cc@5887 PS1, Line 5887: ASSERT_OK(session->SetFlushMode(KuduSession::MANUAL_FLUSH)); > Would AUTO_FLUSH_SYNC help shorten these tests by obviating the need for an I wanted to see that the error comes from Apply(), and not from the underlying Flush() if it were AUTO_FLUSH_SYNC. Yep, I think using lambdas would help here. I'll update this correspondingly. http://gerrit.cloudera.org:8080/#/c/12512/1/src/kudu/client/write_op.h File src/kudu/client/write_op.h: http://gerrit.cloudera.org:8080/#/c/12512/1/src/kudu/client/write_op.h@111 PS1, Line 111: virtual Status Verify() const; > The way of calling type() was the first route I tried to use, but it didn't Probably, using calling KuduWriteOperation::type() in KuduSession::Apply() should work. -- To view, visit http://gerrit.cloudera.org:8080/12512 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I822f20f3242df7983c63ac146f298acfcfbafc68 Gerrit-Change-Number: 12512 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 21:33:12 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1868: Part 1: Add timer-based RPC timeouts
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12338 )
Change subject: KUDU-1868: Part 1: Add timer-based RPC timeouts
..
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12338/9/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/12338/9/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1378
PS9, Line 1378: long timeoutMs) {
If there's a parent RPC, shouldn't we reuse its deadline?
--
To view, visit http://gerrit.cloudera.org:8080/12338
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8d823b63ac0a41cc5e42b63a7c19e0ef777e1dea
Gerrit-Change-Number: 12338
Gerrit-PatchSet: 9
Gerrit-Owner: Will Berkeley
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Tue, 19 Feb 2019 21:24:29 +
Gerrit-HasComments: Yes
[kudu-CR] Rename DeadlineTracker to TimeoutTracker
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12373 ) Change subject: Rename DeadlineTracker to TimeoutTracker .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3f465c925856390ecf4747e84bdd5a67c51c69eb Gerrit-Change-Number: 12373 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 19 Feb 2019 21:04:07 + Gerrit-HasComments: No
[kudu-CR] [client] extra validation on rows upon Session::Apply()
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12512 ) Change subject: [client] extra validation on rows upon Session::Apply() .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12512/1/src/kudu/client/write_op.h File src/kudu/client/write_op.h: http://gerrit.cloudera.org:8080/#/c/12512/1/src/kudu/client/write_op.h@111 PS1, Line 111: virtual Status Verify() const; > Is this safe from an ABI compatibility perspective? The KDE wiki suggests t The way of calling type() was the first route I tried to use, but it didn't work. The issue was due to the problem declaring a private sub-class to be a friend of another class. I.e., I could not figure out how to declare KuduSession::Data to be a friend to KuduWriteOperation. I'm not sure that's possible in C++. However, there might be other way around. I liked the solution with the Verify() method since it looked better from the conceptual point. -- To view, visit http://gerrit.cloudera.org:8080/12512 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I822f20f3242df7983c63ac146f298acfcfbafc68 Gerrit-Change-Number: 12512 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 21:02:59 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12474 to look at the new patch set (#15). Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. For convenience, this change adds two static utility methods (HostPort::IsLoopback and HostPort::AddrToString), and refactors Sockaddr::host and Sockaddr::IsAnyLocalAddress to use these static methods. Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/socket.cc 5 files changed, 163 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12474/15 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 15 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2690: don't roll log schema on failed alter
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12462 ) Change subject: KUDU-2690: don't roll log schema on failed alter .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/12462/2/src/kudu/tablet/transactions/alter_schema_transaction.cc File src/kudu/tablet/transactions/alter_schema_transaction.cc: http://gerrit.cloudera.org:8080/#/c/12462/2/src/kudu/tablet/transactions/alter_schema_transaction.cc@153 PS2, Line 153: } : : // The schema lock was acquired by Tablet::CreatePreparedAlterSchem > I'm wondering, if this is skipped, whether a client waiting on an ongoing A In this case, AlterSchema the function succeeded, but if the logical operation didn't succeed, there would be no change to metadata's schema_version_ (see tablet/tablet.cc::AlterSchema()). I can add a comment to that effect. Given that's the case, do you still think it's worth marking dirty? I'm hesitant because KUDU-2681 means that over-reporting can be costly and make things painful to debug; and in the case that that report has no real changes, I'd rather not report anything. -- To view, visit http://gerrit.cloudera.org:8080/12462 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id761851741297e29a4666bec0c34fc4f7285f715 Gerrit-Change-Number: 12462 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 19 Feb 2019 20:55:26 + Gerrit-HasComments: Yes
[kudu-CR] Rename DeadlineTracker to TimeoutTracker
Hello Mike Percy, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12373 to look at the new patch set (#5). Change subject: Rename DeadlineTracker to TimeoutTracker .. Rename DeadlineTracker to TimeoutTracker A deadline is the latest time by which something should be completed. It's an instant, like "next Tuesday at noon". A timeout is an interval of time allowed for some event to occur or be completed. It's a delta, like "15 minutes". The DeadlineTracker tracked a "relative deadline", relative to when the instance's "deadline" was set. That's actually a timeout. This patch harmonizes the names to the concepts. Change-Id: I3f465c925856390ecf4747e84bdd5a67c51c69eb --- M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java M java/kudu-client/src/main/java/org/apache/kudu/client/BatchResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableRequest.java D java/kudu-client/src/main/java/org/apache/kudu/client/DeadlineTracker.java M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/IsAlterTableDoneRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPartitioner.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java M java/kudu-client/src/main/java/org/apache/kudu/client/ListTablesRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/ListTabletServersRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/ListTabletsRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java M java/kudu-client/src/main/java/org/apache/kudu/client/PingRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java A java/kudu-client/src/main/java/org/apache/kudu/client/TimeoutTracker.java M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java R java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeoutTracker.java M java/kudu-test-utils/src/main/java/org/apache/kudu/test/KuduTestHarness.java M java/kudu-test-utils/src/test/java/org/apache/kudu/test/TestMiniKuduCluster.java 26 files changed, 232 insertions(+), 233 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/12373/5 -- To view, visit http://gerrit.cloudera.org:8080/12373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3f465c925856390ecf4747e84bdd5a67c51c69eb Gerrit-Change-Number: 12373 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2701 Fix compaction loop due to using wrong rowset size
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12488 to look at the new patch set (#2). Change subject: KUDU-2701 Fix compaction loop due to using wrong rowset size .. KUDU-2701 Fix compaction loop due to using wrong rowset size Compaction policy originally evaluated the size of a rowset to be the size of the rowset's base data and its REDOs. This size is used to calculate the probability mass of the rowset and the weight of the rowset in the compaction knapsack problem. Mistakenly, it was also used as the size of a rowset for KUDU-1400 small rowset compaction policy. This is wrong- the size of the whole rowset should be used. The reason for this is the following: small rowset compaction compacts rowsets when the number of rowsets is reduced. The number of rowsets produced depends on the total size of the data when written. If a partial size of the rowset is used, this can lead to bad decisions being made about compaction. For example, I discovered a case where a tablet had 8 rowsets of "size" 16MB. In reality, this size was the base data size plus REDOs. Small rowset compaction policy determined that it would be good to compact these 8 rowsets, believing that it would produce 4 rowsets of size 32MB. However, the rowsets were actually 32MB in size, and compacting them produced 8 rowsets of size 32MB identical to the previous 8, and therefore 8 rowsets that appeared to be of size 16MB to compaction policy. Thus these 8 were chosen to be compacted, and so on... This patch changes the small rowset compaction policy code to use the full size of the rowsets. All other uses of size remain the same. It's not totally clear to me why just base data and REDOs are used, but in any case changing it would change CDFs and change knapsack calculations, which could lead to pretty big differences in compaction behavior. Since, outside this bug, compaction is working fine, and since this patch is targeted to unblock 1.9, I opted to make a minimal change to fix the bug and not evaluate any larger chance to compaction policy. Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 --- M src/kudu/tablet/compaction_policy.cc M src/kudu/tablet/rowset_info.cc M src/kudu/tablet/rowset_info.h 3 files changed, 28 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/12488/2 -- To view, visit http://gerrit.cloudera.org:8080/12488 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I21b7ff6333137aaf1e98ef4849691dd08e24e007 Gerrit-Change-Number: 12488 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1868: Part 1: Add timer-based RPC timeouts
Hello Mike Percy, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12338 to look at the new patch set (#9). Change subject: KUDU-1868: Part 1: Add timer-based RPC timeouts .. KUDU-1868: Part 1: Add timer-based RPC timeouts Currently, the Java client requires some kind of event to detect the timeout of an RPC: either a response from the server in the chain of sub-RPCs or a socket read timeout on the connection. This patch adds a timer task to actively time out an RPC once it passes its deadline. Part 2 will eliminate socket read timeouts from the Java client, except possibly in the case of negotiation, which will fully resolve KUDU-1868. There is one test included, which checks that timeouts occur without an "outside stimulus" like a response from the server. This patch should not degrade the performance of the client. Even though every timer task holds a reference to its RPC, when the RPC completes it cancels the timer task, which will make the timer release it at the next tick. This means the RPC and its task should be available to be GC'd after the next tick of the timer. Change-Id: I8d823b63ac0a41cc5e42b63a7c19e0ef777e1dea --- M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java M java/kudu-client/src/main/java/org/apache/kudu/client/BatchResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToMasterRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/IsAlterTableDoneRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java M java/kudu-client/src/main/java/org/apache/kudu/client/ListTablesRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/ListTablesResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/ListTabletServersRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/ListTabletServersResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/ListTabletsRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/ListTabletsResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java M java/kudu-client/src/main/java/org/apache/kudu/client/OperationResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/PingRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java M java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java 33 files changed, 422 insertions(+), 184 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/38/12338/9 -- To view, visit http://gerrit.cloudera.org:8080/12338 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8d823b63ac0a41cc5e42b63a7c19e0ef777e1dea Gerrit-Change-Number: 12338 Gerrit-PatchSet: 9 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2690: don't roll log schema on failed alter
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12462 ) Change subject: KUDU-2690: don't roll log schema on failed alter .. Patch Set 6: (4 comments) http://gerrit.cloudera.org:8080/#/c/12462/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12462/6//COMMIT_MSG@25 PS6, Line 25: use uses http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/tablet/tablet_bootstrap.cc File src/kudu/tablet/tablet_bootstrap.cc: http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/tablet/tablet_bootstrap.cc@1458 PS6, Line 1458: // 3. The commit message contains a 'result', which should only happen if the : //alter resulted in a failure. Exit out without attempting the alter. AlterSchemas aren't very common, so maybe we should follow the convention of other REPLICATE messages and put a result in even on success? AFAICT this is what we do for Write; do we do the same for ChangeConfig? http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/tablet/tablet_replica-test.cc File src/kudu/tablet/tablet_replica-test.cc: http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/tablet/tablet_replica-test.cc@137 PS6, Line 137: scoped_refptr cmeta_manager( Could we store this so that your new test needn't recreate it as part of the test? http://gerrit.cloudera.org:8080/#/c/12462/2/src/kudu/tablet/transactions/alter_schema_transaction.cc File src/kudu/tablet/transactions/alter_schema_transaction.cc: http://gerrit.cloudera.org:8080/#/c/12462/2/src/kudu/tablet/transactions/alter_schema_transaction.cc@153 PS2, Line 153: } : : // The schema lock was acquired by Tablet::CreatePreparedAlterSchem > Nothing has changed, so I'm inclined to say no. That said, I'm not sure if I'm wondering, if this is skipped, whether a client waiting on an ongoing AlterTable operation will be notified of its completion. Tablet.AlterSchema succeeded, which means the metadata's schema_version_ was updated, which would make it into the next heartbeat if a full heartbeat was scheduled. But without marking the tablet as dirty, will that happen? If so, why? -- To view, visit http://gerrit.cloudera.org:8080/12462 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id761851741297e29a4666bec0c34fc4f7285f715 Gerrit-Change-Number: 12462 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 19 Feb 2019 20:11:18 + Gerrit-HasComments: Yes
[kudu-CR] [client] extra validation on rows upon Session::Apply()
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12512 ) Change subject: [client] extra validation on rows upon Session::Apply() .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/12512/1/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/12512/1/src/kudu/client/client-test.cc@5887 PS1, Line 5887: ASSERT_OK(session->SetFlushMode(KuduSession::MANUAL_FLUSH)); Would AUTO_FLUSH_SYNC help shorten these tests by obviating the need for an explicit Flush() call? Maybe you could reduce another LOC or two by testing each case through a lambda that takes care of session creation, op application, and flushing? http://gerrit.cloudera.org:8080/#/c/12512/1/src/kudu/client/write_op.h File src/kudu/client/write_op.h: http://gerrit.cloudera.org:8080/#/c/12512/1/src/kudu/client/write_op.h@107 PS1, Line 107: spot spotted http://gerrit.cloudera.org:8080/#/c/12512/1/src/kudu/client/write_op.h@111 PS1, Line 111: virtual Status Verify() const; Is this safe from an ABI compatibility perspective? The KDE wiki suggests the answer is 'no' (https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B), but I'm wondering whether it's only an issue for applications that subclass a base class that we provide, which isn't the case here (i.e. we don't expect third parties to subclass KuduWriteOperation). Anyway, you could do the validation within KuduSession using a switch on type(), thus obviating the need for a new virtual function. -- To view, visit http://gerrit.cloudera.org:8080/12512 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I822f20f3242df7983c63ac146f298acfcfbafc68 Gerrit-Change-Number: 12512 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 20:28:04 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/integration-tests/security-itest.cc@330 PS14, Line 330: KUDU_RETURN_NOT_OK_LOG(GetLocalNetworks(&local_networks), ERROR, > You can use just RETURN_NOT_OK; the KUDU_ prefix is only really intended fo Done http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/util/net/net_util.h File src/kudu/util/net/net_util.h: http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/util/net/net_util.h@96 PS14, Line 96: const > No need for const. Done http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/util/net/net_util.cc@311 PS14, Line 311: std:: > Drop the std:: prefix. Done http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/socket.cc File src/kudu/util/net/socket.cc: http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/socket.cc@304 PS10, Line 304: return true; > Do we need to further condition this local.IsAnyLocalAddress()? I'm guessin I think, we do not need to condition this on local.isAnyLocalAddress, because there is no possible scenario where the remote is a loopback and the connection is coming from outside. If a network interface is seeing a connection from 127.x.x.x, this connection is guaranteed to be local. However, the condition "local.IsAnyLocalAddress() AND NOT remote.IsAnyLocalAddress()" does not guarantee that the connection is local. Also, here is the conversation between Dan and Todd on KUDU-1900 about this: D: So the check would become 'remote addr is in 127.0.0.0/8 OR remote addr == local addr'? I think that should be safe. T: Yea, I think if the remote address is in that loopback subnet that you mentioned we can skip TLS. -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 10 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 19:02:47 + Gerrit-HasComments: Yes
[kudu-CR] Fix some compilation warnings
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12522 )
Change subject: Fix some compilation warnings
..
Patch Set 2:
(1 comment)
gcc continues to frustrate. By inspection it's clear that these aren't
problematic (i.e. an OUT parameter must be set if a RETURN_NOT_OK doesn't
return).
http://gerrit.cloudera.org:8080/#/c/12522/2/src/kudu/util/logging.cc
File src/kudu/util/logging.cc:
http://gerrit.cloudera.org:8080/#/c/12522/2/src/kudu/util/logging.cc@177
PS2, Line 177: if (write(STDERR_FILENO, msg, arraysize(msg)) < 0) {
: // Ignore errors.
: }
Would ignore_result(write(...)) work? See gutil/basictypes.h.
--
To view, visit http://gerrit.cloudera.org:8080/12522
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia11e3a3ec1b81332aac0b1c841a78d9fb145c33d
Gerrit-Change-Number: 12522
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <405403...@qq.com>
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 19 Feb 2019 18:53:49 +
Gerrit-HasComments: Yes
[kudu-CR] [test] Use the kudu binary in the external mini cluster
Hello Tidy Bot, Mike Percy, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12523 to look at the new patch set (#4). Change subject: [test] Use the kudu binary in the external mini cluster .. [test] Use the kudu binary in the external mini cluster This patch changes the externa mini cluster to use the kudu binary and it’s ability to start a master and tserver instead of the kudu-master and kudu-tserver binaries. This means the kudu-binary jar only needs to ship a single binary. Change-Id: Ie963f5d50571f3d5ce55e40fcded245bf29b1b55 --- M src/kudu/integration-tests/minidump_generation-itest.cc M src/kudu/integration-tests/security-faults-itest.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/tools/tool_action_tserver.cc M src/kudu/tserver/tablet_server_main.cc 5 files changed, 26 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/12523/4 -- To view, visit http://gerrit.cloudera.org:8080/12523 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie963f5d50571f3d5ce55e40fcded245bf29b1b55 Gerrit-Change-Number: 12523 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-1900: add loopback check and test
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12474 ) Change subject: KUDU-1900: add loopback check and test .. Patch Set 14: (4 comments) http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/integration-tests/security-itest.cc@330 PS14, Line 330: KUDU_RETURN_NOT_OK(GetLocalNetworks(&local_networks)); You can use just RETURN_NOT_OK; the KUDU_ prefix is only really intended for the C++ thirdparty client (to ensure that our macros are "namespaced" and won't collide with someone else's RETURN_NOT_OK). http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/util/net/net_util.h File src/kudu/util/net/net_util.h: http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/util/net/net_util.h@96 PS14, Line 96: const No need for const. http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: http://gerrit.cloudera.org:8080/#/c/12474/14/src/kudu/util/net/net_util.cc@311 PS14, Line 311: std:: Drop the std:: prefix. http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/socket.cc File src/kudu/util/net/socket.cc: http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/util/net/socket.cc@304 PS10, Line 304: return true; > Do we need to further condition this local.IsAnyLocalAddress()? I'm guessin Could you address this question? -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 14 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Feb 2019 18:38:37 + Gerrit-HasComments: Yes
[kudu-CR] [sentry] add privilege scope validation to SentryAuthzProvider
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider
..
Patch Set 2:
(6 comments)
Took a quick look at this. I'm not sure I understand the essence of the Sentry
scoping, it seems I need to revisit this once more.
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@12
PS2, Line 12: a privilege implies another when,
: 1. the authorizable from the former implies the authorizable
from the
: latter,
: 2. the action from the former implies the action from the latter,
: 3. and grant option from the former implies the grant option
from the
: latter.
Is there a document about that? A link to Sentry's docs would be helpful.
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@33
PS2, Line 33: sentry API will return anything that matches
> Understanding this requires an understanding of the Sentry API, what it ret
+1
So, in this case the list of returned privileges will contain all ALTER, DROP
and RENAME even if we are not interested in that?
http://gerrit.cloudera.org:8080/#/c/12500/2//COMMIT_MSG@39
PS2, Line 39: This patch adds privilge scope validation to SentryAuthzProvider
to
: ensure only authorizable with a higher privilege scope on the
hierarchy
: can imply authorizables with a lower scope on the hierarchy.
What would be the problem if not having this patch? Could you elaborate on
details? An example would be great.
http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/master/sentry_authz_provider-test.cc@425
PS2, Line 425: AuthzOptions {
: true,
: SentryPrivilegeScope::Scope::TABLE,
: },
: AuthzOptions {
: false,
: SentryPrivilegeScope::Scope::TABLE,
: },
: AuthzOptions {
: true,
: SentryPrivilegeScope::Scope::DATABASE,
: },
: AuthzOptions {
: false,
: SentryPrivilegeScope::Scope::DATABASE,
: },
: AuthzOptions {
: true,
: SentryPrivilegeScope::Scope::SERVER,
: },
: AuthzOptions {
: false,
: SentryPrivilegeScope::Scope::SERVER,
: }
Did you want just the Cartesian product? If so, maybe use separate parameters
and use ::testing::Combine(); there is an example in
https://github.com/apache/kudu/blob/2be008763cda5eb28c0dfa8863e396575fce91ab/src/kudu/tools/kudu-admin-test.cc#L406
http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/sentry/sentry_privilege_scope.h
File src/kudu/sentry/sentry_privilege_scope.h:
http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/sentry/sentry_privilege_scope.h@58
PS2, Line 58: imply
implies
http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/sentry/sentry_privilege_scope.cc
File src/kudu/sentry/sentry_privilege_scope.cc:
http://gerrit.cloudera.org:8080/#/c/12500/2/src/kudu/sentry/sentry_privilege_scope.cc@42
PS2, Line 42: return "";
maybe:
LOG(FATAL) << static_cast(scope) << ": unknown scope";
return nullptr;
--
To view, visit http://gerrit.cloudera.org:8080/12500
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a
Gerrit-Change-Number: 12500
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 19 Feb 2019 18:24:09 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1900: add loopback check and test
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12474 to look at the new patch set (#14). Change subject: KUDU-1900: add loopback check and test .. KUDU-1900: add loopback check and test This change modifies Socket::IsLoopbackConnection to check if remote IP is in 127.0.0.0/8 subnet. With this change all connections from 127.0.0.0/8 are treated as local. This change adds force_external_client_ip parameter to AuthTokenIssuingTest. Before this change, AuthTokenIssuingTest relied on Kudu treating connections between non-matching addresses in 127.0.0.0/8 subnet as external. With this change, the test forces Kudu client to use an non-loopback IP address for test cases that verify external connections. If an external IP address is not available, test cases that require an external IP will be skipped. For convenience, this change adds two static utility methods (HostPort::IsLoopback and HostPort::AddrToString), and refactors Sockaddr::host and Sockaddr::IsAnyLocalAddress to use these static methods. Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f --- M src/kudu/integration-tests/security-itest.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h M src/kudu/util/net/sockaddr.cc M src/kudu/util/net/socket.cc 5 files changed, 163 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/12474/14 -- To view, visit http://gerrit.cloudera.org:8080/12474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f Gerrit-Change-Number: 12474 Gerrit-PatchSet: 14 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1900: add loopback check and test
Greg Solovyev has posted comments on this change. (
http://gerrit.cloudera.org:8080/12474 )
Change subject: KUDU-1900: add loopback check and test
..
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/integration-tests/security-itest.cc@330
PS10, Line 330: KUDU_RETURN_NOT_OK_LOG(GetLocalNetworks(&local_networks),
ERROR,
> Let's take an example: an operation whose execution makes a tree-like graph
Got it. Thanks a lot for the explanation!
http://gerrit.cloudera.org:8080/#/c/12474/10/src/kudu/integration-tests/security-itest.cc@334
PS10, Line 334: if (!network.IsLoopBack()) {
> Nit: indentation
Done
--
To view, visit http://gerrit.cloudera.org:8080/12474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3483a9729ddeeb7901e3738532a45b49e713208f
Gerrit-Change-Number: 12474
Gerrit-PatchSet: 10
Gerrit-Owner: Greg Solovyev
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 19 Feb 2019 18:04:19 +
Gerrit-HasComments: Yes
[kudu-CR] [test] Use the kudu binary in the external mini cluster
Hello Tidy Bot, Mike Percy, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12523 to look at the new patch set (#3). Change subject: [test] Use the kudu binary in the external mini cluster .. [test] Use the kudu binary in the external mini cluster This patch changes the externa mini cluster to use the kudu binary and it’s ability to start a master and tserver instead of the kudu-master and kudu-tserver binaries. This means the kudu-binary jar only needs to ship a single binary. Change-Id: Ie963f5d50571f3d5ce55e40fcded245bf29b1b55 --- M src/kudu/integration-tests/minidump_generation-itest.cc M src/kudu/integration-tests/security-faults-itest.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/tools/tool_action_tserver.cc M src/kudu/tserver/tablet_server_main.cc 5 files changed, 25 insertions(+), 21 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/12523/3 -- To view, visit http://gerrit.cloudera.org:8080/12523 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie963f5d50571f3d5ce55e40fcded245bf29b1b55 Gerrit-Change-Number: 12523 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [test] Use the kudu binary in the external mini cluster
Hello Tidy Bot, Mike Percy, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12523 to look at the new patch set (#2). Change subject: [test] Use the kudu binary in the external mini cluster .. [test] Use the kudu binary in the external mini cluster This patch changes the externa mini cluster to use the kudu binary and it’s ability to start a master and tserver instead of the kudu-master and kudu-tserver binaries. This means the kudu-binary jar only needs to ship a single binary. Change-Id: Ie963f5d50571f3d5ce55e40fcded245bf29b1b55 --- M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/tools/tool_action_tserver.cc M src/kudu/tserver/tablet_server_main.cc 3 files changed, 20 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/12523/2 -- To view, visit http://gerrit.cloudera.org:8080/12523 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie963f5d50571f3d5ce55e40fcded245bf29b1b55 Gerrit-Change-Number: 12523 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] Fix some compilation warnings
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12522 to look at the new patch set (#2). Change subject: Fix some compilation warnings .. Fix some compilation warnings Fix compilation warnings under CentOS 7.3 with gcc 4.8.5, and Ubuntu 18.04 with gcc 7.3.0 Change-Id: Ia11e3a3ec1b81332aac0b1c841a78d9fb145c33d --- M src/kudu/common/row_changelist.cc M src/kudu/common/row_changelist.h M src/kudu/consensus/consensus_queue.cc M src/kudu/consensus/log_util.cc M src/kudu/tablet/deltafile.cc M src/kudu/util/env_posix.cc M src/kudu/util/logging.cc 7 files changed, 13 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/12522/2 -- To view, visit http://gerrit.cloudera.org:8080/12522 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia11e3a3ec1b81332aac0b1c841a78d9fb145c33d Gerrit-Change-Number: 12522 Gerrit-PatchSet: 2 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [test] Use the kudu binary in the external mini cluster
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12523 Change subject: [test] Use the kudu binary in the external mini cluster .. [test] Use the kudu binary in the external mini cluster This patch changes the externa mini cluster to use the kudu binary and it’s ability to start a master and tserver instead of the kudu-master and kudu-tserver binaries. This means the kudu-binary jar only needs to ship a single binary. Change-Id: Ie963f5d50571f3d5ce55e40fcded245bf29b1b55 --- M src/kudu/mini-cluster/external_mini_cluster.cc 1 file changed, 11 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/12523/1 -- To view, visit http://gerrit.cloudera.org:8080/12523 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ie963f5d50571f3d5ce55e40fcded245bf29b1b55 Gerrit-Change-Number: 12523 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] [tools] Support starting master and tablet server via the kudu binary
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12517 to look at the new patch set (#3). Change subject: [tools] Support starting master and tablet server via the kudu binary .. [tools] Support starting master and tablet server via the kudu binary Adds commands to the kudu tools to run the master and tablet server: - `kudu master start ...` - `kudu tserver start …` This means we can ship a single binary in the kudu docker image or potentially the kudu-binary jar reducing the size by appoximately 66%. Though follow up changes may be needed in the kudu-binary case. The behavior is the same as running the kudu-master and kudu-tserver binaries except that the log name is based on the binary name, so be default the log will be kudu. instead of kudu-master. or kudu-tserver.. The log_filename flag is added as an optional parameter to help make this naming convention clear and show it’s easily overridable. This patch also contains a few releated fixes for default flags ensuring that they can be overwritten when neccessary. These changes also ensure flags are not incorrectly reported as non-default. Change-Id: I3717cbac930b3506a76f7a51388c64afbcbb480e --- M src/kudu/master/master_main.cc M src/kudu/tools/tool_action_master.cc M src/kudu/tools/tool_action_tserver.cc M src/kudu/tools/tool_main.cc M src/kudu/tserver/tablet_server.h M src/kudu/tserver/tablet_server_main.cc M src/kudu/util/flags.cc M src/kudu/util/flags.h M src/kudu/util/logging.cc 9 files changed, 222 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/12517/3 -- To view, visit http://gerrit.cloudera.org:8080/12517 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3717cbac930b3506a76f7a51388c64afbcbb480e Gerrit-Change-Number: 12517 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Fix some compilation warnings
Yingchun Lai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12522 Change subject: Fix some compilation warnings .. Fix some compilation warnings Fix compilation warnings under CentOS 7.3 with gcc 4.8.5, and Ubuntu 18.04 with gcc 7.3.0 Change-Id: Ia11e3a3ec1b81332aac0b1c841a78d9fb145c33d --- M src/kudu/common/row_changelist.cc M src/kudu/common/row_changelist.h M src/kudu/consensus/consensus_queue.cc M src/kudu/consensus/log_util.cc M src/kudu/tablet/deltafile.cc M src/kudu/util/env_posix.cc M src/kudu/util/logging.cc 7 files changed, 13 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/12522/1 -- To view, visit http://gerrit.cloudera.org:8080/12522 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ia11e3a3ec1b81332aac0b1c841a78d9fb145c33d Gerrit-Change-Number: 12522 Gerrit-PatchSet: 1 Gerrit-Owner: Yingchun Lai <405403...@qq.com>
