[kudu-CR] Fix typos
Ádám Bakai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20927 Change subject: Fix typos .. Fix typos Change-Id: I71524db97455bb27db44b5820cbe81fd95e207d9 --- M src/kudu/fs/dir_manager.cc M src/kudu/tools/ksck_checksum.h M src/kudu/util/minidump.h 3 files changed, 3 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/20927/1 -- To view, visit http://gerrit.cloudera.org:8080/20927 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I71524db97455bb27db44b5820cbe81fd95e207d9 Gerrit-Change-Number: 20927 Gerrit-PatchSet: 1 Gerrit-Owner: Ádám Bakai
[kudu-CR] Fix CheckHolePunch for bigger than 4k blocks.
Zoltan Martonka has posted comments on this change. ( http://gerrit.cloudera.org:8080/20894 ) Change subject: Fix CheckHolePunch for bigger than 4k blocks. .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/20894/2/src/kudu/fs/dir_util.cc File src/kudu/fs/dir_util.cc: http://gerrit.cloudera.org:8080/#/c/20894/2/src/kudu/fs/dir_util.cc@a68 PS2, Line 68: > Maybe I am missing something here. PreAllocate always calls allocate, you are right. If you call it with non-whole blocks it just rounds up the reservation. If you allocate 16k it will take a whole 64k block. If you would allocate 80k, it would take up 128k. If you holepunch a non-aligned interval, it will zero the bits out, but only free up blocks that are entirely in the interval. (e.g if you holepunch from [2k,10k] on a 4k filesystem, it will only free up the 2nd block. it will zero out the second half of the 1st block and the first half of the 3rd block). If you do multiple holepunches, that overlap, it won't free up blocks that are shared between. (e.g on a 4k filesystem you holepunch [2k, 6k] [6k, 10k], then the 2nd block (4k to 8k)will still be reserved by the file. Here is an easy to run c++ example (for xfs): https://github.com/martonka/kudu_random_stuff/blob/master/cpp/hole_punch_test.cpp -- To view, visit http://gerrit.cloudera.org:8080/20894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib4af73f0aa25db674fe0a34355cecd27a0c68417 Gerrit-Change-Number: 20894 Gerrit-PatchSet: 5 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Fri, 19 Jan 2024 13:31:29 + Gerrit-HasComments: Yes
[kudu-CR] Fix typos
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20927 ) Change subject: Fix typos .. Fix typos Change-Id: I71524db97455bb27db44b5820cbe81fd95e207d9 Reviewed-on: http://gerrit.cloudera.org:8080/20927 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M src/kudu/fs/dir_manager.cc M src/kudu/tools/ksck_checksum.h M src/kudu/util/minidump.h 3 files changed, 3 insertions(+), 3 deletions(-) Approvals: Kudu Jenkins: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/20927 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I71524db97455bb27db44b5820cbe81fd95e207d9 Gerrit-Change-Number: 20927 Gerrit-PatchSet: 2 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Fix typos
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20927 ) Change subject: Fix typos .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/20927 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I71524db97455bb27db44b5820cbe81fd95e207d9 Gerrit-Change-Number: 20927 Gerrit-PatchSet: 1 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 19 Jan 2024 14:35:30 + Gerrit-HasComments: No
[kudu-CR] KUDU-3527 Fix block manager test on 64k filesystems
Hello Mahesh Reddy, Alexey Serbin, Ashwani Raina, Kudu Jenkins, Abhishek Chennaka, Wang Xixu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20725 to look at the new patch set (#7). Change subject: KUDU-3527 Fix block manager test on 64k filesystems .. KUDU-3527 Fix block manager test on 64k filesystems BlockManagerTest.TestMetadataOkayDespiteFailure might fail on systems where fs_block_size=64k. Currently tablets fail to load if one metadata is missing but there is still a non-empty ".data" file. If FLAGS_env_inject_eio is not zero, then there is a chance that, when we delete the container file, we only delete the ".meta", but leave the ".data" file. In the current test on systems with fs_block_size=4k deletion never occurs. Changing to kNumAppends=64 will cause the test to randomly fail on x86 systems too, although only with a 2-3% chance (at least on my ubuntu20 machine). The short term solution is to make sure no container becomes full. A jira was created: KUDU-3528 and a test was added: DISABLED_TestReloadBlockManagerDespiteFailure for the original issue. Change-Id: I7e325bde502b7d7f39dd17fa84cb7eb42a3d7c20 --- M src/kudu/fs/block_manager-test.cc 1 file changed, 98 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/20725/7 -- To view, visit http://gerrit.cloudera.org:8080/20725 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7e325bde502b7d7f39dd17fa84cb7eb42a3d7c20 Gerrit-Change-Number: 20725 Gerrit-PatchSet: 7 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka
[kudu-CR](branch-1.17.x) KUDU-3526 [java] Scanner should bind with a tserver in java client.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20923 ) Change subject: KUDU-3526 [java] Scanner should bind with a tserver in java client. .. Patch Set 1: Verified+1 unrelated test failure in IgnoredTserverGoesDownDuringRebalancingTest.TserverDown -- To view, visit http://gerrit.cloudera.org:8080/20923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.17.x Gerrit-MessageType: comment Gerrit-Change-Id: I9cf65f4215e99198dd41b43d14e50c8c23b8a9b2 Gerrit-Change-Number: 20923 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng Gerrit-Reviewer: Yifan Zhang Gerrit-Comment-Date: Fri, 19 Jan 2024 18:08:05 + Gerrit-HasComments: No
[kudu-CR](branch-1.17.x) KUDU-3526 [java] Scanner should bind with a tserver in java client.
Alexey Serbin has removed a vote on this change. Change subject: KUDU-3526 [java] Scanner should bind with a tserver in java client. .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/20923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.17.x Gerrit-MessageType: deleteVote Gerrit-Change-Id: I9cf65f4215e99198dd41b43d14e50c8c23b8a9b2 Gerrit-Change-Number: 20923 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng Gerrit-Reviewer: Yifan Zhang
[kudu-CR](branch-1.17.x) KUDU-3491 Destruct master before creating a new one
Abhishek Chennaka has submitted this change and it was merged. ( 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) Reviewed-on: http://gerrit.cloudera.org:8080/20917 Tested-by: Alexey Serbin Reviewed-by: Abhishek Chennaka --- M src/kudu/master/dynamic_multi_master-test.cc M src/kudu/master/master_runner.cc 2 files changed, 43 insertions(+), 2 deletions(-) Approvals: Alexey Serbin: Verified Abhishek Chennaka: Looks good to me, approved -- 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: merged Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Gerrit-Change-Number: 20917 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai
[kudu-CR](branch-1.17.x) KUDU-3526 [java] Scanner should bind with a tserver in java client.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/20923 ) Change subject: KUDU-3526 [java] Scanner should bind with a tserver in java client. .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/20923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.17.x Gerrit-MessageType: comment Gerrit-Change-Id: I9cf65f4215e99198dd41b43d14e50c8c23b8a9b2 Gerrit-Change-Number: 20923 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng Gerrit-Reviewer: Yifan Zhang Gerrit-Comment-Date: Fri, 19 Jan 2024 18:59:04 + Gerrit-HasComments: No
[kudu-CR](branch-1.17.x) KUDU-3491 Destruct master before creating a new one
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/20917 ) Change subject: KUDU-3491 Destruct master before creating a new one .. Patch Set 1: Code-Review+2 -- 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: comment Gerrit-Change-Id: I3c1019d092bbf9e58f4fc33753a1218bc79735d3 Gerrit-Change-Number: 20917 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Fri, 19 Jan 2024 18:58:09 + Gerrit-HasComments: No
[kudu-CR](branch-1.17.x) KUDU-3526 [java] Scanner should bind with a tserver in java client.
Abhishek Chennaka has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20923 ) Change subject: KUDU-3526 [java] Scanner should bind with a tserver in java client. .. KUDU-3526 [java] Scanner should bind with a tserver in java client. For now, scan requests sent by the java client might fail with scanner not found error. It is because that scanner does not bind with the tserver with which it first communicates. The code in method scanNextRows of java client is still trying to search for the tserver by the locations and selection policy. So if the leader is changed and the next scan request is sent to the new leader, the tserver will respond with the "scanner not found" exception. We should bind the scanner to the tserver, similar to how it is done in the C++ client. Change-Id: I9cf65f4215e99198dd41b43d14e50c8c23b8a9b2 Reviewed-on: http://gerrit.cloudera.org:8080/20715 Tested-by: Kudu Jenkins Reviewed-by: Yifan Zhang Reviewed-by: Alexey Serbin (cherry picked from commit 3895f4f796579071615addfa12d2abf75753b9a1) Reviewed-on: http://gerrit.cloudera.org:8080/20923 Tested-by: Alexey Serbin Reviewed-by: Abhishek Chennaka --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java 4 files changed, 116 insertions(+), 7 deletions(-) Approvals: Alexey Serbin: Verified Abhishek Chennaka: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/20923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.17.x Gerrit-MessageType: merged Gerrit-Change-Id: I9cf65f4215e99198dd41b43d14e50c8c23b8a9b2 Gerrit-Change-Number: 20923 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng Gerrit-Reviewer: Yifan Zhang
[kudu-CR] KUDU-3527 Fix block manager test on 64k filesystems
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20725 ) Change subject: KUDU-3527 Fix block manager test on 64k filesystems .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/20725/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20725/7//COMMIT_MSG@10 PS7, Line 10: fs_block_size What's "fs_block_size"? Is it indeed the filesystem's block size (i.e. what 'stat -f' reports) or this is about the IO block size (a.k.a. optimal block size for I/O, or what 'stat' reports)? It would great to have more clarity here in the commit message at least since there is a lot of confusion around this topic already, and bringing in some more isn't a good idea, not at all. Same for the comments in the code, etc. On my Graviton3 VM, the actual FS block size for an XFS filesystem is 4K, but the optimal IO block size is 64K: [root@aserbin-rh88-graviton3 ~]# stat -f .bashrc File: ".bashrc" ID: 10303 Namelen: 255 Type: xfs Block size: 4096 Fundamental block size: 4096 Blocks: Total: 52243707 Free: 9130378Available: 9130378 Inodes: Total: 73555248 Free: 73076679 [root@aserbin-rh88-graviton3 ~]# stat .bashrc File: .bashrc Size: 307 Blocks: 8 IO Block: 65536 regular file Device: 10303h/66307d Inode: 1223994 Links: 1 Access: (0644/-rw-r--r--) Uid: (0/root) Gid: (0/root) Access: 2024-01-19 11:03:05.043364091 -0800 Modify: 2023-10-06 16:47:49.797395518 -0700 Change: 2023-10-06 16:47:49.797395518 -0700 Birth: 2023-10-06 16:47:49.797395518 -0700 -- To view, visit http://gerrit.cloudera.org:8080/20725 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7e325bde502b7d7f39dd17fa84cb7eb42a3d7c20 Gerrit-Change-Number: 20725 Gerrit-PatchSet: 7 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Fri, 19 Jan 2024 19:17:58 + Gerrit-HasComments: Yes
[kudu-CR] Fix CheckHolePunch for bigger than 4k blocks.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20894 ) Change subject: Fix CheckHolePunch for bigger than 4k blocks. .. Patch Set 5: Code-Review+2 Thank you for the fix! I verified it works as expected, at least at Graviton3 VM instance with FS logical block sizes of 8K, 16K, 32K, and 64K. -- To view, visit http://gerrit.cloudera.org:8080/20894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib4af73f0aa25db674fe0a34355cecd27a0c68417 Gerrit-Change-Number: 20894 Gerrit-PatchSet: 5 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Fri, 19 Jan 2024 22:42:28 + Gerrit-HasComments: No
[kudu-CR] Fix CheckHolePunch for bigger than 4k blocks.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20894 ) Change subject: Fix CheckHolePunch for bigger than 4k blocks. .. Fix CheckHolePunch for bigger than 4k blocks. With the new 64k ARM core, you can use filesystems with up to 64k block size. Currently Kudu does not start if the data is on a filesystem with larger than 4k blocks. Problem: CheckHolePunch has hard-coded 4k block size instead of using the get env->GetBlockSize. Kudu seems to be running fine on a 64k fs after this fix. More info: https://kudu.apache.org/docs/troubleshooting.html#req_hole_punching Change-Id: Ib4af73f0aa25db674fe0a34355cecd27a0c68417 Reviewed-on: http://gerrit.cloudera.org:8080/20894 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M src/kudu/fs/dir_util.cc 1 file changed, 8 insertions(+), 6 deletions(-) Approvals: Kudu Jenkins: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/20894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ib4af73f0aa25db674fe0a34355cecd27a0c68417 Gerrit-Change-Number: 20894 Gerrit-PatchSet: 6 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka
[kudu-CR](branch-1.17.x) Fix CheckHolePunch for bigger than 4k blocks.
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20930 Change subject: Fix CheckHolePunch for bigger than 4k blocks. .. Fix CheckHolePunch for bigger than 4k blocks. With the new 64k ARM core, you can use filesystems with up to 64k block size. Currently Kudu does not start if the data is on a filesystem with larger than 4k blocks. Problem: CheckHolePunch has hard-coded 4k block size instead of using the get env->GetBlockSize. Kudu seems to be running fine on a 64k fs after this fix. More info: https://kudu.apache.org/docs/troubleshooting.html#req_hole_punching Change-Id: Ib4af73f0aa25db674fe0a34355cecd27a0c68417 Reviewed-on: http://gerrit.cloudera.org:8080/20894 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin (cherry picked from commit de87aca3dd29177658790cffeb7a1de0c8d69231) --- M src/kudu/fs/dir_util.cc 1 file changed, 8 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/20930/1 -- To view, visit http://gerrit.cloudera.org:8080/20930 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.17.x Gerrit-MessageType: newchange Gerrit-Change-Id: Ib4af73f0aa25db674fe0a34355cecd27a0c68417 Gerrit-Change-Number: 20930 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Zoltan Martonka
[kudu-CR] KUDU-3527 Fix block manager test on 64k filesystems
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/20725 )
Change subject: KUDU-3527 Fix block manager test on 64k filesystems
..
Patch Set 7:
(9 comments)
Overall looks good to me, but I think this patch needs a bit of clean-up at
least easier comprehension later on when it's time to enable the test.
http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc@928
PS7, Line 928: const int kNumTries = 3;
: const int kNumBlockTries = 500;
: const int kNumAppends = 4;
nit: could these be constexpr?
http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc@937
PS7, Line 937: With non-zero crash_on_eio
nit: With crash_on_eio enabled
http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc@939
PS7, Line 939: It is not the
: // scope of this test to test this case.
This is looks quite confusing given what this test scenario is doing. Instead
of this, maybe it's better to add a comment at line 926 to explain why this
test is here and why it's added as a disabled one?
Overall, please update this block comment, clarifying on the following points:
* the 'what' part: what this test scenario is doing
* the 'why' part: why this test scenario is here and why it is added as a
disabled one?
* the 'when' part: when this test scenario should be enabled, if ever?
You might consider splitting it in multiple piece, placing some of them at line
926, prefacing the scenario.
http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc@942
PS7, Line 942: arm
nit ARM
http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc@942
PS7, Line 942: like the 64k option of rhel 8.8
What are those exactly?
http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc@951
PS7, Line 951: If you set a lower value, the test becomes flaky. With 0.2 it
almost always fails.
This might become confusing later on when the root cause of KUDU-3528 is
addressed: it's not very likely that this comment will be fixed once the
DISABLED_ prefix is removed.
Maybe, just mention what exactly happens here, not resorting to 'flaky' and
'test failure' terms? I'd think of explaining that the scenario ends up with
non-empty .data file and no .meta file much less often if FLAGS_env_inject_eio
set to a value less than 0.2.
http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc@958
PS7, Line 958: the result
nit: the result block ID
http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc@982
PS7, Line 982: if (s.ok()) {
: InsertOrDie(&ids, id);
: num_created++;
: }
If this is just a way to overcome injected IO errors, why not to set
FLAGS_env_inject_eio = 0 in the scope while calling create_a_block()?
Otherwise, please add a comment explaining why it's important to have IO error
injections enabled when creating blocks.
http://gerrit.cloudera.org:8080/#/c/20725/7/src/kudu/fs/block_manager-test.cc@999
PS7, Line 999: if (it != ids.end()) {
: it++;
: }
nit for here and elsewhere: it would be great if you preserved more of the
original comments from the TestMetadataOkayDespiteFailure scenario that are
still helpful to the reader to understand what's the intention behind some of
these actions.
--
To view, visit http://gerrit.cloudera.org:8080/20725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e325bde502b7d7f39dd17fa84cb7eb42a3d7c20
Gerrit-Change-Number: 20725
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Martonka
Gerrit-Reviewer: Abhishek Chennaka
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Ashwani Raina
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy
Gerrit-Reviewer: Wang Xixu <1450306...@qq.com>
Gerrit-Reviewer: Zoltan Martonka
Gerrit-Comment-Date: Fri, 19 Jan 2024 23:55:43 +
Gerrit-HasComments: Yes
[kudu-CR](branch-1.17.x) Fix CheckHolePunch for bigger than 4k blocks.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20930 ) Change subject: Fix CheckHolePunch for bigger than 4k blocks. .. Patch Set 1: Verified+1 unrelated test failure in MasterMigrationTest.TestEndToEndMigration (TSAN): KUDU-2439 -- To view, visit http://gerrit.cloudera.org:8080/20930 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.17.x Gerrit-MessageType: comment Gerrit-Change-Id: Ib4af73f0aa25db674fe0a34355cecd27a0c68417 Gerrit-Change-Number: 20930 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Sat, 20 Jan 2024 00:45:51 + Gerrit-HasComments: No
[kudu-CR](branch-1.17.x) Fix CheckHolePunch for bigger than 4k blocks.
Alexey Serbin has removed a vote on this change. Change subject: Fix CheckHolePunch for bigger than 4k blocks. .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/20930 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.17.x Gerrit-MessageType: deleteVote Gerrit-Change-Id: Ib4af73f0aa25db674fe0a34355cecd27a0c68417 Gerrit-Change-Number: 20930 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Martonka
