[kudu-CR] KUDU-3371 [fs] Use RocksDB to store LBM metadata
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18569 ) Change subject: KUDU-3371 [fs] Use RocksDB to store LBM metadata .. Patch Set 71: (74 comments) http://gerrit.cloudera.org:8080/#/c/18569/71//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18569/71//COMMIT_MSG@12 PS71, Line 12: long time bootstrap consumption nit: long bootstrap times http://gerrit.cloudera.org:8080/#/c/18569/71//COMMIT_MSG@24 PS71, Line 24: different nit: difference http://gerrit.cloudera.org:8080/#/c/18569/71//COMMIT_MSG@25 PS71, Line 25: is nit: is that http://gerrit.cloudera.org:8080/#/c/18569/71//COMMIT_MSG@27 PS71, Line 27: the nit: The http://gerrit.cloudera.org:8080/#/c/18569/71//COMMIT_MSG@26 PS71, Line 26: a : native nit: in a native http://gerrit.cloudera.org:8080/#/c/18569/71//COMMIT_MSG@27 PS71, Line 27: , nit: end the sentence here, it's long enough already http://gerrit.cloudera.org:8080/#/c/18569/71//COMMIT_MSG@28 PS71, Line 28: clase class http://gerrit.cloudera.org:8080/#/c/18569/71//COMMIT_MSG@29 PS71, Line 29: container nit: a container http://gerrit.cloudera.org:8080/#/c/18569/71//COMMIT_MSG@35 PS71, Line 35: container nit: a container http://gerrit.cloudera.org:8080/#/c/18569/71//COMMIT_MSG@38 PS71, Line 38: load containers when bootstrap nit: loading containers during the bootstrap phase http://gerrit.cloudera.org:8080/#/c/18569/71//COMMIT_MSG@39 PS71, Line 39: container nit: a container http://gerrit.cloudera.org:8080/#/c/18569/71//COMMIT_MSG@43 PS71, Line 43: container nit: a container http://gerrit.cloudera.org:8080/#/c/18569/71//COMMIT_MSG@56 PS71, Line 56: use nit: uses http://gerrit.cloudera.org:8080/#/c/18569/71//COMMIT_MSG@57 PS71, Line 57: , it is specified by flag : '--block_manager'. The new block manager is enabled by setting --block_manager=logr http://gerrit.cloudera.org:8080/#/c/18569/71//COMMIT_MSG@66 PS71, Line 66: it : shows that reopen staged reduced upto 90% time cost. nit: it shows that the time spent to re-open tablet server's metadata when 99.99% of all the records removed reduced about 9.5 times when using LogBlockContainerRdbMeta instead of LogBlockContainerNativeMeta. http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/client/CMakeLists.txt File src/kudu/client/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/client/CMakeLists.txt@284 PS71, Line 284: rocksdb This looks a bit odd: how does it happen that Kudu C++ client tests need rocksdb as well? Is rocksdb library really needed here? http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/CMakeLists.txt File src/kudu/fs/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/CMakeLists.txt@44 PS71, Line 44: kudu_test_util This looks wrong: a non-test libkudu_fs library is now dependent on libkudu_test_util test-only library. http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/block_manager-test.cc@1205 PS71, Line 1205: larger nit: greater http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/block_manager-test.cc@1243 PS71, Line 1243: to into http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/block_manager-test.cc@1243 PS71, Line 1243: is not take effect on does not affect http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/block_manager-test.cc@1243 PS71, Line 1243: so the metadata updating of "logr" : // block_manager works well updating the metadata stored in the logr-based block manager succeeds without any errors http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/dir_manager.h File src/kudu/fs/dir_manager.h: http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/dir_manager.h@121 PS71, Line 121: perpare preparatory http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/dir_manager.h@202 PS71, Line 202: public style nit: add a space before the 'public' label, similar to the other visibility labels in this file http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/dir_manager.h@220 PS71, Line 220: private ditto for the 'private' label http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/dir_manager.cc File src/kudu/fs/dir_manager.cc: http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/dir_manager.cc@207 PS71, Line 207: shared_ptr RdbDir::s_block_cache_; Why do you need it here? Isn't it enough to have it initialized using std::once() in the code below? http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/dir_manager.cc@229 PS71, Line 229: The A http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/dir_manager.cc@229 PS71, Line 229: will be is http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/dir_manager.cc@229 PS71, Line 229: is not does not
[kudu-CR] [net] DiagnosticSocket wrapper for sock diag API
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/20892 ) Change subject: [net] DiagnosticSocket wrapper for sock_diag API .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/20892 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4a4f37ca4b0df8ca543ec383d89766cbf1b1e526 Gerrit-Change-Number: 20892 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Thu, 18 Jan 2024 02:21:01 + Gerrit-HasComments: No
[kudu-CR](branch-1.17.x) KUDU-3491 Destruct master before creating a new one
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20917 Change subject: KUDU-3491 Destruct master before creating a new one .. KUDU-3491 Destruct master before creating a new one ServerBase constructor runs MinidumpExceptionHandler constructor that calls RegisterMinidumpExceptionHandler(). This function increments the static atomic variable current_num_instances_. Then the ServerBase is destructed, a similar process happens and current_num_instances_ gets decremented. If current_num_instances_ is not zero before incrementing or not 1 before decrementing, then it is considered an error. This indicates that only one Server can run at any given time. But in case of multi-master config, the master server is replaced, and without the change it is possible that the second server's constructor precede first server's destructor. This change makes it sure that the destructor is executed before the second one's constructor. Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Reviewed-on: http://gerrit.cloudera.org:8080/20913 Reviewed-by: Attila Bukor Reviewed-by: Mahesh Reddy Tested-by: Kudu Jenkins (cherry picked from commit 7562277fc6f68b0dcab593d56de03bb344a95b3e) --- M src/kudu/master/dynamic_multi_master-test.cc M src/kudu/master/master_runner.cc 2 files changed, 43 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/20917/1 -- To view, visit http://gerrit.cloudera.org:8080/20917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.17.x Gerrit-MessageType: newchange Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Gerrit-Change-Number: 20917 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Ádám Bakai
[kudu-CR] [net] DiagnosticSocket wrapper for sock diag API
Alexey Serbin has removed a vote on this change. Change subject: [net] DiagnosticSocket wrapper for sock_diag API .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/20892 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I4a4f37ca4b0df8ca543ec383d89766cbf1b1e526 Gerrit-Change-Number: 20892 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai
[kudu-CR] [net] DiagnosticSocket wrapper for sock diag API
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20892 ) Change subject: [net] DiagnosticSocket wrapper for sock_diag API .. Patch Set 3: Verified+1 unrelated CI pipeline failure (TSAN): Could not submit C++ distributed test job -- To view, visit http://gerrit.cloudera.org:8080/20892 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4a4f37ca4b0df8ca543ec383d89766cbf1b1e526 Gerrit-Change-Number: 20892 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Thu, 18 Jan 2024 00:06:04 + Gerrit-HasComments: No
[kudu-CR] KUDU-3491 Destruct master before creating a new one
Attila Bukor has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20913 ) Change subject: KUDU-3491 Destruct master before creating a new one .. KUDU-3491 Destruct master before creating a new one ServerBase constructor runs MinidumpExceptionHandler constructor that calls RegisterMinidumpExceptionHandler(). This function increments the static atomic variable current_num_instances_. Then the ServerBase is destructed, a similar process happens and current_num_instances_ gets decremented. If current_num_instances_ is not zero before incrementing or not 1 before decrementing, then it is considered an error. This indicates that only one Server can run at any given time. But in case of multi-master config, the master server is replaced, and without the change it is possible that the second server's constructor precede first server's destructor. This change makes it sure that the destructor is executed before the second one's constructor. Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Reviewed-on: http://gerrit.cloudera.org:8080/20913 Reviewed-by: Attila Bukor Reviewed-by: Mahesh Reddy Tested-by: Kudu Jenkins --- M src/kudu/master/dynamic_multi_master-test.cc M src/kudu/master/master_runner.cc 2 files changed, 43 insertions(+), 2 deletions(-) Approvals: Attila Bukor: Looks good to me, approved Mahesh Reddy: Looks good to me, but someone else must approve Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/20913 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Gerrit-Change-Number: 20913 Gerrit-PatchSet: 8 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Ádám Bakai
[kudu-CR] KUDU-3491 Destruct master before creating a new one
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20913 ) Change subject: KUDU-3491 Destruct master before creating a new one .. Patch Set 7: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/20913 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Gerrit-Change-Number: 20913 Gerrit-PatchSet: 7 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Wed, 17 Jan 2024 19:14:41 + Gerrit-HasComments: No
[kudu-CR] KUDU-3491 Destruct master before creating a new one
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/20913 ) Change subject: KUDU-3491 Destruct master before creating a new one .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/20913 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Gerrit-Change-Number: 20913 Gerrit-PatchSet: 7 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Wed, 17 Jan 2024 19:01:58 + Gerrit-HasComments: No
[kudu-CR] KUDU-3491 Destruct master before creating a new one
Hello Mahesh Reddy, Tidy Bot, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20913 to look at the new patch set (#7). Change subject: KUDU-3491 Destruct master before creating a new one .. KUDU-3491 Destruct master before creating a new one ServerBase constructor runs MinidumpExceptionHandler constructor that calls RegisterMinidumpExceptionHandler(). This function increments the static atomic variable current_num_instances_. Then the ServerBase is destructed, a similar process happens and current_num_instances_ gets decremented. If current_num_instances_ is not zero before incrementing or not 1 before decrementing, then it is considered an error. This indicates that only one Server can run at any given time. But in case of multi-master config, the master server is replaced, and without the change it is possible that the second server's constructor precede first server's destructor. This change makes it sure that the destructor is executed before the second one's constructor. Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 --- M src/kudu/master/dynamic_multi_master-test.cc M src/kudu/master/master_runner.cc 2 files changed, 43 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/20913/7 -- To view, visit http://gerrit.cloudera.org:8080/20913 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Gerrit-Change-Number: 20913 Gerrit-PatchSet: 7 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Ádám Bakai
[kudu-CR] KUDU-3491 Destruct master before creating a new one
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20913 ) Change subject: KUDU-3491 Destruct master before creating a new one .. Patch Set 6: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/20913 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Gerrit-Change-Number: 20913 Gerrit-PatchSet: 6 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Wed, 17 Jan 2024 18:23:56 + Gerrit-HasComments: No
[kudu-CR] KUDU-3491 Destruct master before creating a new one
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/20913 ) Change subject: KUDU-3491 Destruct master before creating a new one .. Patch Set 6: Code-Review+2 Thanks -- To view, visit http://gerrit.cloudera.org:8080/20913 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Gerrit-Change-Number: 20913 Gerrit-PatchSet: 6 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Wed, 17 Jan 2024 18:10:19 + Gerrit-HasComments: No
[kudu-CR] KUDU-3491 Destruct master before creating a new one
Hello Mahesh Reddy, Tidy Bot, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20913 to look at the new patch set (#6). Change subject: KUDU-3491 Destruct master before creating a new one .. KUDU-3491 Destruct master before creating a new one ServerBase constructor runs MinidumpExceptionHandler constructor that calls RegisterMinidumpExceptionHandler(). This function increments the static atomic variable current_num_instances_. Then the ServerBase is destructed, a similar process happens and current_num_instances_ gets decremented. If current_num_instances_ is not zero before incrementing or not 1 before decrementing, then it is considered an error. This indicates that only one Server can run at any given time. But in case of multi-master config, the master server is replaced, and without the change it is possible that the second server's constructor precede first server's destructor. This change makes it sure that the destructor is executed before the second one's constructor. Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 --- M src/kudu/master/dynamic_multi_master-test.cc M src/kudu/master/master_runner.cc 2 files changed, 40 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/20913/6 -- To view, visit http://gerrit.cloudera.org:8080/20913 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Gerrit-Change-Number: 20913 Gerrit-PatchSet: 6 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Ádám Bakai
[kudu-CR] KUDU-3491 Destruct master before creating a new one
Hello Mahesh Reddy, Tidy Bot, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20913 to look at the new patch set (#5). Change subject: KUDU-3491 Destruct master before creating a new one .. KUDU-3491 Destruct master before creating a new one ServerBase constructor runs MinidumpExceptionHandler constructor that calls RegisterMinidumpExceptionHandler(). This function increments the static atomic variable current_num_instances_. Then the ServerBase is destructed, a similar process happens and current_num_instances_ gets decremented. If current_num_instances_ is not zero before incrementing or not 1 before decrementing, then it is considered an error. This indicates that only one Server can run at any given time. But in case of multi-master config, the master server is replaced, and without the change it is possible that the second server's constructor precede first server's destructor. This change makes it sure that the destructor is executed before the second one's constructor. Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 --- M src/kudu/master/dynamic_multi_master-test.cc M src/kudu/master/master_runner.cc 2 files changed, 40 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/20913/5 -- To view, visit http://gerrit.cloudera.org:8080/20913 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Gerrit-Change-Number: 20913 Gerrit-PatchSet: 5 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Ádám Bakai
[kudu-CR] KUDU-3491 Destruct master before creating a new one
Ádám Bakai has posted comments on this change. (
http://gerrit.cloudera.org:8080/20913 )
Change subject: KUDU-3491 Destruct master before creating a new one
..
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20913/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20913/4//COMMIT_MSG@20
PS4, Line 20: This patch fixes KUDU-3491 issue.
> nit: no need for this line now as it's in the subject line
Done
http://gerrit.cloudera.org:8080/#/c/20913/3/src/kudu/master/dynamic_multi_master-test.cc
File src/kudu/master/dynamic_multi_master-test.cc:
http://gerrit.cloudera.org:8080/#/c/20913/3/src/kudu/master/dynamic_multi_master-test.cc@1483
PS3, Line 1483:
opts_.extra_master_flags.emplace_back("--enable_minidumps=true");
> Can you add a comment that minidumps are disabled for mini cluster by defau
Done
--
To view, visit http://gerrit.cloudera.org:8080/20913
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3
Gerrit-Change-Number: 20913
Gerrit-PatchSet: 4
Gerrit-Owner: Ádám Bakai
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Ádám Bakai
Gerrit-Comment-Date: Wed, 17 Jan 2024 18:08:44 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3491 Destruct master before creating a new one
Attila Bukor has posted comments on this change. (
http://gerrit.cloudera.org:8080/20913 )
Change subject: KUDU-3491 Destruct master before creating a new one
..
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20913/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20913/4//COMMIT_MSG@20
PS4, Line 20: This patch fixes KUDU-3491 issue.
nit: no need for this line now as it's in the subject line
http://gerrit.cloudera.org:8080/#/c/20913/3/src/kudu/master/dynamic_multi_master-test.cc
File src/kudu/master/dynamic_multi_master-test.cc:
http://gerrit.cloudera.org:8080/#/c/20913/3/src/kudu/master/dynamic_multi_master-test.cc@1483
PS3, Line 1483:
opts_.extra_master_flags.emplace_back("--enable_minidumps=true");
Can you add a comment that minidumps are disabled for mini cluster by default
which is the reason this wasn't caught earlier?
--
To view, visit http://gerrit.cloudera.org:8080/20913
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3
Gerrit-Change-Number: 20913
Gerrit-PatchSet: 4
Gerrit-Owner: Ádám Bakai
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Ádám Bakai
Gerrit-Comment-Date: Wed, 17 Jan 2024 17:42:00 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3491 Destruct master before creating a new one
Hello Mahesh Reddy, Tidy Bot, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20913 to look at the new patch set (#4). Change subject: KUDU-3491 Destruct master before creating a new one .. KUDU-3491 Destruct master before creating a new one ServerBase constructor runs MinidumpExceptionHandler constructor that calls RegisterMinidumpExceptionHandler(). This function increments the static atomic variable current_num_instances_. Then the ServerBase is destructed, a similar process happens and current_num_instances_ gets decremented. If current_num_instances_ is not zero before incrementing or not 1 before decrementing, then it is considered an error. This indicates that only one Server can run at any given time. But in case of multi-master config, the master server is replaced, and without the change it is possible that the second server's constructor precede first server's destructor. This change makes it sure that the destructor is executed before the second one's constructor. This patch fixes KUDU-3491 issue. Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 --- M src/kudu/master/dynamic_multi_master-test.cc M src/kudu/master/master_runner.cc 2 files changed, 37 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/20913/4 -- To view, visit http://gerrit.cloudera.org:8080/20913 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Gerrit-Change-Number: 20913 Gerrit-PatchSet: 4 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Ádám Bakai
[kudu-CR] KUDU-3491 Destruct master before creating a new one
Ádám Bakai has posted comments on this change. ( http://gerrit.cloudera.org:8080/20913 ) Change subject: KUDU-3491 Destruct master before creating a new one .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/20913/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20913/2//COMMIT_MSG@7 PS2, Line 7: [master_runner] > nit: replace this tag with the ticket ID, and rephrase it to something like Done http://gerrit.cloudera.org:8080/#/c/20913/2/src/kudu/master/master_runner.cc File src/kudu/master/master_runner.cc: PS2: > +1 Done PS2: > Can you add a test that would fail with the old behavior and pass with the Done -- To view, visit http://gerrit.cloudera.org:8080/20913 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Gerrit-Change-Number: 20913 Gerrit-PatchSet: 2 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Wed, 17 Jan 2024 17:28:53 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3491 Destruct master before creating a new one
Hello Mahesh Reddy, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20913 to look at the new patch set (#3). Change subject: KUDU-3491 Destruct master before creating a new one .. KUDU-3491 Destruct master before creating a new one ServerBase constructor runs MinidumpExceptionHandler constructor that calls RegisterMinidumpExceptionHandler(). This function increments the static atomic variable current_num_instances_. Then the ServerBase is destructed, a similar process happens and current_num_instances_ gets decremented. If current_num_instances_ is not zero before incrementing or not 1 before decrementing, then it is considered an error. This indicates that only one Server can run at any given time. But in case of multi-master config, the master server is replaced, and without the change it is possible that the second server's constructor precede first server's destructor. This change makes it sure that the destructor is executed before the second one's constructor. This patch fixes KUDU-3491 issue. Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 --- M src/kudu/master/dynamic_multi_master-test.cc M src/kudu/master/master_runner.cc 2 files changed, 39 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/20913/3 -- To view, visit http://gerrit.cloudera.org:8080/20913 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Gerrit-Change-Number: 20913 Gerrit-PatchSet: 3 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy
[kudu-CR] [master runner] Destruct server before new one
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20913 ) Change subject: [master_runner] Destruct server before new one .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/20913/2/src/kudu/master/master_runner.cc File src/kudu/master/master_runner.cc: PS2: > Can you add a test that would fail with the old behavior and pass with the +1 -- To view, visit http://gerrit.cloudera.org:8080/20913 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Gerrit-Change-Number: 20913 Gerrit-PatchSet: 2 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Wed, 17 Jan 2024 17:02:33 + Gerrit-HasComments: Yes
[kudu-CR] [compaction] Add tests to generate high memory rowset compaction
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20816 to look at the new patch set (#9). Change subject: [compaction] Add tests to generate high memory rowset compaction .. [compaction] Add tests to generate high memory rowset compaction This change implements a better approach to test out high memory compaction case. There is an existing patch: https://gerrit.cloudera.org/#/c/19278/ that uses different approach which does not scale well because of excessive memory usage by client. Patch contains two tests: 1. TestRowSetCompactionProceedWithNoBudgetingConstraints (generates deltas with memory requirements under budget) 2. TestRowSetCompactionSkipWithBudgetingConstraints (generates deltas with memory requirements over budget) and a helper function: GenHighMemConsumptionDeltas Using this helper function, callers can generate deltas of different sizes as per test needs. The size_factor can be used to achieve that. Increasing size_factor by 1 adds 1 MB worth of deltas as far as rowset compaction memory consumption is concerned. Change-Id: I1996558e71c49314c6acf12faf854c796548318c --- M src/kudu/tablet/compaction-test.cc 1 file changed, 141 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/20816/9 -- To view, visit http://gerrit.cloudera.org:8080/20816 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1996558e71c49314c6acf12faf854c796548318c Gerrit-Change-Number: 20816 Gerrit-PatchSet: 9 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [master runner] Destruct server before new one
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/20913 ) Change subject: [master_runner] Destruct server before new one .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/20913/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20913/2//COMMIT_MSG@7 PS2, Line 7: [master_runner] nit: replace this tag with the ticket ID, and rephrase it to something like "Destruct master before creating a new one" http://gerrit.cloudera.org:8080/#/c/20913/2/src/kudu/master/master_runner.cc File src/kudu/master/master_runner.cc: PS2: Can you add a test that would fail with the old behavior and pass with the new one? -- To view, visit http://gerrit.cloudera.org:8080/20913 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Gerrit-Change-Number: 20913 Gerrit-PatchSet: 2 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 17 Jan 2024 15:17:21 + Gerrit-HasComments: Yes
[kudu-CR] [master runner] Destruct server before new one
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20913 to look at the new patch set (#2). Change subject: [master_runner] Destruct server before new one .. [master_runner] Destruct server before new one ServerBase constructor runs MinidumpExceptionHandler constructor that calls RegisterMinidumpExceptionHandler(). This function increments the static atomic variable current_num_instances_. Then the ServerBase is destructed, a similar process happens and current_num_instances_ gets decremented. If current_num_instances_ is not zero before incrementing or not 1 before decrementing, then it is considered an error. This indicates that only one Server can run at any given time. But in case of multi-master config, the master server is replaced, and without the change it is possible that the second server's constructor precede first server's destructor. This change makes it sure that the destructor is executed before the second one's constructor. This patch fixes KUDU-3491 issue. Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 --- M src/kudu/master/master_runner.cc 1 file changed, 1 insertion(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/20913/2 -- To view, visit http://gerrit.cloudera.org:8080/20913 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Gerrit-Change-Number: 20913 Gerrit-PatchSet: 2 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [master runner] Destruct server before construct new one
Ádám Bakai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20913 Change subject: [master_runner] Destruct server before construct new one .. [master_runner] Destruct server before construct new one ServerBase constructor runs MinidumpExceptionHandler constructor that calls RegisterMinidumpExceptionHandler(). This function increments the static atomic variable current_num_instances_. Then the ServerBase is destructed, a similar process happens and current_num_instances_ gets decremented. If current_num_instances_ is not zero before incrementing or not 1 before decrementing, then it is considered an error. This indicates that only one Server can run at any given time. But in case of multi-master config, the master server is replaced, and without the change it is possible that the second server's constructor precede first server's destructor. This change makes it sure that the destructor is executed before the second one's constructor. This patch fixes KUDU-3491 issue. Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 --- M src/kudu/master/master_runner.cc 1 file changed, 1 insertion(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/20913/1 -- To view, visit http://gerrit.cloudera.org:8080/20913 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Gerrit-Change-Number: 20913 Gerrit-PatchSet: 1 Gerrit-Owner: Ádám Bakai
[kudu-CR] [compaction/test] Add tests to generate heavy rowset compaction
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20816 to look at the new patch set (#8). Change subject: [compaction/test] Add tests to generate heavy rowset compaction .. [compaction/test] Add tests to generate heavy rowset compaction This change implements a better approach to test out high memory compaction case. There is an existing patch: https://gerrit.cloudera.org/#/c/19278/ that uses different approach which does not scale well because of excessive memory usage by client. Patch contains two tests: 1. TestRowSetCompactionProceedWithNoBudgetingConstraints (generates deltas with memory requirements under budget) 2. TestRowSetCompactionSkipWithBudgetingConstraints (generates deltas with memory requirements over budget) and a helper function: GenHighMemConsumptionDeltas Using this helper function, callers can generate deltas of different sizes as per test needs. The size_factor can be used to achieve that. Increasing size_factor by 1 adds 1 MB worth of deltas as far as rowset compaction memory consumption is concerned. Change-Id: I1996558e71c49314c6acf12faf854c796548318c --- M src/kudu/tablet/compaction-test.cc 1 file changed, 141 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/20816/8 -- To view, visit http://gerrit.cloudera.org:8080/20816 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1996558e71c49314c6acf12faf854c796548318c Gerrit-Change-Number: 20816 Gerrit-PatchSet: 8 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [compaction/test] Add tests to generate heavy rowset compaction
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20816 ) Change subject: [compaction/test] Add tests to generate heavy rowset compaction .. Patch Set 7: (16 comments) http://gerrit.cloudera.org:8080/#/c/20816/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20816/6//COMMIT_MSG@23 PS6, Line 23: this h > nit: this helper function Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc File src/kudu/tablet/compaction-test.cc: http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@142 PS6, Line 142: > Why not to make this method returning Status instead? That would also help Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@153 PS6, Line 153: (type = > here and below: why not to use ASSERT_OK here, similar to the lines above? Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@163 PS6, Line 163: > shouldn't this be int64_t too? also in UpdateOriginalRowsNoFlush Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@164 PS6, Line 164: > Shouldn't this be wrapped into NO_FATALS() to exit as soon as any assertion Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@176 PS6, Line 176: UpdateOriginalRowsNoF > ditto: either wrap into NO_FATALS() or into ASSERT_OK() if changing the sig Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@605 PS6, Line 605: (Mutation** > What's 'heavy sized'? Does it simply mean 'large' or there is some other m Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@606 PS6, Line 606: AndDelete(Row > generates 1 MB deltas ? Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@607 PS6, Line 607: > nit: drop this part Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@608 PS6, Line 608: // Workload to generate large sized deltas for compaction. > The commit message says increasing the size_factor by 1 increases the delta Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@610 PS6, Line 610: > It would be great to document the 'delta_mem_factor' parameter as well. Not applicable anymore. Removed this argument. Callers will take care of setting the flags now. http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@632 PS6, Line 632: > Do you think this test should run for ASAN/TSAN builds as well, or it shoul Theoretically, it may not make much sense for these tests to run for TSAN builds because there is no race condition as such to be found. But it would not hurt to see how compaction does under large workloads and potential memory overhead introduced by TSAN. ASAN may sound more desired in this case as against TSAN because running into memory errors are of higher probability here. I am of the opinion of keeping it enabled for both though. It may involve some memory overhead and increase in execution time. This is already a slow test so that should not be a problem. http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@637 PS6, Line 637: // to make sure that when rowset compaction is invoked, it takes > Here and in another test below: to avoid flakiness due to the amount of ava The problem with setting FLAGS_memory_limit_hard_bytes in the test is that it remains set for the lifetime of the test binary in global variable g_hard_limit. If single set would do the trick for these two tests, it would have be re-visited if there is an additional test added here, in future, that works on hard limit. If I want to change the limit to different value in this test or a different one under this binary, I will have to explicitly set the global variable, which is not a clean approach. If there is a workaround for all this, we still need to take into account the current consumption of the process while setting hard limit which seems to be somewhat an overhead for a simple test. It makes more sense to just keep it under legitimate and acceptable limit (i.e. 20 MB) for this case where we want compaction to proceed. If a mere 20 MB memory is not available on a test node, maybe adjusting this value isn't our big problem. Anyway, I will decrease it further to 2 MB. For second test below, the memory requirement is set for ~2 TB which is quite high for an average node. http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@648 PS6, Line 648: his test a > nit: start/stop at different scope levels looks a bit odd; maybe move sw.st Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@663 PS6, Line 663: // Create a memrowset with 10 rows and several updates. > These two scenarios looks almost
[kudu-CR] [compaction/test] Add tests to generate heavy rowset compaction
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20816 to look at the new patch set (#7). Change subject: [compaction/test] Add tests to generate heavy rowset compaction .. [compaction/test] Add tests to generate heavy rowset compaction This change implements a better approach to test out high memory compaction case. There is an existing patch: https://gerrit.cloudera.org/#/c/19278/ that uses different approach which does not scale well because of excessive memory usage by client. Patch contains two tests: 1. TestRowSetCompactionProceedWithNoBudgetingConstraints (generates deltas with memory requirements under budget) 2. TestRowSetCompactionSkipWithBudgetingConstraints (generates deltas with memory requirements over budget) and a helper function: GenHighMemConsumptionDeltas Using this helper function, callers can generate deltas of different sizes as per test needs. The size_factor can be used to achieve that. Increasing size_factor by 1 adds 1 MB worth of deltas as far as rowset compaction memory consumption is concerned. Change-Id: I1996558e71c49314c6acf12faf854c796548318c --- M src/kudu/tablet/compaction-test.cc 1 file changed, 142 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/20816/7 -- To view, visit http://gerrit.cloudera.org:8080/20816 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1996558e71c49314c6acf12faf854c796548318c Gerrit-Change-Number: 20816 Gerrit-PatchSet: 7 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
