[kudu-CR] KUDU-2238. DMS not flush under memory pressure.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8904 ) Change subject: KUDU-2238. DMS not flush under memory pressure. .. Patch Set 4: Code-Review+2 Looks good. Please report back if this helps with your workload -- To view, visit http://gerrit.cloudera.org:8080/8904 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0c04d76aa0e3888352dc56eeb493a5437ef47e42 Gerrit-Change-Number: 8904 Gerrit-PatchSet: 4 Gerrit-Owner: zhen.zhang Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: zhen.zhang Gerrit-Comment-Date: Wed, 17 Jan 2018 07:54:15 + Gerrit-HasComments: No
[kudu-CR] KUDU-2238. DMS not flush under memory pressure.
zhen.zhang has posted comments on this change. (
http://gerrit.cloudera.org:8080/8904 )
Change subject: KUDU-2238. DMS not flush under memory pressure.
..
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8904/2/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/8904/2/src/kudu/tablet/tablet.cc@1909
PS2, Line 1909: (score > max_score - 1 && mem > mem_size)) {
> Good point.
Good idea, thanks Todd. Already updated.
--
To view, visit http://gerrit.cloudera.org:8080/8904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0c04d76aa0e3888352dc56eeb493a5437ef47e42
Gerrit-Change-Number: 8904
Gerrit-PatchSet: 4
Gerrit-Owner: zhen.zhang
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: zhen.zhang
Gerrit-Comment-Date: Wed, 17 Jan 2018 07:47:52 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2238. DMS not flush under memory pressure.
Hello Mike Percy, Jean-Daniel Cryans, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8904 to look at the new patch set (#4). Change subject: KUDU-2238. DMS not flush under memory pressure. .. KUDU-2238. DMS not flush under memory pressure. When we choose DMS to flush, now we always pick the DMS with highest log retention. However, as KUDU-2238 shows, in some cases DMS with highest log retention may only consume little memory, and other DMSs may consume much more memory, but get no chance to be flushed, even under memory pressure. This patch gives the ability to take memory consumption into consideration when we choose DMS. Change-Id: I0c04d76aa0e3888352dc56eeb493a5437ef47e42 --- M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_replica_mm_ops.cc 3 files changed, 18 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8904/4 -- To view, visit http://gerrit.cloudera.org:8080/8904 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0c04d76aa0e3888352dc56eeb493a5437ef47e42 Gerrit-Change-Number: 8904 Gerrit-PatchSet: 4 Gerrit-Owner: zhen.zhang Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: zhen.zhang
[kudu-CR] KUDU-2238. DMS not flush under memory pressure.
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/8904 )
Change subject: KUDU-2238. DMS not flush under memory pressure.
..
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8904/2/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/8904/2/src/kudu/tablet/tablet.cc@1909
PS2, Line 1909: (size == retention_size && mem > mem_size)) {
> As both score and max_score are double, maybe it's not good to compare them
Good point.
In this case I think we should go with something like:
score > max_score - 1 && mem > mem_size
(we don't need to worry about std::abs since we already compared above the case
where score > max_score). Since the units of score is bytes*100 here we expect
large numbers and so I think "- 1" is pretty reasonable vs 0.0001 or something.
--
To view, visit http://gerrit.cloudera.org:8080/8904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0c04d76aa0e3888352dc56eeb493a5437ef47e42
Gerrit-Change-Number: 8904
Gerrit-PatchSet: 2
Gerrit-Owner: zhen.zhang
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: zhen.zhang
Gerrit-Comment-Date: Wed, 17 Jan 2018 07:35:44 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2238. DMS not flush under memory pressure.
zhen.zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/8904 ) Change subject: KUDU-2238. DMS not flush under memory pressure. .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/8904/2/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/8904/2/src/kudu/tablet/tablet.cc@1909 PS2, Line 1909: double score = mem * mem_weight + size * (100 - mem_weight); > should this tie-breaker line now say "score == max_score && mem > mem_size" As both score and max_score are double, maybe it's not good to compare them, and "score==max_score" can represent the same logic, so I leave it here. BTW, I'm not sure how we do double comparison in kudu, is "std::abs(score-max_score) < 0.0001" good? -- To view, visit http://gerrit.cloudera.org:8080/8904 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0c04d76aa0e3888352dc56eeb493a5437ef47e42 Gerrit-Change-Number: 8904 Gerrit-PatchSet: 3 Gerrit-Owner: zhen.zhang Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: zhen.zhang Gerrit-Comment-Date: Wed, 17 Jan 2018 07:30:38 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2208 Add RETRY ON EINTR() to Subprocess
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9015 ) Change subject: KUDU-2208 Add RETRY_ON_EINTR() to Subprocess .. KUDU-2208 Add RETRY_ON_EINTR() to Subprocess This patch submits a unit test that create a Subprocess thread and while it is starting and waiting, another thread sends kill signals to the Subprocess thread. Since the pthread_kill() signal is sent asynchronously, sometimes the pthread_kill() raises error signal 3. In that condition, the unit test logs the incident as an INFO. Prior to this bug fix patch, the Subprocess throws an exception because it cannot handle the kill signal in Wait() state. To fix the bug, we add RETRY_ON_EINTR() inside Subprocess::DoWait() function. Furthermore, this patch also moves the RETRY_ON_EINTR() function that occurs in many places across the code to os-util.h and it adds alarm reset in the end of TestReadFromStdoutAndStderr in Subprocess-test. Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333 Reviewed-on: http://gerrit.cloudera.org:8080/9015 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon --- M src/kudu/consensus/log_index.cc M src/kudu/util/env_posix.cc M src/kudu/util/net/socket.cc M src/kudu/util/os-util.h M src/kudu/util/subprocess-test.cc M src/kudu/util/subprocess.cc 6 files changed, 77 insertions(+), 22 deletions(-) Approvals: Kudu Jenkins: Verified Todd Lipcon: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/9015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333 Gerrit-Change-Number: 9015 Gerrit-PatchSet: 13 Gerrit-Owner: Jeffrey F. Lukman Gerrit-Reviewer: Jeffrey F. Lukman Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2208 Add RETRY ON EINTR() to Subprocess
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9015 ) Change subject: KUDU-2208 Add RETRY_ON_EINTR() to Subprocess .. Patch Set 12: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/9015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333 Gerrit-Change-Number: 9015 Gerrit-PatchSet: 12 Gerrit-Owner: Jeffrey F. Lukman Gerrit-Reviewer: Jeffrey F. Lukman Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Jan 2018 07:30:34 + Gerrit-HasComments: No
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9027 to look at the new patch set (#11). Change subject: KUDU-1489: allow configuration of metadata dir .. KUDU-1489: allow configuration of metadata dir Metadata files currently reside in the first configured data directory, which isn't always the best choice; this first drive, if not performant, can act as a performance bottleneck. Often times the drive containing the WALs is a better choice, as it is recommended to be the fastest. This patch allows users to configure their metadata directory through the new fs_metadata_dir flag. If empty, Kudu will check if a metadata directory exists in the first member of fs_data_dirs, and if none exists, Kudu will use fs_wal_dir as the root directory for metadata. Otherwise, the flag will be honored verbatim. Aside from the update to the directory location, codepaths that previously assumed that the first data directory _must_ be healthy have been updated. The remaining invariant is that at least a single data directory must be healthy. The following test changes are included: - fs_manager-test: unit tests for various scenarios - data_dirs-test: a test case is updated to show that we can now open the directory manager with a failed first data directory (previously this codepath would hit D/CHECK failures) - mini_tablet_server-test: an end-to-end test is added to manually go back and forth between the old and new default metadata directories - kudu-tool-test: a small test that tools still work, but only when the appropriate FS flags are provided Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db --- M src/kudu/fs/block_manager_util.cc M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tserver/mini_tablet_server-test.cc 11 files changed, 328 insertions(+), 69 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/11 -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 ) Change subject: KUDU-1489: allow configuration of metadata dir .. Patch Set 11: (4 comments) http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/data_dirs.cc@728 PS10, Line 728: CHECK_NE(first_healthy, -1); // Guaranteed by LoadInstances(). > maybe just make this a CHECK since it'll avoid some ugly stack overwriting Done http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc@94 PS10, Line 94: compatibility > typo Done http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc@96 PS10, Line 96: exist > nit: exists Done http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc@98 PS10, Line 98: TAG_FLAG(fs_metadata_dir, stable); > maybe this should be stable out of the gate? Done -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Jan 2018 07:29:34 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2195 (part 1): always sync PBC-format metadata files
Hello Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/9043 to review the following change. Change subject: KUDU-2195 (part 1): always sync PBC-format metadata files .. KUDU-2195 (part 1): always sync PBC-format metadata files This changes the writing of metadata files to always fsync data before renaming into place. Previously, we didn't do this in a few cases, most notably for consensus metadata when WAL fsync is off (the default). However, ext4 already has some automatic fsync when it detects the commmon "write and rename-to-replace" paradigm used by a lot of software. So, adding the explicit fsync isn't likely to slow down ext4 much. More importantly, xfs does _not_ have this behavior, and we've recently seen some issues on an xfs-enabled system where consensus metadata files were unexpectedly zero-length or appear to have lost term changes, causing tablets to fail to initialize. Initial investigation seems to indicate that the hosts with these issues had experienced some hard resets, lending further credence to the theory that it was due to lack of syncing metadata. Lots of good background reading can be found here (particularly in the comments): https://lwn.net/Articles/351422/ Change-Id: I4f0c911662b2ff35fcf3915790248ba85bf6026f --- M src/kudu/consensus/consensus_meta.cc M src/kudu/fs/block_manager_util.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/fs_manager.cc M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc M src/kudu/server/server_base.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/util/pb_util-test.cc M src/kudu/util/pb_util.cc M src/kudu/util/pb_util.h 11 files changed, 48 insertions(+), 76 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9043/1 -- To view, visit http://gerrit.cloudera.org:8080/9043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I4f0c911662b2ff35fcf3915790248ba85bf6026f Gerrit-Change-Number: 9043 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo
[kudu-CR] KUDU-2261: The order of the responses after flush should match the order we call apply
Hello Mike Percy, Jean-Daniel Cryans, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9029 to look at the new patch set (#2). Change subject: KUDU-2261: The order of the responses after flush should match the order we call apply .. KUDU-2261: The order of the responses after flush should match the order we call apply The response list of flush() should have the same order of we apply operations, so it's easier to know which operation failed and which succeeded. Change-Id: Ib37c9e85ad03731bb7d5b83be77d40fcd95e803a --- 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/test/java/org/apache/kudu/client/TestKuduSession.java 4 files changed, 71 insertions(+), 10 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/9029/2 -- To view, visit http://gerrit.cloudera.org:8080/9029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib37c9e85ad03731bb7d5b83be77d40fcd95e803a Gerrit-Change-Number: 9029 Gerrit-PatchSet: 2 Gerrit-Owner: zhen.zhang Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: zhen.zhang
[kudu-CR] KUDU-2208 Add RETRY ON EINTR() to Subprocess
Jeffrey F. Lukman has posted comments on this change. ( http://gerrit.cloudera.org:8080/9015 ) Change subject: KUDU-2208 Add RETRY_ON_EINTR() to Subprocess .. Patch Set 12: (4 comments) http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/os-util.h File src/kudu/util/os-util.h: http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/os-util.h@27 PS10, Line 27: > should probably #include here since EINTR is defined there Done http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/subprocess-test.cc File src/kudu/util/subprocess-test.cc: http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/subprocess-test.cc@338 PS10, Line 338: ASSERT_TRUE(err == 0 || err == ESRCH); > nit: I think this would read better as ASSERT_TRUE(err == 0 || err == ESRCH Done http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/subprocess-test.cc@340 PS10, Line 340: LOG(INFO) << "Async kill signal failed with err=" << err << > can you add an ASSERT_TRUE(t_finished) here? we should only get ESRCH in th Done http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/subprocess-test.cc@344 PS10, Line 344: // Add microseconds delay to make the unit test runs faster and more reliable > can you change this to rand() % 1? It still runs quickly on my machine with Done -- To view, visit http://gerrit.cloudera.org:8080/9015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333 Gerrit-Change-Number: 9015 Gerrit-PatchSet: 12 Gerrit-Owner: Jeffrey F. Lukman Gerrit-Reviewer: Jeffrey F. Lukman Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Jan 2018 06:44:32 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2208 Add RETRY ON EINTR() to Subprocess
Hello Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9015 to look at the new patch set (#12). Change subject: KUDU-2208 Add RETRY_ON_EINTR() to Subprocess .. KUDU-2208 Add RETRY_ON_EINTR() to Subprocess This patch submits a unit test that create a Subprocess thread and while it is starting and waiting, another thread sends kill signals to the Subprocess thread. Since the pthread_kill() signal is sent asynchronously, sometimes the pthread_kill() raises error signal 3. In that condition, the unit test logs the incident as an INFO. Prior to this bug fix patch, the Subprocess throws an exception because it cannot handle the kill signal in Wait() state. To fix the bug, we add RETRY_ON_EINTR() inside Subprocess::DoWait() function. Furthermore, this patch also moves the RETRY_ON_EINTR() function that occurs in many places across the code to os-util.h and it adds alarm reset in the end of TestReadFromStdoutAndStderr in Subprocess-test. Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333 --- M src/kudu/consensus/log_index.cc M src/kudu/util/env_posix.cc M src/kudu/util/net/socket.cc M src/kudu/util/os-util.h M src/kudu/util/subprocess-test.cc M src/kudu/util/subprocess.cc 6 files changed, 77 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/9015/12 -- To view, visit http://gerrit.cloudera.org:8080/9015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333 Gerrit-Change-Number: 9015 Gerrit-PatchSet: 12 Gerrit-Owner: Jeffrey F. Lukman Gerrit-Reviewer: Jeffrey F. Lukman Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2208 Add RETRY ON EINTR() to Subprocess
Jeffrey F. Lukman has abandoned this change. ( http://gerrit.cloudera.org:8080/9042 ) Change subject: KUDU-2208 Add RETRY_ON_EINTR() to Subprocess .. Abandoned Wrong commit -- To view, visit http://gerrit.cloudera.org:8080/9042 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: Iffabada54eb2359730e9e2973b263d71645fa572 Gerrit-Change-Number: 9042 Gerrit-PatchSet: 1 Gerrit-Owner: Jeffrey F. Lukman Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2208 Add RETRY ON EINTR() to Subprocess
Jeffrey F. Lukman has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9042 Change subject: KUDU-2208 Add RETRY_ON_EINTR() to Subprocess .. KUDU-2208 Add RETRY_ON_EINTR() to Subprocess This patch submits a unit test that create a Subprocess thread and while it is starting and waiting, another thread sends kill signals to the Subprocess thread. Since the pthread_kill() signal is sent asynchronously, sometimes the pthread_kill() raises error signal 3. In that condition, the unit test logs the incident as an INFO. Prior to this bug fix patch, the Subprocess throws an exception because it cannot handle the kill signal in Wait() state. To fix the bug, we add RETRY_ON_EINTR() inside Subprocess::DoWait() function. Furthermore, this patch also moves the RETRY_ON_EINTR() function that occurs in many places across the code to os-util.h and it adds alarm reset in the end of TestReadFromStdoutAndStderr in Subprocess-test Change-Id: Iffabada54eb2359730e9e2973b263d71645fa572 --- M src/kudu/util/os-util.h M src/kudu/util/subprocess-test.cc 2 files changed, 5 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/9042/1 -- To view, visit http://gerrit.cloudera.org:8080/9042 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Iffabada54eb2359730e9e2973b263d71645fa572 Gerrit-Change-Number: 9042 Gerrit-PatchSet: 1 Gerrit-Owner: Jeffrey F. Lukman
[kudu-CR] tools: Add debug mode to pb dump tool
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9024 ) Change subject: tools: Add debug mode to pb dump tool .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/9024/1/src/kudu/util/pb_util.cc File src/kudu/util/pb_util.cc: http://gerrit.cloudera.org:8080/#/c/9024/1/src/kudu/util/pb_util.cc@930 PS1, Line 930: // Fallthrough. use FALLTHROUGH_INTENDED from macros.h which has some magic effect on clang. Or, since this is just the common case of multiple case labels, and not a case where there is some code for DEFAULT which then falls through to DEBUG, I don't think it's really necessary in the first place. Mostly I like to document fallthrough in cases like: case A: some code; some more code; case B: even more code; break; because it looks like someone forgot the 'break' on case A. -- To view, visit http://gerrit.cloudera.org:8080/9024 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ica2a74a2e252d140cf7704ff68037a64db19cd80 Gerrit-Change-Number: 9024 Gerrit-PatchSet: 1 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Jan 2018 06:22:28 + Gerrit-HasComments: Yes
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/7821 ) Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions .. Patch Set 3: Code-Review+1 lgtm, please test using LD_LIBRARY_PATH against both versions before pushing just in case we missed something -- To view, visit http://gerrit.cloudera.org:8080/7821 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7 Gerrit-Change-Number: 7821 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Jan 2018 06:18:38 + Gerrit-HasComments: No
[kudu-CR] KUDU-2208 Add RETRY ON EINTR() to Subprocess
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9015 ) Change subject: KUDU-2208 Add RETRY_ON_EINTR() to Subprocess .. Patch Set 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/os-util.h File src/kudu/util/os-util.h: http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/os-util.h@27 PS10, Line 27: #include should probably #include here since EINTR is defined there http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/subprocess-test.cc File src/kudu/util/subprocess-test.cc: http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/subprocess-test.cc@338 PS10, Line 338: ASSERT_FALSE(err != 0 && err != ESRCH); nit: I think this would read better as ASSERT_TRUE(err == 0 || err == ESRCH) http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/subprocess-test.cc@340 PS10, Line 340: LOG(INFO) << "Async kill signal failed with err=" << err << can you add an ASSERT_TRUE(t_finished) here? we should only get ESRCH in the case where the thread finished http://gerrit.cloudera.org:8080/#/c/9015/10/src/kudu/util/subprocess-test.cc@344 PS10, Line 344: SleepFor(MonoDelta::FromMicroseconds(rand() % 100)); can you change this to rand() % 1? It still runs quickly on my machine with that setting and seems more likely to produce races -- To view, visit http://gerrit.cloudera.org:8080/9015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333 Gerrit-Change-Number: 9015 Gerrit-PatchSet: 10 Gerrit-Owner: Jeffrey F. Lukman Gerrit-Reviewer: Jeffrey F. Lukman Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Jan 2018 06:16:51 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2238. DMS not flush under memory pressure.
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/8904 )
Change subject: KUDU-2238. DMS not flush under memory pressure.
..
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8904/2/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/8904/2/src/kudu/tablet/tablet.cc@1909
PS2, Line 1909: (size == retention_size && mem > mem_size)) {
should this tie-breaker line now say "score == max_score && mem > mem_size"?
--
To view, visit http://gerrit.cloudera.org:8080/8904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0c04d76aa0e3888352dc56eeb493a5437ef47e42
Gerrit-Change-Number: 8904
Gerrit-PatchSet: 2
Gerrit-Owner: zhen.zhang
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: zhen.zhang
Gerrit-Comment-Date: Wed, 17 Jan 2018 06:12:12 +
Gerrit-HasComments: Yes
[kudu-CR](branch-1.5.x) KUDU-2193 (part 2): avoid holding TSTabletManager::lock for a long time
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8526 ) Change subject: KUDU-2193 (part 2): avoid holding TSTabletManager::lock_ for a long time .. KUDU-2193 (part 2): avoid holding TSTabletManager::lock_ for a long time This changes tablet report generation to only hold the TSTabletManager lock long enough to copy a list of refs to the relevant tablets. Even though the lock is a reader-writer lock, a long read-side critical section can end up blocking other readers as long as any writer shows up. I saw the following in a cluster: - election storm ongoing - T1 generating a tablet report (thus holding the reader lock) - blocks for a long time getting ConsensusState from some tablets currently mid-fsync. - T2 handling a DeleteTablet or CreateTablet call (waiting for writer lock) - T3 through T20: blocking on reader lock in LookupTablet() The effect here is that all other threads end up blocked until T1 finishes its tablet report generation, which in this case can be tens of seconds due to all of the fsync traffic. These blocked threads then contribute to the ongoing election storm since they may delay timely responses to vote requests from other tablets. I tested this and the previous patch on a cluster that was previously experiencing the issue. I triggered some elections by kill -STOPping some servers and resuming them a few seconds later. Without these patches, other servers started doing lots of context switches (due to the spinlock used prior to this patch series) and I saw lots of blocked threads in pstack. With the patch, things seem cleaner. The following screenshot shows before/after voluntary_context_switch rate: https://i.imgur.com/7zcbImw.png (the missing data around 5pm is my restarting the servers with this patch) Change-Id: I0f645775e8347a18af112b308dba56a3b4a2c681 Reviewed-on: http://gerrit.cloudera.org:8080/8346 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon (cherry picked from commit f2c21b4fac15fcaa5f53a6c7960ecd19a0664c45) Reviewed-on: http://gerrit.cloudera.org:8080/8526 Reviewed-by: Adar Dembo --- M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 2 files changed, 31 insertions(+), 18 deletions(-) Approvals: Kudu Jenkins: Verified Adar Dembo: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/8526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.5.x Gerrit-MessageType: merged Gerrit-Change-Id: I0f645775e8347a18af112b308dba56a3b4a2c681 Gerrit-Change-Number: 8526 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR](branch-1.4.x) KUDU-2193 (part 2): avoid holding TSTabletManager::lock for a long time
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8528 ) Change subject: KUDU-2193 (part 2): avoid holding TSTabletManager::lock_ for a long time .. KUDU-2193 (part 2): avoid holding TSTabletManager::lock_ for a long time This changes tablet report generation to only hold the TSTabletManager lock long enough to copy a list of refs to the relevant tablets. Even though the lock is a reader-writer lock, a long read-side critical section can end up blocking other readers as long as any writer shows up. I saw the following in a cluster: - election storm ongoing - T1 generating a tablet report (thus holding the reader lock) - blocks for a long time getting ConsensusState from some tablets currently mid-fsync. - T2 handling a DeleteTablet or CreateTablet call (waiting for writer lock) - T3 through T20: blocking on reader lock in LookupTablet() The effect here is that all other threads end up blocked until T1 finishes its tablet report generation, which in this case can be tens of seconds due to all of the fsync traffic. These blocked threads then contribute to the ongoing election storm since they may delay timely responses to vote requests from other tablets. I tested this and the previous patch on a cluster that was previously experiencing the issue. I triggered some elections by kill -STOPping some servers and resuming them a few seconds later. Without these patches, other servers started doing lots of context switches (due to the spinlock used prior to this patch series) and I saw lots of blocked threads in pstack. With the patch, things seem cleaner. The following screenshot shows before/after voluntary_context_switch rate: https://i.imgur.com/7zcbImw.png (the missing data around 5pm is my restarting the servers with this patch) Change-Id: I0f645775e8347a18af112b308dba56a3b4a2c681 Reviewed-on: http://gerrit.cloudera.org:8080/8346 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon (cherry picked from commit f2c21b4fac15fcaa5f53a6c7960ecd19a0664c45) Reviewed-on: http://gerrit.cloudera.org:8080/8528 Reviewed-by: Adar Dembo --- M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 2 files changed, 31 insertions(+), 18 deletions(-) Approvals: Kudu Jenkins: Verified Adar Dembo: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/8528 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.4.x Gerrit-MessageType: merged Gerrit-Change-Id: I0f645775e8347a18af112b308dba56a3b4a2c681 Gerrit-Change-Number: 8528 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 ) Change subject: KUDU-1489: allow configuration of metadata dir .. Patch Set 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/data_dirs.cc@728 PS10, Line 728: DCHECK_NE(first_healthy, -1); // Guaranteed by LoadInstances(). maybe just make this a CHECK since it'll avoid some ugly stack overwriting on the next line, and this isn't any sort of perf-critical code http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc@94 PS10, Line 94: compatability typo http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc@96 PS10, Line 96: exist nit: exists http://gerrit.cloudera.org:8080/#/c/9027/10/src/kudu/fs/fs_manager.cc@98 PS10, Line 98: TAG_FLAG(fs_metadata_dir, advanced); maybe this should be stable out of the gate? -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 10 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Jan 2018 06:07:00 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2261: The order of the responses after flush should match the order we call apply
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/9029 )
Change subject: KUDU-2261: The order of the responses after flush should match
the order we call apply
..
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9029/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File
java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/9029/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@22
PS1, Line 22: import java.util.*;
nit: we don't use wildcard imports
http://gerrit.cloudera.org:8080/#/c/9029/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@383
PS1, Line 383: batchResponses.add(Deferred.fromResult(new
BatchResponse(opsFailedInLookup, opsFailedIndexesList)));
nit: please wrap
http://gerrit.cloudera.org:8080/#/c/9029/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@485
PS1, Line 485: for ( int i = 0; i < indexList.size(); i++) {
nit: extra space
http://gerrit.cloudera.org:8080/#/c/9029/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@486
PS1, Line 486: responses[indexList.get(i)] = responseList.get(i);
nit: can you add an assert like:
int idx = indexList.get(i);
assert responses[idx] == null;
responses[idx] = responseList.get(i);
http://gerrit.cloudera.org:8080/#/c/9029/1/java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java:
http://gerrit.cloudera.org:8080/#/c/9029/1/java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java@46
PS1, Line 46: /** Holds indexes of operations. */
can you specify "in the original user's batch"
--
To view, visit http://gerrit.cloudera.org:8080/9029
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib37c9e85ad03731bb7d5b83be77d40fcd95e803a
Gerrit-Change-Number: 9029
Gerrit-PatchSet: 1
Gerrit-Owner: zhen.zhang
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: zhen.zhang
Gerrit-Comment-Date: Wed, 17 Jan 2018 05:53:18 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1913: LIFO wake up ordering for threadpool worker threads
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/9021 )
Change subject: KUDU-1913: LIFO wake up ordering for threadpool worker threads
..
Patch Set 1: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/test_util.h
File src/kudu/util/test_util.h:
http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/test_util.h@115
PS1, Line 115: // Do not use any back-off and loop every millisecond.
nit: it doesn't check every millisecond, but rather sleeps one millisecond
between checks (which might result in checking less frequently in the case that
the checks themselves take some time)
http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/threadpool-test.cc
File src/kudu/util/threadpool-test.cc:
http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/threadpool-test.cc@934
PS1, Line 934: [](){}
> o_O
at least he didn't use the equivalent syntax <:]{%>
http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/threadpool.cc@621
PS1, Line 621: auto cleanup = MakeScopedCleanup([&]() {
can you use the SCOPED_CLEANUP macro instead if you don't need the named
variable for cancellation purposes?
--
To view, visit http://gerrit.cloudera.org:8080/9021
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8036bf0d15f9ffcb3fa76579e3bfe0a340d38320
Gerrit-Change-Number: 9021
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Wed, 17 Jan 2018 05:48:08 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9027 to look at the new patch set (#10). Change subject: KUDU-1489: allow configuration of metadata dir .. KUDU-1489: allow configuration of metadata dir Metadata files currently reside in the first configured data directory, which isn't always the best choice; this first drive, if not performant, can act as a performance bottleneck. Often times the drive containing the WALs is a better choice, as it is recommended to be the fastest. This patch allows users to configure their metadata directory through the new fs_metadata_dir flag. If empty, Kudu will check if a metadata directory exists in the first member of fs_data_dirs, and if none exists, Kudu will use fs_wal_dir as the root directory for metadata. Otherwise, the flag will be honored verbatim. Aside from the update to the directory location, codepaths that previously assumed that the first data directory _must_ be healthy have been updated. The remaining invariant is that at least a single data directory must be healthy. The following test changes are included: - fs_manager-test: unit tests for various scenarios - data_dirs-test: a test case is updated to show that we can now open the directory manager with a failed first data directory (previously this codepath would hit D/CHECK failures) - mini_tablet_server-test: an end-to-end test is added to manually go back and forth between the old and new default metadata directories - kudu-tool-test: a small test that tools still work, but only when the appropriate FS flags are provided Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db --- M src/kudu/fs/block_manager_util.cc M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tserver/mini_tablet_server-test.cc 11 files changed, 327 insertions(+), 69 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/10 -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 10 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] [catalog manager] more info if unable to replace a replica
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9040 ) Change subject: [catalog_manager] more info if unable to replace a replica .. Patch Set 4: > Shouldn't we also periodically print this warning even if the > cluster is healthy and there are no replicas to replace? One option I can see is to print this sort of warning every time a new table created with the replication factor greater than one and equal to the number of tablet servers in the cluster. Would it make sense? -- To view, visit http://gerrit.cloudera.org:8080/9040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id5f562c6d1ff526daa785ea535e440598c03cd37 Gerrit-Change-Number: 9040 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Wed, 17 Jan 2018 04:10:19 + Gerrit-HasComments: No
[kudu-CR] [catalog manager] more info if unable to replace a replica
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9040 ) Change subject: [catalog_manager] more info if unable to replace a replica .. [catalog_manager] more info if unable to replace a replica Output actionable warning message when the catalog manager is unable to find a spot for a replacement replica. Since the 3-4-3 replication scheme is now enabled by default, this might be useful in case if running a cluster with just 3 tablet servers when tables have replication factor of 3. Change-Id: Id5f562c6d1ff526daa785ea535e440598c03cd37 Reviewed-on: http://gerrit.cloudera.org:8080/9040 Reviewed-by: Mike Percy Tested-by: Kudu Jenkins --- M src/kudu/master/catalog_manager.cc M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h 3 files changed, 41 insertions(+), 9 deletions(-) Approvals: Mike Percy: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/9040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Id5f562c6d1ff526daa785ea535e440598c03cd37 Gerrit-Change-Number: 9040 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] [catalog manager] more info if unable to replace a replica
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9040 ) Change subject: [catalog_manager] more info if unable to replace a replica .. Patch Set 3: > Shouldn't we also periodically print this warning even if the > cluster is healthy and there are no replicas to replace? I think that's a good idea. Where would it be the best place to add such a warning? -- To view, visit http://gerrit.cloudera.org:8080/9040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id5f562c6d1ff526daa785ea535e440598c03cd37 Gerrit-Change-Number: 9040 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Wed, 17 Jan 2018 03:17:56 + Gerrit-HasComments: No
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9027 to look at the new patch set (#9). Change subject: KUDU-1489: allow configuration of metadata dir .. KUDU-1489: allow configuration of metadata dir Metadata files currently reside in the first configured data directory, which isn't always the best choice; this first drive, if not performant, can act as a performance bottleneck. Often times the drive containing the WALs is a better choice, as it is recommended to be the fastest. This patch allows users to configure their metadata directory through the new fs_metadata_dir flag. If empty, Kudu will check if a metadata directory exists in the first member of fs_data_dirs, and if none exists, Kudu will use fs_wal_dir as the root directory for metadata. Otherwise, the flag will be honored verbatim. Aside from the update to the directory location, codepaths that previously assumed that the first data directory _must_ be healthy have been updated. The remaining invariant is that at least a single data directory must be healthy. The following test changes are included: - fs_manager-test: unit tests for various scenarios - data_dirs-test: a test case is updated to show that we can now open the directory manager with a failed first data directory (previously this codepath would hit D/CHECK failures) - mini_tablet_server-test: an end-to-end test is added to manually go back and forth between the old and new default metadata directories - kudu-tool-test: a small test that tools still work, but only when the appropriate FS flags are provided Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db --- M src/kudu/fs/block_manager_util.cc M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tserver/mini_tablet_server-test.cc 11 files changed, 327 insertions(+), 69 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/9 -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 9 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir
..
Patch Set 7:
(9 comments)
Done, and added a more integration-y test per David's feedback.
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/block_manager_util.cc
File src/kudu/fs/block_manager_util.cc:
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/block_manager_util.cc@176
PS6, Line 176:
> typo
Done
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@248
PS6, Line 248: "wal"
> could we be more specific? maybe look for full suffix?
Done
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@255
PS6, Line 255: LOG(INFO) << s.ToString();
> leftover from testing? if you want to print the status on failure you can d
Done
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@258
PS6, Line 258:
: TEST_F(FsManagerTestBase, TestMetadataDirInData
> check the status
Per Adar's comment, I moved these each into their own test.
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@263
PS6, Line 263:
: // Creating a brand new FS layout configured with metadata in
the first data
: // directory emulates the default behavior in Kudu 1.6 and
below.
: opts.metadata_root = GetTestPath("data");
: ReinitFsManagerWithOpts(opts);
> in the case before this one you test with a metadata root path that does no
Done
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@269
PS6, Line 269: ASSERT_OK(fs_manager()->Open());
: ASSERT_STR_CONTAINS(fs_manager()->GetTabletMetadataDir(),
"data");
> wait, empty string is different from "unset"? why?
Mm, not sure I understand, what do you mean by "unset" here?
This behavior is for backwards compatibility: older deployments can continue
running with no change to their configurations (i.e. with an empty
fs_metadata_dir flag) while maintaining that their metadata will be in the
first data dir. New deployments will use the fs_wal_dir for metadata if
fs_metadata_dir is empty.
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@276
PS6, Line 276: ASSERT_OK(fs_manager()->Open());
: ASSERT_STR_CONTAINS(fs_manager()->GetTabletMe
> same
Ack
http://gerrit.cloudera.org:8080/#/c/9027/7/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/9027/7/src/kudu/fs/fs_manager-test.cc@534
PS7, Line 534: // Adding a new data dir elsewhere in the list is OK.
> This isn't what the test does anymore though; it's just shuffling the order
Ah you're right, my bad. Removing it. I'll add a similar test up where we test
metadata in the first data root.
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@256
PS4, Line 256: const string meta_dir_in_data_root =
JoinPathSegments(canonicalized_data_fs_roots_[0].path,
> My concern is with the following sequence:
I see. Step 3 would fail at CreateFileSystemRoots(). The canonicalized roots
wouldn't be empty, so this would lead to an AlreadyPresent error.
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 17 Jan 2018 02:57:03 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9027 to look at the new patch set (#8). Change subject: KUDU-1489: allow configuration of metadata dir .. KUDU-1489: allow configuration of metadata dir Metadata files currently reside in the first configured data directory, which isn't always the best choice; this first drive, if not performant, can act as a bottleneck. Often times the drive containing the WALs is a better choice, as it is recommended to be the fastest. This patch allows users to configure their metadata directory through the new fs_metadata_dir flag. If empty, Kudu will check if a metadata directory exists in the first member of fs_data_dirs, and if none exists, Kudu will use fs_wal_dir as the root directory for metadata. Otherwise, the flag will be honored verbatim. Aside from the update to the directory location, codepaths that previously assumed that the first data directory _must_ be healthy have been updated. The remaining invariant is that at least a single data directory must be healthy. The following tests are added: - fs_manager-test: unit tests for various scenarios - data_dirs-test: a test case is updated to show that we can now open the directory manager with a failed first data directory (previously this codepath would hit D/CHECK failures) - mini_tablet_server-test: an end-to-end test is added to switch back and forth between the old and new default metadata directories - kudu-tool-test: a small test that tools still work, but only when the appropriate FS flags are provided Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db --- M src/kudu/fs/block_manager_util.cc M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tserver/mini_tablet_server-test.cc 10 files changed, 316 insertions(+), 69 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/8 -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] [catalog manager] more info if unable to replace a replica
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9040 ) Change subject: [catalog_manager] more info if unable to replace a replica .. Patch Set 3: Shouldn't we also periodically print this warning even if the cluster is healthy and there are no replicas to replace? -- To view, visit http://gerrit.cloudera.org:8080/9040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id5f562c6d1ff526daa785ea535e440598c03cd37 Gerrit-Change-Number: 9040 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Wed, 17 Jan 2018 02:47:41 + Gerrit-HasComments: No
[kudu-CR] [catalog manager] more info if unable to replace a replica
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9040 ) Change subject: [catalog_manager] more info if unable to replace a replica .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/9040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id5f562c6d1ff526daa785ea535e440598c03cd37 Gerrit-Change-Number: 9040 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Wed, 17 Jan 2018 02:45:59 + Gerrit-HasComments: No
[kudu-CR] [catalog manager] more info if unable to replace a replica
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9040 ) Change subject: [catalog_manager] more info if unable to replace a replica .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/9040/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/9040/2/src/kudu/master/catalog_manager.cc@3164 PS2, Line 3164: should be present > how about: are required Done http://gerrit.cloudera.org:8080/#/c/9040/2/src/kudu/master/catalog_manager.cc@3172 PS2, Line 3172: KLOG_EVERY_N(WARNING, 100) > how about: KLOG_EVERY_N_SECS(WARNING, 60) Done -- To view, visit http://gerrit.cloudera.org:8080/9040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id5f562c6d1ff526daa785ea535e440598c03cd37 Gerrit-Change-Number: 9040 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Wed, 17 Jan 2018 02:41:04 + Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] more info if unable to replace a replica
Hello Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9040 to look at the new patch set (#3). Change subject: [catalog_manager] more info if unable to replace a replica .. [catalog_manager] more info if unable to replace a replica Output actionable warning message when the catalog manager is unable to find a spot for a replacement replica. Since the 3-4-3 replication scheme is now enabled by default, this might be useful in case if running a cluster with just 3 tablet servers when tables have replication factor of 3. Change-Id: Id5f562c6d1ff526daa785ea535e440598c03cd37 --- M src/kudu/master/catalog_manager.cc M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h 3 files changed, 41 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/9040/3 -- To view, visit http://gerrit.cloudera.org:8080/9040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id5f562c6d1ff526daa785ea535e440598c03cd37 Gerrit-Change-Number: 9040 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/7821 ) Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions .. Patch Set 3: Code-Review+1 LGTM (deferring to other reviewers who might have some more feedback). -- To view, visit http://gerrit.cloudera.org:8080/7821 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7 Gerrit-Change-Number: 7821 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Jan 2018 02:32:06 + Gerrit-HasComments: No
[kudu-CR] KUDU-2148: do not crash on GetStatus during server startup
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/9041 )
Change subject: KUDU-2148: do not crash on GetStatus during server startup
..
Patch Set 2:
(5 comments)
lgtm, just a few nits
http://gerrit.cloudera.org:8080/#/c/9041/1/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/9041/1/src/kudu/server/server_base.cc@522
PS1, Line 522: rpc
nit: RPC
http://gerrit.cloudera.org:8080/#/c/9041/1/src/kudu/server/server_base.cc@525
PS1, Line 525: rpc
nit: RPC
http://gerrit.cloudera.org:8080/#/c/9041/2/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/9041/2/src/kudu/tserver/tablet_server-test.cc@211
PS2, Line 211: controller.Reset();
nit: maybe, just move RpcController under the scope of the 'while() {}' loop
below?
http://gerrit.cloudera.org:8080/#/c/9041/2/src/kudu/tserver/tablet_server-test.cc@215
PS2, Line 215: CHECK(resp.has_status());
nit: since there are many iterations of calling the GetStatus() method, maybe
it's cleaner to move the 'resp' under the scope of the 'while() {}' loop to
start with a clean state every iteration?
http://gerrit.cloudera.org:8080/#/c/9041/2/src/kudu/tserver/tablet_server-test.cc@230
PS2, Line 230: ASSERT_OK(mini_server_->Restart());
nit: consider adding some scope-related mechanics to join the thread if this
assert fails.
--
To view, visit http://gerrit.cloudera.org:8080/9041
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
Gerrit-Change-Number: 9041
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 17 Jan 2018 02:21:05 +
Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] more info if unable to replace a replica
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9040 ) Change subject: [catalog_manager] more info if unable to replace a replica .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/9040/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/9040/2/src/kudu/master/catalog_manager.cc@3164 PS2, Line 3164: should be present how about: are required http://gerrit.cloudera.org:8080/#/c/9040/2/src/kudu/master/catalog_manager.cc@3172 PS2, Line 3172: KLOG_EVERY_N(WARNING, 100) how about: KLOG_EVERY_N_SECS(WARNING, 60) -- To view, visit http://gerrit.cloudera.org:8080/9040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id5f562c6d1ff526daa785ea535e440598c03cd37 Gerrit-Change-Number: 9040 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Wed, 17 Jan 2018 01:58:59 + Gerrit-HasComments: Yes
[kudu-CR] consensus: Fix KUDU-1735 regression test in 3-4-3 mode
Mike Percy has abandoned this change. ( http://gerrit.cloudera.org:8080/9013 ) Change subject: consensus: Fix KUDU-1735 regression test in 3-4-3 mode .. Abandoned Superseded by https://gerrit.cloudera.org/c/8989/ -- To view, visit http://gerrit.cloudera.org:8080/9013 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I275ba76c7fc7711e05b9dbda02a659d4245c40d0 Gerrit-Change-Number: 9013 Gerrit-PatchSet: 1 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2148: do not crash on GetStatus during server startup
Hello Alexey Serbin, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9041
to look at the new patch set (#2).
Change subject: KUDU-2148: do not crash on GetStatus during server startup
..
KUDU-2148: do not crash on GetStatus during server startup
The fix is straight-forward but there are backwards compatibility
implications for old clients who trigger the race: because they're not aware
of the new 'error' field in the protobuf, they'll just see the incomplete
'status' field. To be more specific, 'node_instance' will be set,
'version_info' won't be, and 'bound_{rpc,http}_addresses' may or may not be
set depending on exactly when the race was hit.
An alternative would be to fail the RPC itself in lieu of adding the 'error'
field. That would ensure that old clients register a failure, though it
comes at the expense of not providing rich error information for new
clients, which is why I went with the above solution instead.
Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
---
M src/kudu/server/generic_service.cc
M src/kudu/server/rpc_server.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/server/server_base.proto
M src/kudu/tserver/tablet_server-test.cc
6 files changed, 82 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/9041/2
--
To view, visit http://gerrit.cloudera.org:8080/9041
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
Gerrit-Change-Number: 9041
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2148: do not crash on GetStatus during server startup
Hello Alexey Serbin, Dan Burkert,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/9041
to review the following change.
Change subject: KUDU-2148: do not crash on GetStatus during server startup
..
KUDU-2148: do not crash on GetStatus during server startup
The fix is straight-forward but there are backwards compatibility
implications for old clients who trigger the race: because they're not aware
of the new 'error' field in the protobuf, they'll just see the incomplete
'status' field. To be more specific, 'node_instance' will be set,
'version_info' won't be, and 'bound_{rpc,http}_addresses' may or may not be
set depending on exactly when the race was hit.
An alternative would be to fail the RPC itself in lieu of adding the 'error'
field. That would ensure that old clients register a failure, though it
comes at the expense of not providing rich error information for new
clients, which is why I went with the above solution instead.
Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
---
M src/kudu/server/generic_service.cc
M src/kudu/server/rpc_server.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/server/server_base.proto
M src/kudu/tserver/tablet_server-test.cc
6 files changed, 82 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/9041/1
--
To view, visit http://gerrit.cloudera.org:8080/9041
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
Gerrit-Change-Number: 9041
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
[kudu-CR] KUDU-2261: The order of the responses after flush should match the order we call apply
zhen.zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/9029 ) Change subject: KUDU-2261: The order of the responses after flush should match the order we call apply .. Patch Set 1: The test failed seems unrelated with this change. -- To view, visit http://gerrit.cloudera.org:8080/9029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib37c9e85ad03731bb7d5b83be77d40fcd95e803a Gerrit-Change-Number: 9029 Gerrit-PatchSet: 1 Gerrit-Owner: zhen.zhang Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: zhen.zhang Gerrit-Comment-Date: Wed, 17 Jan 2018 01:41:45 + Gerrit-HasComments: No
[kudu-CR] KUDU-1913: LIFO wake up ordering for threadpool worker threads
Dan Burkert has posted comments on this change. (
http://gerrit.cloudera.org:8080/9021 )
Change subject: KUDU-1913: LIFO wake up ordering for threadpool worker threads
..
Patch Set 1: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/threadpool-test.cc
File src/kudu/util/threadpool-test.cc:
http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/threadpool-test.cc@934
PS1, Line 934: [](){}
o_O
--
To view, visit http://gerrit.cloudera.org:8080/9021
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8036bf0d15f9ffcb3fa76579e3bfe0a340d38320
Gerrit-Change-Number: 9021
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Wed, 17 Jan 2018 01:28:47 +
Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] more info if unable to replace a replica
Hello Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9040 to look at the new patch set (#2). Change subject: [catalog_manager] more info if unable to replace a replica .. [catalog_manager] more info if unable to replace a replica Output actionable warning message when the catalog manager is unable to find a place for a replacement replica. Since the 3-4-3 replication scheme is now enabled by default, this might be useful in case if running a cluster with just 3 tablet servers when tables have replication factor of 3. Change-Id: Id5f562c6d1ff526daa785ea535e440598c03cd37 --- M src/kudu/master/catalog_manager.cc M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h 3 files changed, 41 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/9040/2 -- To view, visit http://gerrit.cloudera.org:8080/9040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id5f562c6d1ff526daa785ea535e440598c03cd37 Gerrit-Change-Number: 9040 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7821 ) Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions .. Patch Set 3: I've updated according to Alexey's suggestion. The check now happens at startup, which is a definite improvement. -- To view, visit http://gerrit.cloudera.org:8080/7821 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7 Gerrit-Change-Number: 7821 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Jan 2018 01:09:06 + Gerrit-HasComments: No
[kudu-CR] mini-cluster: rename data root to cluster root
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9033 ) Change subject: mini-cluster: rename data_root to cluster_root .. mini-cluster: rename data_root to cluster_root I found that the name data_root for the root directory in which to place the cluster's data and logs was a confusing name because: 1. we use "data" here loosely to describe any old bytes, encompassing not only data blocks, but WALs and logs as well, and 2. the concept of a "data root" already exists to mean a user-specified location to place data blocks (i.e. an entry in fs_data_dirs). As such, this patch renames External- and InternalMiniClusterOptions' "data_root" members to "cluster_root". Change-Id: Id8ccc9e232f47a684d6b9226fd84639c4c94d0c3 Reviewed-on: http://gerrit.cloudera.org:8080/9033 Reviewed-by: Adar Dembo Tested-by: Andrew Wong --- M src/kudu/benchmarks/tpch/tpch1.cc M src/kudu/benchmarks/tpch/tpch_real_world.cc M src/kudu/integration-tests/ts_itest-base.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_test.cc 10 files changed, 42 insertions(+), 42 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Andrew Wong: Verified -- To view, visit http://gerrit.cloudera.org:8080/9033 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Id8ccc9e232f47a684d6b9226fd84639c4c94d0c3 Gerrit-Change-Number: 9033 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Hello Alexey Serbin, Henry Robinson, Adar Dembo, Sailesh Mukil, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7821 to look at the new patch set (#3). Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions .. rpc: allow setting --rpc_tls_min_protocol on older RHEL versions Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7 --- M src/kudu/security/tls_context.cc 1 file changed, 36 insertions(+), 12 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/7821/3 -- To view, visit http://gerrit.cloudera.org:8080/7821 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7 Gerrit-Change-Number: 7821 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] heavy-update-compaction-itest
Dan Burkert has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9037 ) Change subject: heavy-update-compaction-itest .. heavy-update-compaction-itest This is an integration test which simulates an extremely update-heavy workload. It was written while trying to repro KUDU-2251, and subsequently led to discovering the issues in KUDU-2253. Change-Id: Ie2e30c63e1fedafe1eeb672346700ee8c5e2bb2c Reviewed-on: http://gerrit.cloudera.org:8080/9037 Reviewed-by: David Ribeiro Alves Tested-by: Kudu Jenkins --- M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/heavy-update-compaction-itest.cc 2 files changed, 245 insertions(+), 0 deletions(-) Approvals: David Ribeiro Alves: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/9037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ie2e30c63e1fedafe1eeb672346700ee8c5e2bb2c Gerrit-Change-Number: 9037 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [catalog manager] more info if unable to replace a replica
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9040 Change subject: [catalog_manager] more info if unable to replace a replica .. [catalog_manager] more info if unable to replace a replica Output actionable warning message when the catalog manager is unable to find a place for a replacement replica. Since the 3-4-3 replication scheme is now enabled by default, this might be useful in case if running a cluster with just 3 tablet servers when tables have replication factor of 3. Change-Id: Id5f562c6d1ff526daa785ea535e440598c03cd37 --- M src/kudu/master/catalog_manager.cc M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h 3 files changed, 36 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/9040/1 -- To view, visit http://gerrit.cloudera.org:8080/9040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Id5f562c6d1ff526daa785ea535e440598c03cd37 Gerrit-Change-Number: 9040 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
[kudu-CR] mini-cluster: rename data root to cluster root
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9033 ) Change subject: mini-cluster: rename data_root to cluster_root .. Patch Set 4: Verified+1 The failure was due to a clock sync issue. -- To view, visit http://gerrit.cloudera.org:8080/9033 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8ccc9e232f47a684d6b9226fd84639c4c94d0c3 Gerrit-Change-Number: 9033 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 16 Jan 2018 22:07:46 + Gerrit-HasComments: No
[kudu-CR] mini-cluster: rename data root to cluster root
Andrew Wong has removed a vote on this change. Change subject: mini-cluster: rename data_root to cluster_root .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/9033 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Id8ccc9e232f47a684d6b9226fd84639c4c94d0c3 Gerrit-Change-Number: 9033 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 ) Change subject: KUDU-1489: allow configuration of metadata dir .. Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/9027/7/src/kudu/fs/fs_manager-test.cc File src/kudu/fs/fs_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/9027/7/src/kudu/fs/fs_manager-test.cc@534 PS7, Line 534: // Adding a new data dir elsewhere in the list is OK. This isn't what the test does anymore though; it's just shuffling the order of the directories in data_roots. Is that worth testing? If so, reword the comment further. If not, remove this part of the test. http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@256 PS4, Line 256: const string meta_dir_in_data_root = JoinPathSegments(canonicalized_data_fs_roots_[0].path, > Is your concern that if the FS layout already partially exists, but not com My concern is with the following sequence: 1. I'm going to create an FS with wal_dir=/a, metadata_dir="", data_dirs=/b. 2. First I do "mkdir -p /b/tablet-meta". 3. Then I do FSManager.CreateInitialFileSystemLayout. 4. Then I do FSManager.Open. I think I'll wind up with metadata_dir=/b/tablet-meta instead of /a/tablet-meta, but maybe this is too contrived due to step #2. What would be interesting is if step #2 could be substituted with an otherwise valid Kudu operation. Anyway, my question was "will step #3 fail because of the directory created in step #2"? If so, this is a non-issue. -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 16 Jan 2018 21:23:46 + Gerrit-HasComments: Yes
[kudu-CR] mini-cluster: rename data root to cluster root
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9033 ) Change subject: mini-cluster: rename data_root to cluster_root .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/9033 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8ccc9e232f47a684d6b9226fd84639c4c94d0c3 Gerrit-Change-Number: 9033 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 16 Jan 2018 21:12:44 + Gerrit-HasComments: No
[kudu-CR] mini-cluster: rename data root to cluster root
Hello David Ribeiro Alves, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9033 to look at the new patch set (#4). Change subject: mini-cluster: rename data_root to cluster_root .. mini-cluster: rename data_root to cluster_root I found that the name data_root for the root directory in which to place the cluster's data and logs was a confusing name because: 1. we use "data" here loosely to describe any old bytes, encompassing not only data blocks, but WALs and logs as well, and 2. the concept of a "data root" already exists to mean a user-specified location to place data blocks (i.e. an entry in fs_data_dirs). As such, this patch renames External- and InternalMiniClusterOptions' "data_root" members to "cluster_root". Change-Id: Id8ccc9e232f47a684d6b9226fd84639c4c94d0c3 --- M src/kudu/benchmarks/tpch/tpch1.cc M src/kudu/benchmarks/tpch/tpch_real_world.cc M src/kudu/integration-tests/ts_itest-base.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_test.cc 10 files changed, 42 insertions(+), 42 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/9033/4 -- To view, visit http://gerrit.cloudera.org:8080/9033 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id8ccc9e232f47a684d6b9226fd84639c4c94d0c3 Gerrit-Change-Number: 9033 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] mini-cluster: rename data root to cluster root
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9033 ) Change subject: mini-cluster: rename data_root to cluster_root .. Patch Set 3: (1 comment) > Patch Set 3: > > (1 comment) http://gerrit.cloudera.org:8080/#/c/9033/3/src/kudu/mini-cluster/external_mini_cluster.h File src/kudu/mini-cluster/external_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/9033/3/src/kudu/mini-cluster/external_mini_cluster.h@90 PS3, Line 90: // Directory in which to store data. > Maybe update this? Done -- To view, visit http://gerrit.cloudera.org:8080/9033 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8ccc9e232f47a684d6b9226fd84639c4c94d0c3 Gerrit-Change-Number: 9033 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 16 Jan 2018 21:04:08 + Gerrit-HasComments: Yes
[kudu-CR] heavy-update-compaction-itest
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/9037 ) Change subject: heavy-update-compaction-itest .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/9037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie2e30c63e1fedafe1eeb672346700ee8c5e2bb2c Gerrit-Change-Number: 9037 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 16 Jan 2018 20:45:16 + Gerrit-HasComments: No
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir
..
Patch Set 6:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG@20
PS4, Line 20: It is up to the user to take caution in changing this flag;
updating the
: flag without also manually moving any existing metadata will
cause Kudu
: to fail at startup.
> Isn't the same true of fs_wal_dir and fs_data_dirs? Or is changing fs_metad
Good point. No point in bringing it up, I'll take this out.
http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG@29
PS4, Line 29: cases
> Nit: case
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/block_manager_util.cc
File src/kudu/fs/block_manager_util.cc:
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/block_manager_util.cc@176
PS4, Line 176: DCHECK_NE(first_healthy, -1); // Guaranteed by
DataDirManager::LoadInstnaces().
> Can we not depend on this, and instead return a bad status if there are no
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@258
PS4, Line 258: env_->DeleteRecursively(GetTestPath("wal"));
: env_->DeleteRecursively(GetTestPath("data"));
> Maybe use different tests so you needn't clean up in between?
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@271
PS4, Line 271: opts.metadata_root = "";
> Maybe opts.metadata_root.clear() would be more idiomatic?
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@284
PS4, Line 284: ASSERT_OK(fs_manager()->Open());
> After this could you check that the WAL root and the metadata root aren't t
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@288
PS4, Line 288: opts.metadata_root = "";
> .clear() here too.
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@517
PS4, Line 517: // Try to open with a new data dir at the front of the list.
> Nit: the other "Try to ..." comments here are all followed up by "this shou
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@524
PS4, Line 524: // But adding a new data dir elsewhere in the list is OK.
> And reword this too.
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.h@90
PS4, Line 90: or the first configured data root if it already contains
: // existing metadata.
> Would it be clearer if we additionally specified that this fallback behavio
Hm, I didn't explicitly call it out, but I reworded it to hopefully clarify
that the first data dir is only used if metadata exists there from a previous
deployment. Let me know what you think.
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@93
PS4, Line 93: DEFINE_string(fs_metadata_dir, "",
> There should be a mention here of the fallback behavior for existing system
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@94
PS4, Line 94: Must be equivalent to fs_wal_dir or a
> I thought this wasn't true anymore?
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@95
PS4, Line 95: fs_dataA_dirs
> fs_data_dirs
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@253
PS4, Line 253: // Check the first data root for metadata.
> Could you LOG here what we actually used for the metadata directory, a la L
Done
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@256
PS4, Line 256: // If there is no metadata in the first data root, use the
WAL root.
> Hmm, but we wouldn't want to do this fallback when creating a brand new fil
Is your concern that if the FS layout already partially exists, but not
completely (such that fs_manager()->Open() fails, but fs_manager()->Create()
succeeds), we may end up going through CreateInitialFileSystemLayout() with the
first data root storing metadata?
That could happen if they fail to start up a cluster with Kudu 1.6, upgrade to
Kudu 1.7, and then start up successfully. I think I'm ok with this though,
although a simple update would be to also check whether the metadata directory
is empty, in which case we could use the WAL dir. What do you think?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@265
PS4, Line 265: const auto& canonicalized_metadata_root =
canonicalized_metadata_fs_root_;
> Why do we need this local?
Done
http://gerrit.cloudera.org:8080/#/
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9027 to look at the new patch set (#7). Change subject: KUDU-1489: allow configuration of metadata dir .. KUDU-1489: allow configuration of metadata dir Metadata files currently reside in the first configured data directory, which isn't always the best choice; this first drive, if not performant, can act as a bottleneck. Often times the drive containing the WALs is a better choice, as it is recommended to be the fastest. This patch allows users to configure their metadata directory through the new fs_metadata_dir flag. If empty, Kudu will check if a metadata directory exists in the first member of fs_data_dirs, and if none exists, Kudu will use fs_wal_dir as the root directory for metadata. Otherwise, the flag will be honored verbatim. Aside from the update to the directory location, codepaths that previously assumed that the first data directory _must_ be healthy have been updated. The remaining invariant is that at least a single data directory must be healthy. New test cases are added to fs_manager-test, and a test case in data_dirs-test is updated as a sanity check to show that we can now open the directory manager with a failed first data directory (previously this codepath would hit D/CHECK failures). Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db --- M src/kudu/fs/block_manager_util.cc M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc 7 files changed, 173 insertions(+), 64 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/7 -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] heavy-update-compaction-itest
Dan Burkert has posted comments on this change. (
http://gerrit.cloudera.org:8080/9037 )
Change subject: heavy-update-compaction-itest
..
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9037/1/src/kudu/integration-tests/heavy-update-compaction-itest.cc
File src/kudu/integration-tests/heavy-update-compaction-itest.cc:
http://gerrit.cloudera.org:8080/#/c/9037/1/src/kudu/integration-tests/heavy-update-compaction-itest.cc@92
PS1, Line 92:
b.AddColumn("val_a")->Type(KuduColumnSchema::STRING)->NotNull();
> Maybe parameterize (or at least store in a class constant) the number of st
Done
http://gerrit.cloudera.org:8080/#/c/9037/1/src/kudu/integration-tests/heavy-update-compaction-itest.cc@168
PS1, Line 168: 100 rows.
> nit: might not be 100, right?
Done
http://gerrit.cloudera.org:8080/#/c/9037/1/src/kudu/integration-tests/heavy-update-compaction-itest.cc@217
PS1, Line 217: scanner.SetTimeoutMillis(120 * 1000);
> make this a flag so that we can change this if we change "rows" or "rounds"
Done
http://gerrit.cloudera.org:8080/#/c/9037/1/src/kudu/integration-tests/heavy-update-compaction-itest.cc@225
PS1, Line 225: const auto &
> Nit: const auto&
Done
http://gerrit.cloudera.org:8080/#/c/9037/1/src/kudu/integration-tests/heavy-update-compaction-itest.cc@230
PS1, Line 230: EXPECT_EQ(actual_val,
final_values[final_values_offset++]);
> Nit: just curious why you used EXPECT here and not ASSERT, given that the o
I don't consider this to be a fatal failure, since the test can keep going if
it fails without causing a logic or memory-safety error. The upside is that if
this case were to fail it may be easier to debug if you get the full set of
errors instead of short circuiting.
--
To view, visit http://gerrit.cloudera.org:8080/9037
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e30c63e1fedafe1eeb672346700ee8c5e2bb2c
Gerrit-Change-Number: 9037
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 16 Jan 2018 20:14:38 +
Gerrit-HasComments: Yes
[kudu-CR] heavy-update-compaction-itest
Hello David Ribeiro Alves, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9037 to look at the new patch set (#2). Change subject: heavy-update-compaction-itest .. heavy-update-compaction-itest This is an integration test which simulates an extremely update-heavy workload. It was written while trying to repro KUDU-2251, and subsequently led to discovering the issues in KUDU-2253. Change-Id: Ie2e30c63e1fedafe1eeb672346700ee8c5e2bb2c --- M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/heavy-update-compaction-itest.cc 2 files changed, 245 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/9037/2 -- To view, visit http://gerrit.cloudera.org:8080/9037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie2e30c63e1fedafe1eeb672346700ee8c5e2bb2c Gerrit-Change-Number: 9037 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1489: allow configuration of metadata dir
David Ribeiro Alves has posted comments on this change. (
http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir
..
Patch Set 6:
(7 comments)
first pass. mostly looked at the test.
it'd be nice to have a more integration-y test that tests at least the most
worrysome cases (downgrade/upgrade most likely scenarios)
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/block_manager_util.cc
File src/kudu/fs/block_manager_util.cc:
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/block_manager_util.cc@176
PS6, Line 176: LoadInstnaces
typo
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@248
PS6, Line 248: "wal"
could we be more specific? maybe look for full suffix?
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@255
PS6, Line 255: LOG(INFO) << s.ToString();
leftover from testing? if you want to print the status on failure you can do so
on the assert below
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@258
PS6, Line 258: env_->DeleteRecursively(GetTestPath("wal"));
: env_->DeleteRecursively(GetTestPath("data"));
check the status
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@263
PS6, Line 263: opts.metadata_root = GetTestPath("data");
: ReinitFsManagerWithOpts(opts);
: ASSERT_OK(fs_manager()->CreateInitialFileSystemLayout());
: ASSERT_OK(fs_manager()->Open());
: ASSERT_STR_CONTAINS(fs_manager()->GetTabletMetadataDir(),
"data");
in the case before this one you test with a metadata root path that does not
exist, before you delete the data/wal could you test that if the metadata root
is set to the old value (like in this case) it fails too?
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@269
PS6, Line 269: // Opening the FsManager with an empty fs_metadata_dir flag
should account
: // for the old default and use the first data directory for
metadata.
wait, empty string is different from "unset"? why?
http://gerrit.cloudera.org:8080/#/c/9027/6/src/kudu/fs/fs_manager-test.cc@276
PS6, Line 276: env_->DeleteRecursively(GetTestPath("wal"));
: env_->DeleteRecursively(GetTestPath("data"));
same
--
To view, visit http://gerrit.cloudera.org:8080/9027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db
Gerrit-Change-Number: 9027
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 16 Jan 2018 20:07:00 +
Gerrit-HasComments: Yes
[kudu-CR] mini-cluster: rename data root to cluster root
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/9033 ) Change subject: mini-cluster: rename data_root to cluster_root .. Patch Set 3: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/9033 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8ccc9e232f47a684d6b9226fd84639c4c94d0c3 Gerrit-Change-Number: 9033 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 16 Jan 2018 19:52:25 + Gerrit-HasComments: No
[kudu-CR] heavy-update-compaction-itest
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/9037 ) Change subject: heavy-update-compaction-itest .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/9037/1/src/kudu/integration-tests/heavy-update-compaction-itest.cc File src/kudu/integration-tests/heavy-update-compaction-itest.cc: http://gerrit.cloudera.org:8080/#/c/9037/1/src/kudu/integration-tests/heavy-update-compaction-itest.cc@168 PS1, Line 168: 100 rows. nit: might not be 100, right? http://gerrit.cloudera.org:8080/#/c/9037/1/src/kudu/integration-tests/heavy-update-compaction-itest.cc@217 PS1, Line 217: scanner.SetTimeoutMillis(120 * 1000); make this a flag so that we can change this if we change "rows" or "rounds" -- To view, visit http://gerrit.cloudera.org:8080/9037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie2e30c63e1fedafe1eeb672346700ee8c5e2bb2c Gerrit-Change-Number: 9037 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 16 Jan 2018 19:50:44 + Gerrit-HasComments: Yes
[kudu-CR] WIP [rpc] don't issue authn tokens over non-confidential connections
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9025 ) Change subject: WIP [rpc] don't issue authn tokens over non-confidential connections .. Patch Set 1: (1 comment) LGTM, I agree about the need for a test. http://gerrit.cloudera.org:8080/#/c/9025/1/src/kudu/rpc/negotiation.cc File src/kudu/rpc/negotiation.cc: http://gerrit.cloudera.org:8080/#/c/9025/1/src/kudu/rpc/negotiation.cc@224 PS1, Line 224: (conn->socket()->IsLoopbackConnection() && !FLAGS_rpc_encrypt_loopback_connections)); This LGTM. I originally expected it would just be client_negotiation.tls_negotiated() || conn->socket()->IsLoopbackConnection() But I think it makes sense to also gate on rpc-encrypt-loopback-connections in order to allow opting-out of this behavior, and I think it makes sense to do it with the same flag in both instances. -- To view, visit http://gerrit.cloudera.org:8080/9025 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie31aa492bcc460dbd43975bccfe571354f3bf885 Gerrit-Change-Number: 9025 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 16 Jan 2018 19:39:17 + Gerrit-HasComments: Yes
[kudu-CR] build: Move fake XML file generation to run-test.sh
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8984 ) Change subject: build: Move fake XML file generation to run-test.sh .. Patch Set 3: (1 comment) I can think of another reason why it may have been done in build-and-test.sh: during precommit, run-test.sh is run on individual distributed test slaves rather than the orchestrating Jenkins slave. Could you check that "generated" JUnit XML files make their way from dist-test slaves back to the orchestrator? http://gerrit.cloudera.org:8080/#/c/8984/3/build-support/run-test.sh File build-support/run-test.sh: http://gerrit.cloudera.org:8080/#/c/8984/3/build-support/run-test.sh@221 PS3, Line 221: if [ ! -f "$XMLFILE" ]; then Should this be conditioned on STATUS != 0? -- To view, visit http://gerrit.cloudera.org:8080/8984 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaa89c806039a96ac0a6b7262ded74f70f49f87ac Gerrit-Change-Number: 8984 Gerrit-PatchSet: 3 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Edward Fancher Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 16 Jan 2018 19:37:53 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#11). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/CMakeLists.txt M src/kudu/common/common.proto A src/kudu/common/decimal.cc A src/kudu/common/decimal.h M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 27 files changed, 868 insertions(+), 164 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/11 -- To view, visit http://gerrit.cloudera.org:8080/8830 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 11 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] heavy-update-compaction-itest
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/9037 )
Change subject: heavy-update-compaction-itest
..
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9037/1/src/kudu/integration-tests/heavy-update-compaction-itest.cc
File src/kudu/integration-tests/heavy-update-compaction-itest.cc:
http://gerrit.cloudera.org:8080/#/c/9037/1/src/kudu/integration-tests/heavy-update-compaction-itest.cc@92
PS1, Line 92:
b.AddColumn("val_a")->Type(KuduColumnSchema::STRING)->NotNull();
Maybe parameterize (or at least store in a class constant) the number of string
rows? Would also help avoid the "i <= 5" magic numbers below.
http://gerrit.cloudera.org:8080/#/c/9037/1/src/kudu/integration-tests/heavy-update-compaction-itest.cc@225
PS1, Line 225: const auto &
Nit: const auto&
http://gerrit.cloudera.org:8080/#/c/9037/1/src/kudu/integration-tests/heavy-update-compaction-itest.cc@230
PS1, Line 230: EXPECT_EQ(actual_val,
final_values[final_values_offset++]);
Nit: just curious why you used EXPECT here and not ASSERT, given that the other
checks in this test are all ASSERT?
--
To view, visit http://gerrit.cloudera.org:8080/9037
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e30c63e1fedafe1eeb672346700ee8c5e2bb2c
Gerrit-Change-Number: 9037
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 16 Jan 2018 19:34:37 +
Gerrit-HasComments: Yes
[kudu-CR] heavy-update-compaction-itest
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9037 ) Change subject: heavy-update-compaction-itest .. Patch Set 1: Note for reviewers: this test was included in revision 6 of https://gerrit.cloudera.org/#/c/8951/. -- To view, visit http://gerrit.cloudera.org:8080/9037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie2e30c63e1fedafe1eeb672346700ee8c5e2bb2c Gerrit-Change-Number: 9037 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 16 Jan 2018 19:33:32 + Gerrit-HasComments: No
[kudu-CR] KUDU-721: [Java] Add DECIMAL column type support
Hello Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8882 to look at the new patch set (#4). Change subject: KUDU-721: [Java] Add DECIMAL column type support .. KUDU-721: [Java] Add DECIMAL column type support This patch adds basic support to the Java client to create, read, and write tables with DECIMAL columns. Change-Id: I6240e3cfe0d6328b68c50099d442ffeeab6c9fd9 --- M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java A java/kudu-client/src/main/java/org/apache/kudu/ColumnTypeAttributes.java M java/kudu-client/src/main/java/org/apache/kudu/Schema.java M java/kudu-client/src/main/java/org/apache/kudu/Type.java M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java M java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java M java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.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/PartialRow.java M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java A java/kudu-client/src/main/java/org/apache/kudu/util/DecimalUtil.java M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestBytes.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestRowResult.java M src/kudu/integration-tests/decimal-itest.cc 19 files changed, 968 insertions(+), 44 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/8882/4 -- To view, visit http://gerrit.cloudera.org:8080/8882 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6240e3cfe0d6328b68c50099d442ffeeab6c9fd9 Gerrit-Change-Number: 8882 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2208 Add RETRY ON EINTR() to Subprocess
Hello Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9015 to look at the new patch set (#10). Change subject: KUDU-2208 Add RETRY_ON_EINTR() to Subprocess .. KUDU-2208 Add RETRY_ON_EINTR() to Subprocess This patch submits a unit test that create a Subprocess thread and while it is starting and waiting, another thread sends kill signals to the Subprocess thread. Since the pthread_kill() signal is sent asynchronously, sometimes the pthread_kill() raises error signal 3. In that condition, the unit test logs the incident as an INFO. Prior to this bug fix patch, the Subprocess throws an exception because it cannot handle the kill signal in Wait() state. To fix the bug, we add RETRY_ON_EINTR() inside Subprocess::DoWait() function. Furthermore, this patch also moves the RETRY_ON_EINTR() function that occurs in many places across the code to os-util.h and it adds alarm reset in the end of TestReadFromStdoutAndStderr in Subprocess-test. Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333 --- M src/kudu/consensus/log_index.cc M src/kudu/util/env_posix.cc M src/kudu/util/net/socket.cc M src/kudu/util/os-util.h M src/kudu/util/subprocess-test.cc M src/kudu/util/subprocess.cc 6 files changed, 74 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/9015/10 -- To view, visit http://gerrit.cloudera.org:8080/9015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I148b4619a8dda8e3e95dd6ea5c6e993a9e37a333 Gerrit-Change-Number: 9015 Gerrit-PatchSet: 10 Gerrit-Owner: Jeffrey F. Lukman Gerrit-Reviewer: Jeffrey F. Lukman Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] build: Move fake XML file generation to run-test.sh
Edward Fancher has posted comments on this change. ( http://gerrit.cloudera.org:8080/8984 ) Change subject: build: Move fake XML file generation to run-test.sh .. Patch Set 3: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/8984 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaa89c806039a96ac0a6b7262ded74f70f49f87ac Gerrit-Change-Number: 8984 Gerrit-PatchSet: 3 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Edward Fancher Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 16 Jan 2018 19:30:54 + Gerrit-HasComments: No
[kudu-CR] heavy-update-compaction-itest
Hello David Ribeiro Alves, Adar Dembo, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/9037 to review the following change. Change subject: heavy-update-compaction-itest .. heavy-update-compaction-itest This is an integration test which simulates an extremely update-heavy workload. It was written while trying to repro KUDU-2251, and subsequently led to discovering the issues in KUDU-2253. Change-Id: Ie2e30c63e1fedafe1eeb672346700ee8c5e2bb2c --- M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/heavy-update-compaction-itest.cc 2 files changed, 239 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/9037/1 -- To view, visit http://gerrit.cloudera.org:8080/9037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ie2e30c63e1fedafe1eeb672346700ee8c5e2bb2c Gerrit-Change-Number: 9037 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2202: support for removing data directories (take two)
Hello Kudu Jenkins, Andrew Wong, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8978 to look at the new patch set (#5). Change subject: KUDU-2202: support for removing data directories (take two) .. KUDU-2202: support for removing data directories (take two) This commit extends "kudu fs update_dirs" to allow the removal of data directories. A data dir may be removed provided there are no tablets configured to use it. The --force flag may be used to override this safety check; tablets that depend on the removed data dir will fail to bootstrap the next time the server is started. I'll be the first to admit that this functionality isn't terribly useful at the moment because Kudu deployments are configured to stripe data across all data directories, so removing a data dir will fail all tablets, which is equivalent to reformatting the node. Nevertheless, it will become more useful as users customize the new --fs_target_data_dirs_per_tablet gflag. This is the second attempt at implementing this functionality. The first used a different interface and was vulnerable to a data loss issue documented in KUDU-2202. Andrew fixed that in commit 47b81c4, which reopens the door for safely removing data directories. I considered a stronger check that only prohibits data dir removal if there were data blocks on the to-be-removed data dir, but decided against it: - It's rare to find a tablet configured to use a data dir but without any actual blocks on it. In fact, that's only likely for empty tables, or tables with empty range partitions. - We'd still need to rewrite any unaffected tablet superblocks and remove the data dir from their data dir groups. There are several interesting modifications here: 1. PathInstanceMetadataFile::CheckIntegrity can now be run without checking for missing instance files. This is used when removing a data dir (its instance file will be missing). 2. The DataDirManager and FsManager may now be constructed with both read_only=true and update_on_disk=true. The CLI tool leverages this functionality to create a "speculative" FsManager using the new data dir configuration that is used to check whether existing tablets depend on the data dir to be removed. 3. The DataDirManager now checks for integrity after making on-disk changes. While not strictly necessary, this seems like a good sanity check to avoid introducing bugs. 4. The "kudu fs update_dirs" action was updated for data dir removal. It now accepts the --force option, without which removal will only be allowed if no tablets are configured to use the removed data dir. Change-Id: Iacb650115bcdf055542ef2777641a3201fc8e30b --- M src/kudu/fs/block_manager_util-test.cc M src/kudu/fs/block_manager_util.cc M src/kudu/fs/block_manager_util.h M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_fs.cc 10 files changed, 344 insertions(+), 90 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/8978/5 -- To view, visit http://gerrit.cloudera.org:8080/8978 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iacb650115bcdf055542ef2777641a3201fc8e30b Gerrit-Change-Number: 8978 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/7821 ) Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/7821/2/src/kudu/security/tls_context.cc File src/kudu/security/tls_context.cc: http://gerrit.cloudera.org:8080/#/c/7821/2/src/kudu/security/tls_context.cc@52 PS2, Line 52: // --rpc-tls-min-protocol=TLSv1.2 option, negotiations will fail at runtime with : // a 'missing protocol' error: : / > I think it's possible to look at SSLv23_method()->version just after initia In other words, SSLv23_method()->version reports the highest version of the TLS/SSL protocol supported by the OpenSSL library. And it's straightforward to compare the reported version with the required protocol version: SSL2_VERSION0x0002 SSL3_VERSION0x0300 TLS1_VERSION0x0301 TLS1_1_VERSION 0x0302 TLS1_2_VERSION 0x0303 -- To view, visit http://gerrit.cloudera.org:8080/7821 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7 Gerrit-Change-Number: 7821 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 16 Jan 2018 19:19:05 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#10). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/CMakeLists.txt M src/kudu/common/common.proto A src/kudu/common/decimal.cc A src/kudu/common/decimal.h M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 27 files changed, 867 insertions(+), 164 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/10 -- To view, visit http://gerrit.cloudera.org:8080/8830 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 10 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2202: support for removing data directories (take two)
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8978 ) Change subject: KUDU-2202: support for removing data directories (take two) .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/8978/4/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: http://gerrit.cloudera.org:8080/#/c/8978/4/src/kudu/fs/data_dirs.cc@810 PS4, Line 810: list of data directories now : // includes the missing directory. > nit: If I'm understanding this correctly, the update should have removed th Ah yes, will update. -- To view, visit http://gerrit.cloudera.org:8080/8978 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacb650115bcdf055542ef2777641a3201fc8e30b Gerrit-Change-Number: 8978 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 16 Jan 2018 19:17:50 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8830 to look at the new patch set (#9). Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) .. KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client) Introduces the Decimal type to the server and C++ client. Follow on work will enhance the utility in the client and add support to the Java client and other integrations and add documentation. The decimal type has column type attributes to support the “parameterized type”. The precision and scale column type attributes are not stored with each value, but instead are leveraged to map to a correctly sized internal type (DECIMAL32, DECIMAL64, DECIMAL128). These internal types are represented and stored as equivalently sized integers. This also removes the int128 suffixes because they break the client by using C++11 features. They may be added back in a different way later. Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 --- M src/kudu/client/CMakeLists.txt M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/value-internal.h M src/kudu/client/value.cc M src/kudu/client/value.h M src/kudu/common/CMakeLists.txt M src/kudu/common/common.proto A src/kudu/common/decimal.cc A src/kudu/common/decimal.h M src/kudu/common/partial_row.cc M src/kudu/common/partial_row.h M src/kudu/common/schema-test.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/types.cc M src/kudu/common/types.h M src/kudu/common/wire_protocol.cc M src/kudu/consensus/log-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/all_types-itest.cc A src/kudu/integration-tests/decimal-itest.cc M src/kudu/util/int128-test.cc M src/kudu/util/int128.h 27 files changed, 867 insertions(+), 164 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/9 -- To view, visit http://gerrit.cloudera.org:8080/8830 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632 Gerrit-Change-Number: 8830 Gerrit-PatchSet: 9 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/8830 )
Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..
Patch Set 8:
(21 comments)
Still tweaking the decimal impl a bit and adding some tests, but pushing an
update for review and discussion.
http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG@17
PS8, Line 17: stored
> not stored? it must be stored in the metadata somewhere right?
Right, I should clarify. What I mean is they are not serialized with each
value. I will update this.
http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG@24
PS8, Line 24: int128 suffixes
> perhaps we should guard them with a preprocessor check for c++11 instead?
Yeah, I think we should. I want to do that in a separate patch since the base
functionality block progress on the Impala side. I can remove it in a separate
patch too if that helps.
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@63
PS8, Line 63: class KuduColumnTypeAttributes {
> should this be exported for users? If so I think we need to PIMPL it for AB
Yes, this should be exported. I will add KUDU_EXPORT. I modeled this to match
KuduColumnStorageAttributes which doesn't use PImpl. Do you know why?
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@177
PS8, Line 177: /// @todo KUDU-809: make this hard-to-use constructor private.
: /// Clients should use the Builder API. Currently only the
Python API
: /// uses this old API.
> this has been deprecated for several releases now and I dont think python u
Oh right, even defaulted arguments aren't compatible.
I can make remove (make it private) if thats the route we should take.
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@323
PS8, Line 323: floating-point
> it's not floating point, right?
This was taken right from the Impala docs.
I agree though, I think a more accurate term would be "fractional".
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@328
PS8, Line 328: /// The precision must be between 0 and 38.
> curious what a precision of 0 means... that can only store the value 0?
Should be between 1 and 0.
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@343
PS8, Line 343: /// columns precision.
> typo: column's
Done
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@133
PS8, Line 133: return kudu::DECIMAL128;
> nit: weird indentation
Done
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@153
PS8, Line 153: case kudu::DECIMAL32 : return KuduColumnSchema::DECIMAL;
> nit: extra space
Done
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@266
PS8, Line 266: return Status::InvalidArgument("no scale provided for
decimal column", data_->name);
> is a default scale of 0 not reasonable? I seem to recall that sql allows DE
This was discussed some on the design doc, but we never settled for sure. I
think a default scale of 0 is reasonable and fairly common. Happy to change to
use that default.
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@285
PS8, Line 285: }
> can we check that no precision/scale are specified for non-decimal column t
Absolutely. Will add that.
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@287
PS8, Line 287: int32_t precision = 0;
: if (data_->has_precision) {
: precision = data_->precision;
: }
> think ternary would be easier to read here and below
agreed.
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@297
PS8, Line 297: KuduColumnTypeAttributes kuduColumnTypeAttributes =
> nit: var name style
Done
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@641
PS8, Line 641: typeAttrs
> nit: style
Done
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h
File src/kudu/common/decimal.h:
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h@53
PS8, Line 53:static const int32_t MIN_DECIMAL_PRECISION = 0;
> see comment elsewhere about precision 0
Done
http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h@63
PS8, Line 63:explicit Decimal(int128_t s) :
> I sort of feel like this Decimal instance should retain its precision and s
Yeah, I commented that I was pretty conflicted about this too on one of Dan's
reviews. Had I created this class with no reference at all I would definitely
have gone that route.
The only thing that made me leave off precision and scale was the Impala
[kudu-CR] mini-cluster: rename data root to cluster root
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9033 ) Change subject: mini-cluster: rename data_root to cluster_root .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/9033/3/src/kudu/mini-cluster/external_mini_cluster.h File src/kudu/mini-cluster/external_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/9033/3/src/kudu/mini-cluster/external_mini_cluster.h@90 PS3, Line 90: // Directory in which to store data. Maybe update this? -- To view, visit http://gerrit.cloudera.org:8080/9033 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8ccc9e232f47a684d6b9226fd84639c4c94d0c3 Gerrit-Change-Number: 9033 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 16 Jan 2018 19:14:58 + Gerrit-HasComments: Yes
[kudu-CR](branch-1.5.x) KUDU-2193 (part 2): avoid holding TSTabletManager::lock for a long time
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8526 ) Change subject: KUDU-2193 (part 2): avoid holding TSTabletManager::lock_ for a long time .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.5.x Gerrit-MessageType: comment Gerrit-Change-Id: I0f645775e8347a18af112b308dba56a3b4a2c681 Gerrit-Change-Number: 8526 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 16 Jan 2018 19:10:29 + Gerrit-HasComments: No
[kudu-CR](branch-1.4.x) KUDU-2193 (part 2): avoid holding TSTabletManager::lock for a long time
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8528 ) Change subject: KUDU-2193 (part 2): avoid holding TSTabletManager::lock_ for a long time .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8528 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.4.x Gerrit-MessageType: comment Gerrit-Change-Id: I0f645775e8347a18af112b308dba56a3b4a2c681 Gerrit-Change-Number: 8528 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 16 Jan 2018 19:10:36 + Gerrit-HasComments: No
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/9027 )
Change subject: KUDU-1489: allow configuration of metadata dir
..
Patch Set 6:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG@20
PS4, Line 20: It is up to the user to take caution in changing this flag;
updating the
: flag without also manually moving any existing metadata will
cause Kudu
: to fail at startup.
Isn't the same true of fs_wal_dir and fs_data_dirs? Or is changing
fs_metadata_dir somehow more dangerous?
I guess what I'm asking is: why call this out explicitly if it works the same
way as those other flags?
http://gerrit.cloudera.org:8080/#/c/9027/4//COMMIT_MSG@29
PS4, Line 29: cases
Nit: case
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/block_manager_util.cc
File src/kudu/fs/block_manager_util.cc:
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/block_manager_util.cc@176
PS4, Line 176: DCHECK_NE(first_healthy, -1); // Guaranteed by
DataDirManager::LoadInstnaces().
Can we not depend on this, and instead return a bad status if there are no
healthy instances? Seems like a weird dependency since this is a "util"
function that ostensibly could be called from anywhere.
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@258
PS4, Line 258: env_->DeleteRecursively(GetTestPath("wal"));
: env_->DeleteRecursively(GetTestPath("data"));
Maybe use different tests so you needn't clean up in between?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@271
PS4, Line 271: opts.metadata_root = "";
Maybe opts.metadata_root.clear() would be more idiomatic?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@284
PS4, Line 284: ASSERT_OK(fs_manager()->Open());
After this could you check that the WAL root and the metadata root aren't the
same?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@288
PS4, Line 288: opts.metadata_root = "";
.clear() here too.
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@517
PS4, Line 517: // Try to open with a new data dir at the front of the list.
Nit: the other "Try to ..." comments here are all followed up by "this should
fail", but this no longer fails. Reword?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager-test.cc@524
PS4, Line 524: // But adding a new data dir elsewhere in the list is OK.
And reword this too.
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.h@90
PS4, Line 90: or the first configured data root if it already contains
: // existing metadata.
Would it be clearer if we additionally specified that this fallback behavior
only takes effect when opening an existing filesystem? That is, when creating a
new filesystem, it's just "verbatim, or WAL dir if empty"?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@93
PS4, Line 93: DEFINE_string(fs_metadata_dir, "",
There should be a mention here of the fallback behavior for existing systems.
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@94
PS4, Line 94: Must be equivalent to fs_wal_dir or a
I thought this wasn't true anymore?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@95
PS4, Line 95: fs_dataA_dirs
fs_data_dirs
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@253
PS4, Line 253: // Check the first data root for metadata.
Could you LOG here what we actually used for the metadata directory, a la
L243-244?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@256
PS4, Line 256: // If there is no metadata in the first data root, use the
WAL root.
Hmm, but we wouldn't want to do this fallback when creating a brand new
filesystem. Does it actually matter? Is it possible for CreateFileSystemLayout
to succeed if meta_dir_in_data_root already exists?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@265
PS4, Line 265: const auto& canonicalized_metadata_root =
canonicalized_metadata_fs_root_;
Why do we need this local?
http://gerrit.cloudera.org:8080/#/c/9027/4/src/kudu/fs/fs_manager.cc@421
PS4, Line 421: if
(dd_manager_->FindUuidIndexByRoot(canonicalized_metadata_fs_root_.path,
&metadata_idx) &&
I'm confused; why would the DataDirManager be aware of the metadata root, since
it's no longer guaranteed to be colocated with the data directory?
Oh, this is just for the
[kudu-CR] mini-cluster: rename data root to cluster root
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9033 ) Change subject: mini-cluster: rename data_root to cluster_root .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/9033/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9033/1//COMMIT_MSG@17 PS1, Line 17: As such, this patch renames External- and InternalMiniClusterOptions' > Could you make the same change to the InternalMiniCluster too? Done -- To view, visit http://gerrit.cloudera.org:8080/9033 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8ccc9e232f47a684d6b9226fd84639c4c94d0c3 Gerrit-Change-Number: 9033 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 16 Jan 2018 18:52:02 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 ) Change subject: KUDU-1489: allow configuration of metadata dir .. Patch Set 5: Sorry, pushed accidentally in conjunction with another patch. Reverting to patch set 4. -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 16 Jan 2018 18:51:48 + Gerrit-HasComments: No
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9027 to look at the new patch set (#6). Change subject: KUDU-1489: allow configuration of metadata dir .. KUDU-1489: allow configuration of metadata dir Metadata files currently reside in the first configured data directory, which isn't always the best choice; this first drive, if not performant, can act as a bottleneck. Often times the drive containing the WALs is a better choice, as it is recommended to be the fastest. This patch allows users to configure their metadata directory through the new fs_metadata_dir flag. If empty, Kudu will check if a metadata directory exists in the first member of fs_data_dirs, and if none exists, Kudu will use fs_wal_dir as the root directory for metadata. Otherwise, the flag will be honored verbatim. It is up to the user to take caution in changing this flag; updating the flag without also manually moving any existing metadata will cause Kudu to fail at startup. Aside from the update to the directory location, codepaths that previously assumed that the first data directory _must_ be healthy have been updated. The remaining invariant is that at least a single data directory must be healthy. A new test cases is added to fs_manager-test, and a test case in data_dirs-test is updated as a sanity check to show that we can now open the directory manager with a failed first data directory (previously this codepath would hit D/CHECK failures). Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db --- M src/kudu/fs/block_manager_util.cc M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc 7 files changed, 160 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/6 -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini-cluster: rename data root to cluster root
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9033 to look at the new patch set (#2). Change subject: mini-cluster: rename data_root to cluster_root .. mini-cluster: rename data_root to cluster_root I found that the name data_root for the root directory in which to place the cluster's data and logs was a confusing name because: 1. we use "data" here loosely to describe any old bytes, encompassing not only data blocks, but WALs and logs as well, and 2. the concept of a "data root" already exists to mean a user-specified location to place data blocks (i.e. an entry in fs_data_dirs). As such, this patch renames External- and InternalMiniClusterOptions' "data_root" members to "cluster_root". Change-Id: Id8ccc9e232f47a684d6b9226fd84639c4c94d0c3 --- M src/kudu/benchmarks/tpch/tpch1.cc M src/kudu/benchmarks/tpch/tpch_real_world.cc M src/kudu/integration-tests/ts_itest-base.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_test.cc 10 files changed, 41 insertions(+), 41 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/9033/2 -- To view, visit http://gerrit.cloudera.org:8080/9033 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id8ccc9e232f47a684d6b9226fd84639c4c94d0c3 Gerrit-Change-Number: 9033 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] fs: move metadata to the WAL directory
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9027 to look at the new patch set (#5). Change subject: fs: move metadata to the WAL directory .. fs: move metadata to the WAL directory Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db --- M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h 2 files changed, 8 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/5 -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9027 ) Change subject: KUDU-1489: allow configuration of metadata dir .. Patch Set 4: I think we should merge KUDU-2202 before merging this (will need to update a couple of things), but feel free to review this in it's current form. -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 16 Jan 2018 18:48:34 + Gerrit-HasComments: No
[kudu-CR] tools: Add debug mode to pb dump tool
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/9024 )
Change subject: tools: Add debug mode to pb dump tool
..
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9024/1//COMMIT_MSG
Commit Message:
PS1:
Could you add a short test to kudu-tool-test? A run with --debug and a check
for the presence of additional output should suffice.
http://gerrit.cloudera.org:8080/#/c/9024/1//COMMIT_MSG@13
PS1, Line 13: TODO: Should this be called "verbose" mode instead of debug mode?
I don't think it really matters, since the additional information is just for
debugging.
http://gerrit.cloudera.org:8080/#/c/9024/1/src/kudu/tools/tool_action_pbc.cc
File src/kudu/tools/tool_action_pbc.cc:
http://gerrit.cloudera.org:8080/#/c/9024/1/src/kudu/tools/tool_action_pbc.cc@92
PS1, Line 92: }
Nit: could you add an else that does LOG(FATAL) or something like that?
http://gerrit.cloudera.org:8080/#/c/9024/1/src/kudu/tools/tool_action_pbc.cc@232
PS1, Line 232: .AddOptionalParameter("oneline")
Should add debug here.
http://gerrit.cloudera.org:8080/#/c/9024/1/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:
http://gerrit.cloudera.org:8080/#/c/9024/1/src/kudu/util/pb_util.cc@937
PS1, Line 937: "---"
Nit: if this is now being used in three places, perhaps make it a constant so
there's no worry about the number of dashes being different in each of the
three places?
--
To view, visit http://gerrit.cloudera.org:8080/9024
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica2a74a2e252d140cf7704ff68037a64db19cd80
Gerrit-Change-Number: 9024
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 16 Jan 2018 18:41:35 +
Gerrit-HasComments: Yes
[kudu-CR] mini-cluster: rename data root to cluster root
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9033 ) Change subject: mini-cluster: rename data_root to cluster_root .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/9033/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9033/1//COMMIT_MSG@17 PS1, Line 17: As such, this patch renames ExternalMiniClusterOptions's data_root to Could you make the same change to the InternalMiniCluster too? -- To view, visit http://gerrit.cloudera.org:8080/9033 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8ccc9e232f47a684d6b9226fd84639c4c94d0c3 Gerrit-Change-Number: 9033 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 16 Jan 2018 18:34:30 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1489: allow configuration of metadata dir
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9027 to look at the new patch set (#4). Change subject: KUDU-1489: allow configuration of metadata dir .. KUDU-1489: allow configuration of metadata dir Metadata files currently reside in the first configured data directory, which isn't always the best choice; this first drive, if not performant, can act as a bottleneck. Often times the drive containing the WALs is a better choice, as it is recommended to be the fastest. This patch allows users to configure their metadata directory through the new fs_metadata_dir flag. If empty, Kudu will check if a metadata directory exists in the first member of fs_data_dirs, and if none exists, Kudu will use fs_wal_dir as the root directory for metadata. Otherwise, the flag will be honored verbatim. It is up to the user to take caution in changing this flag; updating the flag without also manually moving any existing metadata will cause Kudu to fail at startup. Aside from the update to the directory location, codepaths that previously assumed that the first data directory _must_ be healthy have been updated. The remaining invariant is that at least a single data directory must be healthy. A new test cases is added to fs_manager-test, and a test case in data_dirs-test is updated as a sanity check to show that we can now open the directory manager with a failed first data directory (previously this codepath would hit D/CHECK failures). Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db --- M src/kudu/fs/block_manager_util.cc M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc 7 files changed, 160 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9027/4 -- To view, visit http://gerrit.cloudera.org:8080/9027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I375c6b2eb283db5fa9c956135d98252c8781f5db Gerrit-Change-Number: 9027 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini-cluster: rename data root to cluster root
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9033 Change subject: mini-cluster: rename data_root to cluster_root .. mini-cluster: rename data_root to cluster_root I found that the name data_root for the root directory in which to place the cluster's data and logs was a confusing name because: 1. we use "data" here loosely to describe any old bytes, encompassing not only data blocks, but WALs and logs as well, and 2. the concept of a "data root" already exists to mean a user-specified location to place data blocks (i.e. an entry in fs_data_dirs). As such, this patch renames ExternalMiniClusterOptions's data_root to cluster_root. Change-Id: Id8ccc9e232f47a684d6b9226fd84639c4c94d0c3 --- M src/kudu/benchmarks/tpch/tpch_real_world.cc M src/kudu/integration-tests/ts_itest-base.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_test.cc 7 files changed, 30 insertions(+), 30 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/9033/1 -- To view, visit http://gerrit.cloudera.org:8080/9033 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Id8ccc9e232f47a684d6b9226fd84639c4c94d0c3 Gerrit-Change-Number: 9033 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] KUDU-2261: The order of the responses after flush should match the order we call apply
zhen.zhang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9029 Change subject: KUDU-2261: The order of the responses after flush should match the order we call apply .. KUDU-2261: The order of the responses after flush should match the order we call apply The response list of flush() should have the same order of we apply operations, so it's easier to know which operation failed and which succeeded. Change-Id: Ib37c9e85ad03731bb7d5b83be77d40fcd95e803a --- 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/test/java/org/apache/kudu/client/TestKuduSession.java 4 files changed, 68 insertions(+), 15 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/9029/1 -- To view, visit http://gerrit.cloudera.org:8080/9029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ib37c9e85ad03731bb7d5b83be77d40fcd95e803a Gerrit-Change-Number: 9029 Gerrit-PatchSet: 1 Gerrit-Owner: zhen.zhang
