[kudu-CR] [log block manager-test] Improve random selection
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/20899 )
Change subject: [log_block_manager-test] Improve random selection
..
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20899/12/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/20899/12/src/kudu/fs/log_block_manager-test.cc@1299
PS12, Line 1299: // Deletes 'kNumBlocks * (1 - kLiveBlockRatio)' blocks.
> No, it wouldn't work. In the test kLiveBlockRatio = 0.1. If I erase everyth
Indeed -- if we are about to call AddDeletedBlock() for 90% of all the blocks,
then we need to remove 10% of elements of the 'ids' container, so the iterator
range for the erase() should be { begin() + 0.9 * size(), end() }, right.
The strange thing is that with this patch
LogBlockManagerTest.TestContainerBlockLimitingByMetadataSizeWithCompaction with
encryption enabled now fails every run, so I suspected something has changed in
the logic of this test.
Did you look at the root case of those failures?
--
To view, visit http://gerrit.cloudera.org:8080/20899
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce
Gerrit-Change-Number: 20899
Gerrit-PatchSet: 13
Gerrit-Owner: Ádám Bakai
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy
Gerrit-Reviewer: Marton Greber
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wang Xixu <1450306...@qq.com>
Gerrit-Reviewer: Zoltan Martonka
Gerrit-Reviewer: Ádám Bakai
Gerrit-Comment-Date: Fri, 23 Feb 2024 18:20:44 +
Gerrit-HasComments: Yes
[kudu-CR] [log block manager-test] Improve random selection
Hello Mahesh Reddy, Tidy Bot, Marton Greber, Zoltan Martonka, Alexey Serbin, Kudu Jenkins, Wang Xixu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20899 to look at the new patch set (#13). Change subject: [log_block_manager-test] Improve random selection .. [log_block_manager-test] Improve random selection The previous solution to remove random blocks was not good, because it was possible that way less blocks are removed than expected because it is possible that rand()%100 returns 0 or 99 every time and that means in extreme cases zero or all the elements are deleted. The new approach removes exactly the same number of blocks every time but randomizes which blocks are removed and the order of removal is randomized, too. Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce --- M src/kudu/fs/log_block_manager-test.cc 1 file changed, 4 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/20899/13 -- To view, visit http://gerrit.cloudera.org:8080/20899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce Gerrit-Change-Number: 20899 Gerrit-PatchSet: 13 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka Gerrit-Reviewer: Ádám Bakai
[kudu-CR] [log block manager-test] Improve random selection
Ádám Bakai has posted comments on this change. ( http://gerrit.cloudera.org:8080/20899 ) Change subject: [log_block_manager-test] Improve random selection .. Patch Set 12: (4 comments) http://gerrit.cloudera.org:8080/#/c/20899/12/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/20899/12/src/kudu/fs/log_block_manager-test.cc@1296 PS12, Line 1296: std::default_random_engine > Is it a requirement to use std::default_random_engine which is implementati Done http://gerrit.cloudera.org:8080/#/c/20899/12/src/kudu/fs/log_block_manager-test.cc@1297 PS12, Line 1297: rng > nit: could use the instance of the 'rng' in-place since it's not needed els Done http://gerrit.cloudera.org:8080/#/c/20899/12/src/kudu/fs/log_block_manager-test.cc@1298 PS12, Line 1298: 1 > style nit: 1.0 Done http://gerrit.cloudera.org:8080/#/c/20899/12/src/kudu/fs/log_block_manager-test.cc@1299 PS12, Line 1299: > If you want to call AddDeletedBlock for the (1.0 - kLiveBlockRatio) of all No, it wouldn't work. In the test kLiveBlockRatio = 0.1. If I erase everything from the kLiveBlockRatio * kNumBlocks position till the end, it ends up only 10% in the ids. And delete is called only on those elements. This was the solution in a previous patch set: ids.erase(ids.begin(), ids.begin() + static_cast(kLiveBlockRatio * kNumBlocks)); It does the same thing as the current solution, but Zoltan Martonka suggested that this type of delete is more efficient. I'm ok with either solution to delete the elements. -- To view, visit http://gerrit.cloudera.org:8080/20899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce Gerrit-Change-Number: 20899 Gerrit-PatchSet: 12 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Wed, 21 Feb 2024 13:38:52 + Gerrit-HasComments: Yes
[kudu-CR] [log block manager-test] Improve random selection
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20899 ) Change subject: [log_block_manager-test] Improve random selection .. Patch Set 12: (4 comments) http://gerrit.cloudera.org:8080/#/c/20899/12/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/20899/12/src/kudu/fs/log_block_manager-test.cc@1296 PS12, Line 1296: std::default_random_engine Is it a requirement to use std::default_random_engine which is implementation-defined and can be different on different platforms? A few test scenarios in this file use std::mt19937 engine already, is it possible to use it here as well? http://gerrit.cloudera.org:8080/#/c/20899/12/src/kudu/fs/log_block_manager-test.cc@1297 PS12, Line 1297: rng nit: could use the instance of the 'rng' in-place since it's not needed elsewhere in the code http://gerrit.cloudera.org:8080/#/c/20899/12/src/kudu/fs/log_block_manager-test.cc@1298 PS12, Line 1298: 1 style nit: 1.0 http://gerrit.cloudera.org:8080/#/c/20899/12/src/kudu/fs/log_block_manager-test.cc@1299 PS12, Line 1299: If you want to call AddDeletedBlock for the (1.0 - kLiveBlockRatio) of all the elements in the resulted array and keep just kLiveBlockRatio part, shouldn't that be instead ids.erase(ids.begin() + kLiveBlockRatio * kNumBlocks, ids.end()) ? -- To view, visit http://gerrit.cloudera.org:8080/20899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce Gerrit-Change-Number: 20899 Gerrit-PatchSet: 12 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Wed, 14 Feb 2024 20:08:37 + Gerrit-HasComments: Yes
[kudu-CR] [log block manager-test] Improve random selection
Hello Mahesh Reddy, Tidy Bot, Marton Greber, Zoltan Martonka, Kudu Jenkins, Wang Xixu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20899 to look at the new patch set (#11). Change subject: [log_block_manager-test] Improve random selection .. [log_block_manager-test] Improve random selection The previous solution to remove random blocks was not good, because it was possible that way less blocks are removed than expected because it is possible that rand()%100 returns 0 or 99 every time and that means in extreme cases zero or all the elements are deleted. The new approach removes exactly the same number of blocks every time but randomizes which blocks are removed and the order of removal is randomized, too. Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce --- M src/kudu/fs/log_block_manager-test.cc 1 file changed, 5 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/20899/11 -- To view, visit http://gerrit.cloudera.org:8080/20899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce Gerrit-Change-Number: 20899 Gerrit-PatchSet: 11 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka Gerrit-Reviewer: Ádám Bakai
[kudu-CR] [log block manager-test] Improve random selection
Hello Mahesh Reddy, Tidy Bot, Marton Greber, Zoltan Martonka, Kudu Jenkins, Wang Xixu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20899 to look at the new patch set (#10). Change subject: [log_block_manager-test] Improve random selection .. [log_block_manager-test] Improve random selection The previous solution to remove random blocks was not good, because it was possible that way less blocks are removed than expected because it is possible that rand()%100 returns 0 or 99 every time and that means in extreme cases zero or all the elements are deleted. The new approach removes exactly the same number of blocks every time but randomizes which blocks are removed and the order of removal is randomized, too. Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce --- M src/kudu/fs/log_block_manager-test.cc 1 file changed, 5 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/20899/10 -- To view, visit http://gerrit.cloudera.org:8080/20899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce Gerrit-Change-Number: 20899 Gerrit-PatchSet: 10 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka Gerrit-Reviewer: Ádám Bakai
[kudu-CR] [log block manager-test] Improve random selection
Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/20899 ) Change subject: [log_block_manager-test] Improve random selection .. Patch Set 9: Code-Review+1 LGTM, lets get a green build and I'll +2 it. -- To view, visit http://gerrit.cloudera.org:8080/20899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce Gerrit-Change-Number: 20899 Gerrit-PatchSet: 9 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Tue, 06 Feb 2024 16:33:46 + Gerrit-HasComments: No
[kudu-CR] [log block manager-test] Improve random selection
Hello Mahesh Reddy, Tidy Bot, Zoltan Martonka, Kudu Jenkins, Wang Xixu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20899 to look at the new patch set (#9). Change subject: [log_block_manager-test] Improve random selection .. [log_block_manager-test] Improve random selection The previous solution to remove random blocks was not good, because it was possible that way less blocks are removed than expected because it is possible that rand()%100 returns 0 or 99 every time and that means in extreme cases zero or all the elements are deleted. The new approach removes exactly the same number of blocks every time but randomizes which blocks are removed and the order of removal is randomized, too. Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce --- M src/kudu/fs/log_block_manager-test.cc 1 file changed, 5 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/20899/9 -- To view, visit http://gerrit.cloudera.org:8080/20899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce Gerrit-Change-Number: 20899 Gerrit-PatchSet: 9 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka Gerrit-Reviewer: Ádám Bakai
[kudu-CR] [log block manager-test] Improve random selection
Hello Mahesh Reddy, Tidy Bot, Zoltan Martonka, Kudu Jenkins, Wang Xixu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20899 to look at the new patch set (#8). Change subject: [log_block_manager-test] Improve random selection .. [log_block_manager-test] Improve random selection The previous solution to remove random blocks was not good, because it was possible that way less blocks are removed than expected because it is possible that rand()%100 returns 0 or 99 every time and that means in extreme cases zero or all the elements are deleted. The new approach removes exactly the same number of blocks every time but randomizes which blocks are removed and the order of removal is randomized, too. Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce --- M src/kudu/fs/log_block_manager-test.cc 1 file changed, 5 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/20899/8 -- To view, visit http://gerrit.cloudera.org:8080/20899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce Gerrit-Change-Number: 20899 Gerrit-PatchSet: 8 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka Gerrit-Reviewer: Ádám Bakai
[kudu-CR] [log block manager-test] Improve random selection
Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/20899 ) Change subject: [log_block_manager-test] Improve random selection .. Patch Set 7: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/20899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce Gerrit-Change-Number: 20899 Gerrit-PatchSet: 7 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Wed, 24 Jan 2024 02:03:00 + Gerrit-HasComments: No
[kudu-CR] [log block manager-test] Improve random selection
Hello Mahesh Reddy, Tidy Bot, Zoltan Martonka, Kudu Jenkins, Wang Xixu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20899 to look at the new patch set (#7). Change subject: [log_block_manager-test] Improve random selection .. [log_block_manager-test] Improve random selection The previous solution to remove random blocks was not good, because it was possible that way less blocks are removed than expected because it is possible that rand()%100 returns 0 or 99 every time and that means in extreme cases zero or all the elements are deleted. The new approach removes exactly the same number of blocks every time but randomizes which blocks are removed and the order of removal is randomized, too. Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce --- M src/kudu/fs/log_block_manager-test.cc 1 file changed, 5 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/20899/7 -- To view, visit http://gerrit.cloudera.org:8080/20899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce Gerrit-Change-Number: 20899 Gerrit-PatchSet: 7 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka Gerrit-Reviewer: Ádám Bakai
[kudu-CR] [log block manager-test] Improve random selection
Hello Mahesh Reddy, Tidy Bot, Zoltan Martonka, Kudu Jenkins, Wang Xixu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20899 to look at the new patch set (#6). Change subject: [log_block_manager-test] Improve random selection .. [log_block_manager-test] Improve random selection The previous solution to remove random blocks was not good, because it was possible that way less blocks are removed than expected because it is possible that rand()%100 returns 0 or 99 every time and that means in extreme cases zero or all the elements are deleted. The new approach removes exactly the same number of blocks every time but randomizes which blocks are removed and the order of removal is randomized, too. Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce --- M src/kudu/fs/log_block_manager-test.cc 1 file changed, 5 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/20899/6 -- To view, visit http://gerrit.cloudera.org:8080/20899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce Gerrit-Change-Number: 20899 Gerrit-PatchSet: 6 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka Gerrit-Reviewer: Ádám Bakai
[kudu-CR] [log block manager-test] Improve random selection
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20899 ) Change subject: [log_block_manager-test] Improve random selection .. Patch Set 5: > Patch Set 5: > > (1 comment) Besides this error, everything else lgtm. -- To view, visit http://gerrit.cloudera.org:8080/20899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce Gerrit-Change-Number: 20899 Gerrit-PatchSet: 5 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Tue, 16 Jan 2024 15:46:02 + Gerrit-HasComments: No
[kudu-CR] [log block manager-test] Improve random selection
Hello Tidy Bot, Zoltan Martonka, Kudu Jenkins, Wang Xixu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20899 to look at the new patch set (#5). Change subject: [log_block_manager-test] Improve random selection .. [log_block_manager-test] Improve random selection The previous solution to remove random blocks was not good, because it was possible that way less blocks are removed than expected because it is possible that rand()%100 returns 0 or 99 every time and that means in extreme cases zero or all the elements are deleted. The new approach removes exactly the same number of blocks every time but randomizes which blocks are removed and the order of removal is randomized, too. Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce --- M src/kudu/fs/log_block_manager-test.cc 1 file changed, 5 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/20899/5 -- To view, visit http://gerrit.cloudera.org:8080/20899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce Gerrit-Change-Number: 20899 Gerrit-PatchSet: 5 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka Gerrit-Reviewer: Ádám Bakai
[kudu-CR] [log block manager-test] Improve random selection
Ádám Bakai has posted comments on this change. (
http://gerrit.cloudera.org:8080/20899 )
Change subject: [log_block_manager-test] Improve random selection
..
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/20899/4/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/20899/4/src/kudu/fs/log_block_manager-test.cc@1296
PS4, Line 1296: auto rng = std::default_random_engine{};
> Did you consider seting a random seed?
Done
http://gerrit.cloudera.org:8080/#/c/20899/4/src/kudu/fs/log_block_manager-test.cc@1298
PS4, Line 1298: ids.erase(ids.begin(), ids.begin() +
static_cast(kLiveBlockRatio * kNumBlocks));
> ids is a local variable in this 17 lines lambda. The current way seems kind
I'm not sure either if it makes easier to understand.
http://gerrit.cloudera.org:8080/#/c/20899/4/src/kudu/fs/log_block_manager-test.cc@1298
PS4, Line 1298: ids.erase(ids.begin(), ids.begin() +
static_cast(kLiveBlockRatio * kNumBlocks));
> Is it better to use another variable to store the to-delete ids, for exampl
Done
--
To view, visit http://gerrit.cloudera.org:8080/20899
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce
Gerrit-Change-Number: 20899
Gerrit-PatchSet: 4
Gerrit-Owner: Ádám Bakai
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wang Xixu <1450306...@qq.com>
Gerrit-Reviewer: Zoltan Martonka
Gerrit-Reviewer: Ádám Bakai
Gerrit-Comment-Date: Tue, 16 Jan 2024 11:42:48 +
Gerrit-HasComments: Yes
[kudu-CR] [log block manager-test] Improve random selection
Zoltan Martonka has posted comments on this change. (
http://gerrit.cloudera.org:8080/20899 )
Change subject: [log_block_manager-test] Improve random selection
..
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20899/4/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/20899/4/src/kudu/fs/log_block_manager-test.cc@1296
PS4, Line 1296: auto rng = std::default_random_engine{};
Did you consider seting a random seed?
Seed seems to be randomised in some of the other tests in this file
(SeedRandom()).
I don't see any reason not to. (It was probably just forgotten).
Also you could delete (begin + (1-ratio) * count, end) to get an absolutely
superfluous perf improvement :P
http://gerrit.cloudera.org:8080/#/c/20899/4/src/kudu/fs/log_block_manager-test.cc@1298
PS4, Line 1298: ids.erase(ids.begin(), ids.begin() +
static_cast(kLiveBlockRatio * kNumBlocks));
> Is it better to use another variable to store the to-delete ids, for exampl
ids is a local variable in this 17 lines lambda. The current way seems kind of
safe for me (and better performance).
--
To view, visit http://gerrit.cloudera.org:8080/20899
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce
Gerrit-Change-Number: 20899
Gerrit-PatchSet: 4
Gerrit-Owner: Ádám Bakai
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wang Xixu <1450306...@qq.com>
Gerrit-Reviewer: Zoltan Martonka
Gerrit-Comment-Date: Tue, 16 Jan 2024 07:48:30 +
Gerrit-HasComments: Yes
[kudu-CR] [log block manager-test] Improve random selection
Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/20899 ) Change subject: [log_block_manager-test] Improve random selection .. Patch Set 4: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/20899/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20899/4//COMMIT_MSG@9 PS4, Line 9: because it : was possible that way less blocks are removed than expected I got what you mean. Could you also give more detail to explain why the previous solution maybe cause this problem. http://gerrit.cloudera.org:8080/#/c/20899/4/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/20899/4/src/kudu/fs/log_block_manager-test.cc@1298 PS4, Line 1298: ids.erase(ids.begin(), ids.begin() + static_cast(kLiveBlockRatio * kNumBlocks)); Is it better to use another variable to store the to-delete ids, for example `to_deleted_ids`? `ids` may be used in other place later. -- To view, visit http://gerrit.cloudera.org:8080/20899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce Gerrit-Change-Number: 20899 Gerrit-PatchSet: 4 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Comment-Date: Tue, 16 Jan 2024 04:03:17 + Gerrit-HasComments: Yes
[kudu-CR] [log block manager-test] Improve random selection
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20899 to look at the new patch set (#4). Change subject: [log_block_manager-test] Improve random selection .. [log_block_manager-test] Improve random selection The previous solution to remove random blocks was not good, because it was possible that way less blocks are removed than expected. The new approach removes exactly the same number of blocks every time but randomizes which blocks are removed and the order of removal is randomized, too. Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce --- M src/kudu/fs/log_block_manager-test.cc 1 file changed, 4 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/20899/4 -- To view, visit http://gerrit.cloudera.org:8080/20899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce Gerrit-Change-Number: 20899 Gerrit-PatchSet: 4 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [log block manager-test] Improve random selection
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20899 to look at the new patch set (#3). Change subject: [log_block_manager-test] Improve random selection .. [log_block_manager-test] Improve random selection The previous solution to remove random blocks was not good, because it was possible that way less blocks are removed than expected. The new approach removes exactly the same number of blocks every time but randomizes which blocks are removed and the order of removal is randomized, too. Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce --- M src/kudu/fs/log_block_manager-test.cc 1 file changed, 4 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/20899/3 -- To view, visit http://gerrit.cloudera.org:8080/20899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce Gerrit-Change-Number: 20899 Gerrit-PatchSet: 3 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [log block manager-test] Improve random selection logic
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20899 to look at the new patch set (#2). Change subject: [log_block_manager-test] Improve random selection logic .. [log_block_manager-test] Improve random selection logic The previous solution to remove random blocks was not good, because it was possible that way less blocks are removed than expected. The new approach removes exactly the same number of blocks every time but randomizes which blocks are removed and the order of removal is randomized, too. Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce --- M src/kudu/fs/log_block_manager-test.cc 1 file changed, 4 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/20899/2 -- To view, visit http://gerrit.cloudera.org:8080/20899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce Gerrit-Change-Number: 20899 Gerrit-PatchSet: 2 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [log block manager-test] Improve random selection logic
Ádám Bakai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20899 Change subject: [log_block_manager-test] Improve random selection logic .. [log_block_manager-test] Improve random selection logic The previous solution to remove random blocks was not good, because it was possible that way less blocks are removed than expected. The new approach removes exactly the same number of blocks every time but randomizes which blocks are removed and the order of removal is randomized, too. Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce --- M src/kudu/fs/log_block_manager-test.cc 1 file changed, 4 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/20899/1 -- To view, visit http://gerrit.cloudera.org:8080/20899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I9f73f0572319784b07845b02d15a7d3e6f31abce Gerrit-Change-Number: 20899 Gerrit-PatchSet: 1 Gerrit-Owner: Ádám Bakai
