[kudu-CR] [client] KUDU-3595: Add a way to set Kudu client's rpc max message size
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21622 ) Change subject: [client] KUDU-3595: Add a way to set Kudu client's rpc_max_message_size .. Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/21622/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21622/2//COMMIT_MSG@9 PS2, Line 9: clien't > nit: client's Done http://gerrit.cloudera.org:8080/#/c/21622/2//COMMIT_MSG@14 PS2, Line 14: Will try to add one either : in this patch > Definitely, it's better to add a test in this patch. Otherwise, how do you I have tested this patch using repro steps mentioned IMPALA-13202 in a kudu + impala co-dev setup by setting message max size in KuduClientBuilder object which in turn is used in client library for usage during receive. However, patch doesn't take into consideration the points you made here: https://gerrit.cloudera.org/c/21622/2/src/kudu/client/client.cc#398 I will plan to add unit test as stated above with refined approach. http://gerrit.cloudera.org:8080/#/c/21622/2/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/21622/2/src/kudu/client/client.h@317 PS2, Line 317: const > 'const' for parameters of integer types doesn't make much sense since they Done http://gerrit.cloudera.org:8080/#/c/21622/2/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/21622/2/src/kudu/client/client.cc@397 PS2, Line 397: rpc_max_message_size_ > I don't think this field was initialized to 0, wasn't it? Done http://gerrit.cloudera.org:8080/#/c/21622/2/src/kudu/client/client.cc@398 PS2, Line 398: FLAGS_rpc_max_message_size = data_->rpc_max_message_size_; > I don't think this is the right way of implementing the required functional I didn't consider the possibility of client integrated into kudu-tserver. Thanks for pointing that out! I thought about the first concern from the usage perspective. My understanding was that it is likely that throughout the application runtime the value of the flag is required to be of similar size to accommodate the payload. However, that may not be the case as it takes away the flexibility of granular parameter settings for each client instance based on their individual requirements. For now, I will move this patch to WIP state to accommodate the better approach. http://gerrit.cloudera.org:8080/#/c/21622/2/src/kudu/client/client_builder-internal.h File src/kudu/client/client_builder-internal.h: http://gerrit.cloudera.org:8080/#/c/21622/2/src/kudu/client/client_builder-internal.h@44 PS2, Line 44: rpc_max_message_size_ > To avoid using special values, consider using std::optional for this, simil Will revisit this in different approach. http://gerrit.cloudera.org:8080/#/c/21622/2/src/kudu/client/client_builder-internal.h@44 PS2, Line 44: int64_t > Would this make more sense as an unsigned value? Will revisit this in different approach. -- To view, visit http://gerrit.cloudera.org:8080/21622 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib8feb5ba92ea604442a643e3286944564e655fa6 Gerrit-Change-Number: 21622 Gerrit-PatchSet: 2 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Tue, 30 Jul 2024 10:16:16 + Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3595: Add a way to set Kudu client's rpc max message size
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21622 to look at the new patch set (#2). Change subject: [client] KUDU-3595: Add a way to set Kudu client's rpc_max_message_size .. [client] KUDU-3595: Add a way to set Kudu client's rpc_max_message_size Kudu clien't default capacity to hold RPC payload can be low in cases where payload goes beyond 50MB. This patch adds a method in KuduClientBuilder to increase the size for client as per application needs. The patch doesn't contain a unit test. Will try to add one either in this patch or in a follow-up patch. Change-Id: Ib8feb5ba92ea604442a643e3286944564e655fa6 --- M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/client_builder-internal.h 3 files changed, 22 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/21622/2 -- To view, visit http://gerrit.cloudera.org:8080/21622 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib8feb5ba92ea604442a643e3286944564e655fa6 Gerrit-Change-Number: 21622 Gerrit-PatchSet: 2 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [client] KUDU-3595: Add a way to set Kudu client's rpc max message size
Ashwani Raina has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21622 Change subject: [client] KUDU-3595: Add a way to set Kudu client's rpc_max_message_size .. [client] KUDU-3595: Add a way to set Kudu client's rpc_max_message_size Kudu clien't default capacity to hold RPC payload can be low in cases where payload goes beyond 50MB. This patch adds a method in KuduClientBuilder to increase the size for client as per application needs. Change-Id: Ib8feb5ba92ea604442a643e3286944564e655fa6 --- M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/client_builder-internal.h 3 files changed, 22 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/21622/1 -- To view, visit http://gerrit.cloudera.org:8080/21622 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ib8feb5ba92ea604442a643e3286944564e655fa6 Gerrit-Change-Number: 21622 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina
[kudu-CR] [tests] re-enable ReplaceTabletsWhileWriting scenario
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21612 ) Change subject: [tests] re-enable ReplaceTabletsWhileWriting scenario .. Patch Set 2: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/21612 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I36b0e69022f07dd701a91e72e16c93f134d00619 Gerrit-Change-Number: 21612 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 25 Jul 2024 13:10:07 + Gerrit-HasComments: No
[kudu-CR] [client] add ScanTokenStaleRaftMembershipTest
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21580 ) Change subject: [client] add ScanTokenStaleRaftMembershipTest .. Patch Set 3: Code-Review+1 (1 comment) LGTM. Just a nitpicker. I am fine if you don't feel the need to address it. http://gerrit.cloudera.org:8080/#/c/21580/3/src/kudu/client/scan_token-test.cc File src/kudu/client/scan_token-test.cc: http://gerrit.cloudera.org:8080/#/c/21580/3/src/kudu/client/scan_token-test.cc@1699 PS3, Line 1699: During the Raft election round, WriteRequestPB : // RPCs should be rejected with REPLICA_NOT_LEADER error code by every replica : // of the tablet nit: Maybe put an ASSERT_STR_CONTAINS for "is not leader of this config" here to ensure the statement is true. -- To view, visit http://gerrit.cloudera.org:8080/21580 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6ce3d549d4ab2502c58deae1250b49ba16bbc914 Gerrit-Change-Number: 21580 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 15 Jul 2024 07:09:06 + Gerrit-HasComments: Yes
[kudu-CR] [client] add ScanTokenStaleRaftMembershipTest
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21580 ) Change subject: [client] add ScanTokenStaleRaftMembershipTest .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/21580/1/src/kudu/client/scan_token-test.cc File src/kudu/client/scan_token-test.cc: http://gerrit.cloudera.org:8080/#/c/21580/1/src/kudu/client/scan_token-test.cc@1590 PS1, Line 1590: // ??? nit: Remove http://gerrit.cloudera.org:8080/#/c/21580/1/src/kudu/client/scan_token-test.cc@1702 PS1, Line 1702: ASSERT_OK(LeaderStepDown(tsd, tablet_id, MonoDelta::FromSeconds(30))); Do you think it would makes sense to add an assert to verify the new_leader_uuid matches with leader uuid? -- To view, visit http://gerrit.cloudera.org:8080/21580 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6ce3d549d4ab2502c58deae1250b49ba16bbc914 Gerrit-Change-Number: 21580 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 12 Jul 2024 12:55:59 + Gerrit-HasComments: Yes
[kudu-CR] [KUDU-3583] Revert the default value of the flag --tablet history max age sec
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21481 ) Change subject: [KUDU-3583] Revert the default value of the flag --tablet_history_max_age_sec .. Patch Set 2: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/21481/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21481/2//COMMIT_MSG@10 PS2, Line 10: --tablet_history_max_age_sec to 15 minutes to avoid write amplifications > I guess there isn't a trade-off that fits every use case. There was an att I remember we had this discussion over slack a while back. The only concern is that after changing it back to 15 minutes, we might start getting customer issues where they are not able to scan old updates. However, I agree that the benefits from decreasing the value are quite worthy, maybe even more. Along with having follow-up patches to introduce more meaningful message, we could also document the fact so that customer who have been relying on default value of 7 days are aware of this change and can make informed decision as far as scanning requirement for old updates is concerned. That way they can avoid running into the situation in the first place. -- To view, visit http://gerrit.cloudera.org:8080/21481 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I63556578bafe2bd49136850f488bd3b656728a4a Gerrit-Change-Number: 21481 Gerrit-PatchSet: 2 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 12 Jul 2024 07:54:21 + Gerrit-HasComments: Yes
[kudu-CR] Update CMake to 3.22.6
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21569 ) Change subject: Update CMake to 3.22.6 .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/21569/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21569/2//COMMIT_MSG@17 PS2, Line 17: ubuntu 18.04, 20.04, 22.04, rhel 8.10 It would be great if you can test it on SLES 12 SP5 and SLES 15 SP4/SP5 as well. -- To view, visit http://gerrit.cloudera.org:8080/21569 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I563e188b6692ebfe0e0b3e98577961a8380f1f31 Gerrit-Change-Number: 21569 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Thu, 11 Jul 2024 11:02:14 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3577 fix altering tables with custom hash schemas
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21486 ) Change subject: KUDU-3577 fix altering tables with custom hash schemas .. Patch Set 2: Code-Review+1 (3 comments) LGTM. Just a couple of nits. http://gerrit.cloudera.org:8080/#/c/21486/2/src/kudu/integration-tests/alter_table-test.cc File src/kudu/integration-tests/alter_table-test.cc: http://gerrit.cloudera.org:8080/#/c/21486/2/src/kudu/integration-tests/alter_table-test.cc@2232 PS2, Line 2232: { nit: Add a comment about dropping a column in this code block. http://gerrit.cloudera.org:8080/#/c/21486/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/21486/2/src/kudu/master/catalog_manager.cc@3031 PS2, Line 3031: !current_pb.partition_schema().custom_hash_schema_ranges().empty() && Just curious: Would this be applicable to cases other than DROP_COLUMN? If not, would it make sense to move this block under applicable case only, to avoid unnecessary checks? http://gerrit.cloudera.org:8080/#/c/21486/2/src/kudu/master/catalog_manager.cc@3782 PS2, Line 3782: nit: remove whitespaces -- To view, visit http://gerrit.cloudera.org:8080/21486 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I21a775538063768b986edd2b6bc25d03360b5216 Gerrit-Change-Number: 21486 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Fri, 07 Jun 2024 12:20:05 + Gerrit-HasComments: Yes
[kudu-CR] Add a benchmark for CBTree concurrent writes.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21447 ) Change subject: Add a benchmark for CBTree concurrent writes. .. Patch Set 8: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/21447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1b0b16e269c70716962fc5ebb4ddca1e2cbe68a4 Gerrit-Change-Number: 21447 Gerrit-PatchSet: 8 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Chovan Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Wed, 29 May 2024 14:42:43 + Gerrit-HasComments: No
[kudu-CR] Add a benchmark for CBTree concurrent writes.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21447 ) Change subject: Add a benchmark for CBTree concurrent writes. .. Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/21447/4/src/kudu/tablet/cbtree-test.cc File src/kudu/tablet/cbtree-test.cc: http://gerrit.cloudera.org:8080/#/c/21447/4/src/kudu/tablet/cbtree-test.cc@57 PS4, Line 57: N > No specific reason, I just had 8 core. So I put it to +1. Also I wanted dif You could use the same logic to output of hardware_concurrency() as you did for 8 cores. However, it is fine if you don't see much benefit in using hardware_concurrency(). http://gerrit.cloudera.org:8080/#/c/21447/8/src/kudu/tablet/cbtree-test.cc File src/kudu/tablet/cbtree-test.cc: http://gerrit.cloudera.org:8080/#/c/21447/8/src/kudu/tablet/cbtree-test.cc@924 PS8, Line 924: ConcurrentReadWriteBenchmark Don't want to delay your commit. Would it make sense to include this as part of benchmark suite? I noticed the TestScanPerformance is already part of the suite(https://github.com/apache/kudu/blob/8d5f82483665fd6229d08fdfe94c87b07f80f986/src/kudu/scripts/benchmarks.sh#L246-L251). Maybe we can add this test as well. I am fine with doing that as a separate change. -- To view, visit http://gerrit.cloudera.org:8080/21447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1b0b16e269c70716962fc5ebb4ddca1e2cbe68a4 Gerrit-Change-Number: 21447 Gerrit-PatchSet: 8 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Chovan Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Wed, 29 May 2024 14:38:24 + Gerrit-HasComments: Yes
[kudu-CR](branch-1.17.x) Update build pattern for fetching flaky tests list
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21466 ) Change subject: Update build pattern for fetching flaky tests list .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/21466 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.17.x Gerrit-MessageType: comment Gerrit-Change-Id: I317a3a32c06c06306b97566f954e0ffd508ce01f Gerrit-Change-Number: 21466 Gerrit-PatchSet: 1 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Comment-Date: Wed, 29 May 2024 08:39:46 + Gerrit-HasComments: No
[kudu-CR] KUDU-3580 the crash bug when run binaries on older CPU machines
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21287 ) Change subject: KUDU-3580 the crash bug when run binaries on older CPU machines .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/21287/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21287/3//COMMIT_MSG@7 PS3, Line 7: the crash bug when run binaries nit: Fix the crash caused when binaries run http://gerrit.cloudera.org:8080/#/c/21287/3//COMMIT_MSG@15 PS3, Line 15: This patch enables the PORTABLE option when building : librocksdb to fix the issue. It would be great if you could expand this a little bit to explain how PORTABLE option fixes this issue or if there is a link documentation, point to it here. -- To view, visit http://gerrit.cloudera.org:8080/21287 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id30ae995c41a592fccbdb822bc1f457c5e6878ac Gerrit-Change-Number: 21287 Gerrit-PatchSet: 3 Gerrit-Owner: Yingchun Lai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Wed, 29 May 2024 06:48:00 + Gerrit-HasComments: Yes
[kudu-CR] Add a benchmark for CBTree concurrent writes.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21447 ) Change subject: Add a benchmark for CBTree concurrent writes. .. Patch Set 4: (2 comments) First pass. Overall looks good. Couple of nits/questions. http://gerrit.cloudera.org:8080/#/c/21447/4/src/kudu/tablet/cbtree-test.cc File src/kudu/tablet/cbtree-test.cc: http://gerrit.cloudera.org:8080/#/c/21447/4/src/kudu/tablet/cbtree-test.cc@57 PS4, Line 57: 9 Any specific reason behind choosing these numbers for number of writes/readers/inserts? For number of reads/writes, would it make sense to use hardware_concurrency() (https://en.cppreference.com/w/cpp/thread/thread/hardware_concurrency) to derive maximum feasible concurrency at runtime? http://gerrit.cloudera.org:8080/#/c/21447/4/src/kudu/tablet/cbtree-test.cc@936 PS4, Line 936: 83 Add a one liner comment on why you chose this. -- To view, visit http://gerrit.cloudera.org:8080/21447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1b0b16e269c70716962fc5ebb4ddca1e2cbe68a4 Gerrit-Change-Number: 21447 Gerrit-PatchSet: 4 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Mon, 27 May 2024 14:45:09 + Gerrit-HasComments: Yes
[kudu-CR] Fix deadlock on fail for CBTree-test
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21446 ) Change subject: Fix deadlock on fail for CBTree-test .. Patch Set 5: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/21446 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia10d05dfdc4a12cb034450f432693f054d138498 Gerrit-Change-Number: 21446 Gerrit-PatchSet: 5 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Mon, 27 May 2024 08:26:53 + Gerrit-HasComments: No
[kudu-CR] Fix deadlock on fail for CBTree-test
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21446 ) Change subject: Fix deadlock on fail for CBTree-test .. Patch Set 3: (3 comments) Overall looks good. just a few nits. http://gerrit.cloudera.org:8080/#/c/21446/3/src/kudu/tablet/cbtree-test.cc File src/kudu/tablet/cbtree-test.cc: http://gerrit.cloudera.org:8080/#/c/21446/3/src/kudu/tablet/cbtree-test.cc@451 PS3, Line 451: num_threads + 1 just curious, why is count initialised to 1 extra than number of threads? http://gerrit.cloudera.org:8080/#/c/21446/3/src/kudu/tablet/cbtree-test.cc@483 PS3, Line 483: go_barrier.Wait() Here and at line 748, make sure extra barrier wait is ok. http://gerrit.cloudera.org:8080/#/c/21446/3/src/kudu/tablet/cbtree-test.cc@485 PS3, Line 485: thread & thread & -> thread& -- To view, visit http://gerrit.cloudera.org:8080/21446 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia10d05dfdc4a12cb034450f432693f054d138498 Gerrit-Change-Number: 21446 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 22 May 2024 12:35:19 + Gerrit-HasComments: Yes
[kudu-CR](branch-1.17.x) Fix row project codegen params noalias overflow
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21448 ) Change subject: Fix row_project codegen params noalias overflow .. Patch Set 2: > > I just wonder did kudu codegen work well if we use 1-based code. > > since LLVM changed it in LLVM 5.0.0. > > There have at least one known issue with codegen that we faced but > it is not yet established whether those were caused due to this > missing change. Also, LLVM upgrade from 4.0.0 to 6.0.0 happened in > 1.7.x (almost 6 years back). > > I am just curious, how did you discover this problem? Jira for known codegen issue - https://issues.apache.org/jira/browse/KUDU-3545 -- To view, visit http://gerrit.cloudera.org:8080/21448 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.17.x Gerrit-MessageType: comment Gerrit-Change-Id: I1dab7d46cff96ed1ebbd020584a066f04e6ca12a Gerrit-Change-Number: 21448 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Anonymous Coward Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 22 May 2024 08:25:19 + Gerrit-HasComments: No
[kudu-CR](branch-1.17.x) Fix row project codegen params noalias overflow
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21448 ) Change subject: Fix row_project codegen params noalias overflow .. Patch Set 2: > I just wonder did kudu codegen work well if we use 1-based code. > since LLVM changed it in LLVM 5.0.0. There have at least one known issue with codegen that we faced but it is not yet established whether those were caused due to this missing change. Also, LLVM upgrade from 4.0.0 to 6.0.0 happened in 1.7.x (almost 6 years back). I am just curious, how did you discover this problem? -- To view, visit http://gerrit.cloudera.org:8080/21448 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.17.x Gerrit-MessageType: comment Gerrit-Change-Id: I1dab7d46cff96ed1ebbd020584a066f04e6ca12a Gerrit-Change-Number: 21448 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Anonymous Coward Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 22 May 2024 08:22:25 + Gerrit-HasComments: No
[kudu-CR](branch-1.17.x) Fix row project codegen params noalias overflow
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21448 ) Change subject: Fix row_project codegen params noalias overflow .. Patch Set 1: Code-Review+1 (2 comments) Overall looks good to me. Just a couple of nits. http://gerrit.cloudera.org:8080/#/c/21448/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21448/1//COMMIT_MSG@9 PS1, Line 9: function->addParamAttr is 0-based indexes, current row_project generator IR code is: : `define i1 @ProjRead(i8* %src, %"class.kudu::RowBlockRow"* noalias %rbrow, %"class.kudu::Arena"* noalias %arena)` nit: alignment required. http://gerrit.cloudera.org:8080/#/c/21448/1//COMMIT_MSG@11 PS1, Line 11: with nit: as -- To view, visit http://gerrit.cloudera.org:8080/21448 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.17.x Gerrit-MessageType: comment Gerrit-Change-Id: I1dab7d46cff96ed1ebbd020584a066f04e6ca12a Gerrit-Change-Number: 21448 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Anonymous Coward Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 22 May 2024 07:26:55 + Gerrit-HasComments: Yes
[kudu-CR] Fix row project codegen params noalias overflow
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/19952 ) Change subject: Fix row_project codegen params noalias overflow .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/19952/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19952/5//COMMIT_MSG@9 PS5, Line 9: function->addParamAttr is 0-based indexes, current row_project generator IR code is: : `define i1 @ProjRead(i8* %src, %"class.kudu::RowBlockRow"* noalias %rbrow, %"class.kudu::Arena"* noalias %arena)` nit: alignment required -- To view, visit http://gerrit.cloudera.org:8080/19952 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1dab7d46cff96ed1ebbd020584a066f04e6ca12a Gerrit-Change-Number: 19952 Gerrit-PatchSet: 5 Gerrit-Owner: Anonymous Coward Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 22 May 2024 07:09:32 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3568 Fix compaction budgeting test by setting memory hard limit
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21416 to look at the new patch set (#2). Change subject: KUDU-3568 Fix compaction budgeting test by setting memory hard limit .. KUDU-3568 Fix compaction budgeting test by setting memory hard limit TestRowSetCompactionSkipWithBudgetingConstraints can fail if the memory on node running the test is high. It happens because the test generates deltas of size worth a few MBs that is multiplied with a preset factor to ensure the result (i.e. memory required for rowset compaction completion) is of high value of the order of 200 GB per rowset. Even though nodes running the test generally don't have so much physical memory, it is still possible to end up with high memory nodes. On such nodes, the test might fail. The patch fixes that problem by deterministically ensuring that compaction memory requirement is always higher than the memory hard limit. It does that by doing the following: 1. Move out the budgeting compaction tests out in a separate binary. 2. This gives flexibility to set the memory hard limit as per test needs. It is important to node that once a memory hard limit is set, it remains the same for all tests executed through binary lifecycle. 3. Set the hard memory limit to 1 GB which is enough to handle compaction requirements for TestRowSetCompactionProceedWithNoBudgetingConstraints. For TestRowSetCompactionSkipWithBudgetingConstraints, it is not enough because we set the delta memory factor high to exceed 1 GB. Both the test are now expected to succeed deterministically. Change-Id: I85d104e1d066507ce8e72a00cc5165cc4b85e48d --- M src/kudu/tablet/CMakeLists.txt A src/kudu/tablet/compaction-highmem-test.cc M src/kudu/tablet/compaction-test.cc 3 files changed, 221 insertions(+), 143 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/21416/2 -- To view, visit http://gerrit.cloudera.org:8080/21416 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I85d104e1d066507ce8e72a00cc5165cc4b85e48d Gerrit-Change-Number: 21416 Gerrit-PatchSet: 2 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3568 Fix compaction budgeting test by setting memory hard limit
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21416 ) Change subject: KUDU-3568 Fix compaction budgeting test by setting memory hard limit .. Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/21416/1/src/kudu/tablet/compaction-highmem-test.cc File src/kudu/tablet/compaction-highmem-test.cc: http://gerrit.cloudera.org:8080/#/c/21416/1/src/kudu/tablet/compaction-highmem-test.cc@18 PS1, Line 18: #include > nit: please use C++ style headers, i.e. Done. On a side note, it got added as part of iwyu fix. I wonder if there is a way to tell iwyu fix to add C++ style headers instead of this. http://gerrit.cloudera.org:8080/#/c/21416/1/src/kudu/tablet/compaction-highmem-test.cc@142 PS1, Line 142: (1024 * 1024 * 1024) > nit: drop the parentheses? Done http://gerrit.cloudera.org:8080/#/c/21416/1/src/kudu/tablet/compaction-highmem-test.cc@142 PS1, Line 142: FLAGS_memory_limit_hard_bytes = (1024 * 1024 * 1024); : : FLAGS_rowset_compaction_ancient_delta_threshold_enabled = true; : FLAGS_rowset_compaction_enforce_preset_factor = true; : FLAGS_rowset_compaction_memory_estimate_enabled = true; : : // Ensure memory budgeting applies : FLAGS_rowset_compaction_estimate_min_deltas_size_mb = 0; > Since this is in a dedicated binary now, consider setting these flags just Nice! Thanks for the tip. http://gerrit.cloudera.org:8080/#/c/21416/1/src/kudu/tablet/compaction-highmem-test.cc@166 PS1, Line 166: sw.elapsed().ToString(), : trace->MetricsAsJSON()); > nit: please fix the indent Done http://gerrit.cloudera.org:8080/#/c/21416/1/src/kudu/tablet/compaction-highmem-test.cc@179 PS1, Line 179: const uint32_t num_rowsets = 10; : const uint32_t num_rows_per_rowset = 2; : const uint32_t num_updates = 5000 * size_factor; > nit: could make these constexpr ? Done for first two. No need for num_updates as it is derived from size_factor parameter which cannot be a constant expression. -- To view, visit http://gerrit.cloudera.org:8080/21416 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I85d104e1d066507ce8e72a00cc5165cc4b85e48d Gerrit-Change-Number: 21416 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 13 May 2024 12:19:49 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3568 Fix compaction budgeting test by setting memory hard limit
Ashwani Raina has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21416 Change subject: KUDU-3568 Fix compaction budgeting test by setting memory hard limit .. KUDU-3568 Fix compaction budgeting test by setting memory hard limit TestRowSetCompactionSkipWithBudgetingConstraints can fail if the memory on node running the test is high. It happens because the test generates deltas of size worth a few MBs that is multiplied with a preset factor to ensure the result (i.e. memory required for rowset compaction completion) is of high value of the order of 200 GB per rowset. Even though nodes running the test generally don't have so much physical memory, it is still possible to end up with high memory nodes. On such nodes, the test might fail. The patch fixes that problem by deterministically ensuring that compaction memory requirement is always higher than the memory hard limit. It does that by doing the following: 1. Move out the budgeting compaction tests out in a separate binary. 2. This gives flexibility to set the memory hard limit as per test needs. It is important to node that once a memory hard limit is set, it remains the same for all tests executed through binary lifecycle. 3. Set the hard memory limit to 1 GB which is enough to handle compaction requirements for TestRowSetCompactionProceedWithNoBudgetingConstraints. For TestRowSetCompactionSkipWithBudgetingConstraints, it is not enough because we set the delta memory factor high to exceed 1 GB. Both the test are now expected to succeed deterministically. Change-Id: I85d104e1d066507ce8e72a00cc5165cc4b85e48d --- M src/kudu/tablet/CMakeLists.txt A src/kudu/tablet/compaction-highmem-test.cc M src/kudu/tablet/compaction-test.cc 3 files changed, 224 insertions(+), 143 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/21416/1 -- To view, visit http://gerrit.cloudera.org:8080/21416 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I85d104e1d066507ce8e72a00cc5165cc4b85e48d Gerrit-Change-Number: 21416 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina
[kudu-CR] KUDU-3568 Fix budgeting constraint test by enabling preset factor
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21360 ) Change subject: KUDU-3568 Fix budgeting constraint test by enabling preset factor .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/21360/2//COMMIT_MSG Commit Message: PS2: > Thank you for the write up -- I must admit I haven't read it yet to digest Thank you for running the test, Alexey. I was able to repro the issue on SLES15. The test is failing because the delta memory factor flag value is not big enough to force compaction budgeting policy logic to skip the compaction. Here physical memory available is of the order of 865 GBs. The test only sets max requirement to 200GB which is easily satisfied on SLES VM. So, instead of compaction being skipped, it goes ahead and completes. I think the right way to set memory requirement is to simply use process_memory::HardLimit() as the deciding element. Based on VM's physical memory, memory requirement can be set so that test expectations are met deterministically. Nevertheless, the current patch still holds although this addresses a different issue w.r.t. invalid usage of rowset_compaction_delta_memory_factor flag when preset factor flag not enabled. -- To view, visit http://gerrit.cloudera.org:8080/21360 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9df218cd2d8ef3709793db267d5a0d651421dbb6 Gerrit-Change-Number: 21360 Gerrit-PatchSet: 2 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Mon, 06 May 2024 09:39:50 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3568 Fix budgeting constraint test by enabling preset factor
Ashwani Raina has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21360 Change subject: KUDU-3568 Fix budgeting constraint test by enabling preset factor .. KUDU-3568 Fix budgeting constraint test by enabling preset factor In the failing test TestRowSetCompactionSkipWithBudgetingConstraints the rowset compaction went ahead even with high memory budget requirement (at least as per test expectations). The test bases it memory requirements on number of factor, one of which is rowset_compaction_delta_memory_factor. It is set to a very high value so that node on which test is running is not able to satisfy the memory requirements and hence skipping of compaction becomes necessary. Failure can happen due to a bug in the test where we are using rowset_compaction_delta_memory_factor without setting rowset_compaction_enforce_preset_factor to true. Since rowset_compaction_enforce_preset_factor is set to false by default, which means compaction policy relies on metrics (instead of memory factor) to come up with memory budgeting numbers. The test passes in most runs, because the metrics, (i.e. compact_rs_mem_usage_to_deltas_size_ratio) that compaction policy relies on, happens to be 0. Due to this, memory budgeting logic chooses the rowset_compaction_delta_memory_factor (set to a very high value in this test) as the factor and since the node doesn't have so much memory, it skips compaction as expected. In a nutshell, rowset_compaction_delta_memory_factor is used when when rowset_compaction_enforce_preset_factor is true. If preset is set to false, test will rely on metrics instead of memory factor defined by rowset_compaction_delta_memory_factor. Change-Id: I9df218cd2d8ef3709793db267d5a0d651421dbb6 --- M src/kudu/tablet/compaction-test.cc 1 file changed, 2 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/21360/1 -- To view, visit http://gerrit.cloudera.org:8080/21360 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I9df218cd2d8ef3709793db267d5a0d651421dbb6 Gerrit-Change-Number: 21360 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina
[kudu-CR] Update build pattern for fetching flaky tests list
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21327 ) Change subject: Update build pattern for fetching flaky tests list .. Patch Set 5: Code-Review+1 Thank you for addressing the all the comments. -- To view, visit http://gerrit.cloudera.org:8080/21327 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I317a3a32c06c06306b97566f954e0ffd508ce01f Gerrit-Change-Number: 21327 Gerrit-PatchSet: 5 Gerrit-Owner: Marton Greber Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Zoltan Chovan Gerrit-Comment-Date: Fri, 19 Apr 2024 13:59:41 + Gerrit-HasComments: No
[kudu-CR] Update build pattern for fetching flaky test list
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21327 ) Change subject: Update build pattern for fetching flaky test list .. Patch Set 4: (2 comments) Overall looks good to me. Just a couple of minor comments. http://gerrit.cloudera.org:8080/#/c/21327/4/build-support/jenkins/build-and-test.sh File build-support/jenkins/build-and-test.sh: http://gerrit.cloudera.org:8080/#/c/21327/4/build-support/jenkins/build-and-test.sh@42 PS4, Line 42: kudu-test nit: Change here as well? http://gerrit.cloudera.org:8080/#/c/21327/4/build-support/jenkins/build-and-test.sh@190 PS4, Line 190: jenkins For my understanding or maybe posterity, where was build_id prefix changed that caused the issue? -- To view, visit http://gerrit.cloudera.org:8080/21327 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I317a3a32c06c06306b97566f954e0ffd508ce01f Gerrit-Change-Number: 21327 Gerrit-PatchSet: 4 Gerrit-Owner: Marton Greber Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Zoltan Chovan Gerrit-Comment-Date: Thu, 18 Apr 2024 15:13:33 + Gerrit-HasComments: Yes
[kudu-CR] Update build pattern for fetching flaky test list
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21327 ) Change subject: Update build pattern for fetching flaky test list .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/21327/2/build-support/jenkins/build-and-test.sh File build-support/jenkins/build-and-test.sh: http://gerrit.cloudera.org:8080/#/c/21327/2/build-support/jenkins/build-and-test.sh@186 PS2, Line 186: becuase nit: because http://gerrit.cloudera.org:8080/#/c/21327/2/build-support/jenkins/build-and-test.sh@187 PS2, Line 187: as nit: As http://gerrit.cloudera.org:8080/#/c/21327/2/build-support/jenkins/build-and-test.sh@188 PS2, Line 188: any nit: Any -- To view, visit http://gerrit.cloudera.org:8080/21327 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I317a3a32c06c06306b97566f954e0ffd508ce01f Gerrit-Change-Number: 21327 Gerrit-PatchSet: 2 Gerrit-Owner: Marton Greber Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Chovan Gerrit-Comment-Date: Thu, 18 Apr 2024 10:21:11 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3371 Fix the crash bug when run binaries on older CPU machines
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21287 ) Change subject: KUDU-3371 Fix the crash bug when run binaries on older CPU machines .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/21287/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21287/1//COMMIT_MSG@7 PS1, Line 7: KUDU-3371 Use a dedicated jira ID that addresses this specific crash. KUDU-3371 is a feature jira that doesn't talk about this specific crash in particular. Please add some more details on rationale behind the chosen fix. -- To view, visit http://gerrit.cloudera.org:8080/21287 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id30ae995c41a592fccbdb822bc1f457c5e6878ac Gerrit-Change-Number: 21287 Gerrit-PatchSet: 1 Gerrit-Owner: Yingchun Lai Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Tue, 16 Apr 2024 11:34:35 + Gerrit-HasComments: Yes
[kudu-CR] [compaction] Code cleanup and readability improvement
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21098 ) Change subject: [compaction] Code cleanup and readability improvement .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/21098/3/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: http://gerrit.cloudera.org:8080/#/c/21098/3/src/kudu/tablet/compaction.cc@993 PS3, Line 993: const CompactionInputRow old_row > Should this be passed as const reference instead? Good catch! -- To view, visit http://gerrit.cloudera.org:8080/21098 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I54709b5e27751581c889854911323fbddab1c4ab Gerrit-Change-Number: 21098 Gerrit-PatchSet: 3 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Marton Greber Gerrit-Comment-Date: Thu, 04 Apr 2024 12:01:45 + Gerrit-HasComments: Yes
[kudu-CR] [compaction] Code cleanup and readability improvement
Hello Mahesh Reddy, Marton Greber, Alexey Serbin, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21098 to look at the new patch set (#4). Change subject: [compaction] Code cleanup and readability improvement .. [compaction] Code cleanup and readability improvement This is a base patch that does not change any functionality. Goal is to break the compaction memory usage improvement change into small ones to make it easy to review. Change-Id: I54709b5e27751581c889854911323fbddab1c4ab --- M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_compaction.cc 3 files changed, 178 insertions(+), 124 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/21098/4 -- To view, visit http://gerrit.cloudera.org:8080/21098 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I54709b5e27751581c889854911323fbddab1c4ab Gerrit-Change-Number: 21098 Gerrit-PatchSet: 4 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Marton Greber
[kudu-CR] [compaction] Code cleanup and readability improvement
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21098 ) Change subject: [compaction] Code cleanup and readability improvement .. Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/21098/2/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: http://gerrit.cloudera.org:8080/#/c/21098/2/src/kudu/tablet/compaction.cc@996 PS2, Line 996: Mutation** new_undo_head) { > While you are this, could you move this parameter to come before 'out' para Done http://gerrit.cloudera.org:8080/#/c/21098/2/src/kudu/tablet/compaction.cc@1182 PS2, Line 1182: bool RemoveAncientUndos(const HistoryGcOpts& history_gc_opts, : const Mutation* redo_head, : Mutation** undo_head) { : if (!history_gc_opts.gc_enabled()) { > While you are at this, maybe modify the signature of this function to retur Done http://gerrit.cloudera.org:8080/#/c/21098/2/src/kudu/tablet/compaction.cc@1235 PS2, Line 1235: > While you are at this, could you move this parameter to come before the out Done http://gerrit.cloudera.org:8080/#/c/21098/2/src/kudu/tablet/compaction.cc@1403 PS2, Line 1403: > Why not to check for the return status of this call? Done http://gerrit.cloudera.org:8080/#/c/21098/2/src/kudu/tablet/compaction.cc@1408 PS2, Line 1408: > ditto Done http://gerrit.cloudera.org:8080/#/c/21098/2/src/kudu/tablet/compaction.cc@1411 PS2, Line 1411: : DVLOG(4) << "Output Row: " << dst_row->schema()->DebugRow(*dst_row) :<< "; RowId: " << index_in_current_drs; : > Why not to move this to the upper level, and avoid passing an extra out par Done http://gerrit.cloudera.org:8080/#/c/21098/2/src/kudu/tablet/compaction.cc@1421 PS2, Line 1421: u = new_undos_head; > Maybe, move the whole definition of UndoListSanityCheck() along with the pl Done http://gerrit.cloudera.org:8080/#/c/21098/2/src/kudu/tablet/compaction.cc@1451 PS2, Line 1451: > Could this be passed as 'const CompactionInputRow&' instead? Done http://gerrit.cloudera.org:8080/#/c/21098/2/src/kudu/tablet/compaction.cc@1452 PS2, Line 1452: Are > Why not 'size_t'? It seems the involved functions under the hood actually Done http://gerrit.cloudera.org:8080/#/c/21098/2/src/kudu/tablet/compaction.cc@1452 PS2, Line 1452: a* aren > nit: cur_row_idx ? Done http://gerrit.cloudera.org:8080/#/c/21098/2/src/kudu/tablet/compaction.cc@1543 PS2, Line 1543: > Could this become Done -- To view, visit http://gerrit.cloudera.org:8080/21098 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I54709b5e27751581c889854911323fbddab1c4ab Gerrit-Change-Number: 21098 Gerrit-PatchSet: 3 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Marton Greber Gerrit-Comment-Date: Wed, 03 Apr 2024 16:42:02 + Gerrit-HasComments: Yes
[kudu-CR] [compaction] Code cleanup and readability improvement
Hello Mahesh Reddy, Marton Greber, Alexey Serbin, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21098 to look at the new patch set (#3). Change subject: [compaction] Code cleanup and readability improvement .. [compaction] Code cleanup and readability improvement This is a base patch that does not change any functionality. Goal is to break the compaction memory usage improvement change into small ones to make it easy to review. Change-Id: I54709b5e27751581c889854911323fbddab1c4ab --- M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_compaction.cc 3 files changed, 178 insertions(+), 124 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/21098/3 -- To view, visit http://gerrit.cloudera.org:8080/21098 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I54709b5e27751581c889854911323fbddab1c4ab Gerrit-Change-Number: 21098 Gerrit-PatchSet: 3 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Marton Greber
[kudu-CR] [ARM] Concurrent binary tree memory barriers fixed.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21127 ) Change subject: [ARM] Concurrent binary tree memory barriers fixed. .. Patch Set 6: Do you think a performance analysis is required to ensure there is no significant regression? -- To view, visit http://gerrit.cloudera.org:8080/21127 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie7c02dc3b9444599ef680fa9b29ae7a4d39dd382 Gerrit-Change-Number: 21127 Gerrit-PatchSet: 6 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Anonymous Coward (763) Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Chovan Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Fri, 29 Mar 2024 14:35:57 + Gerrit-HasComments: No
[kudu-CR] [ARM] Concurrent binary tree memory barriers fixed.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21127 ) Change subject: [ARM] Concurrent binary tree memory barriers fixed. .. Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/21127/6/src/kudu/tablet/concurrent_btree.h File src/kudu/tablet/concurrent_btree.h: http://gerrit.cloudera.org:8080/#/c/21127/6/src/kudu/tablet/concurrent_btree.h@139 PS6, Line 139: * nit: add space http://gerrit.cloudera.org:8080/#/c/21127/6/src/kudu/tablet/concurrent_btree.h@378 PS6, Line 378: StableVersionAcquire Just for my understanding, what is the difference between StableVersionAcquire and VersionAcquire? -- To view, visit http://gerrit.cloudera.org:8080/21127 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie7c02dc3b9444599ef680fa9b29ae7a4d39dd382 Gerrit-Change-Number: 21127 Gerrit-PatchSet: 6 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Anonymous Coward (763) Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Chovan Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Fri, 29 Mar 2024 13:47:34 + Gerrit-HasComments: Yes
[kudu-CR] [compaction] Code cleanup and readability improvement
Hello Mahesh Reddy, Marton Greber, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21098 to look at the new patch set (#2). Change subject: [compaction] Code cleanup and readability improvement .. [compaction] Code cleanup and readability improvement This is a base patch that does not change any functionality. Goal is to break the compaction memory usage improvement change into small ones to make it easy to review. Change-Id: I54709b5e27751581c889854911323fbddab1c4ab --- M src/kudu/tablet/compaction.cc 1 file changed, 140 insertions(+), 88 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/21098/2 -- To view, visit http://gerrit.cloudera.org:8080/21098 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I54709b5e27751581c889854911323fbddab1c4ab Gerrit-Change-Number: 21098 Gerrit-PatchSet: 2 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Marton Greber
[kudu-CR] [compaction] Code cleanup and readability improvement
Ashwani Raina has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21098 Change subject: [compaction] Code cleanup and readability improvement .. [compaction] Code cleanup and readability improvement This patch does not change any functionality. Goal is to break the compaction memory usage improvement change into small ones to make it easy to review. Change-Id: I54709b5e27751581c889854911323fbddab1c4ab --- M src/kudu/tablet/compaction.cc 1 file changed, 140 insertions(+), 88 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/21098/1 -- To view, visit http://gerrit.cloudera.org:8080/21098 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I54709b5e27751581c889854911323fbddab1c4ab Gerrit-Change-Number: 21098 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina
[kudu-CR] [ranger] KUDU-3558 Error out early when keytab file flag is empty
Ashwani Raina has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21097 Change subject: [ranger] KUDU-3558 Error out early when keytab file flag is empty .. [ranger] KUDU-3558 Error out early when keytab file flag is empty This patch enables ranger client to catch a missing keytab file scenario early on before even starting the Ranger subprocess. Upon detection, log the error message and avoid spawning the Ranger subprocess. This helps in debugging of scenario when Kerberos is disabled and Ranger subprocess is started without doing prechecks only to find out in later stage during Ranger plugin init. The patch also modifies two existing tests by adding dummy keytab file path to make it pass as before. Also, add a test to empty keytab file scenario. Change-Id: Iaf4df18f91a479f5d1ce4d959bd2dbb5e395eb1b --- M src/kudu/ranger/ranger_client-test.cc M src/kudu/ranger/ranger_client.cc 2 files changed, 14 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/21097/1 -- To view, visit http://gerrit.cloudera.org:8080/21097 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Iaf4df18f91a479f5d1ce4d959bd2dbb5e395eb1b Gerrit-Change-Number: 21097 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina
[kudu-CR] KUDU-3527 Fix block manager test when using 64k container block alignment
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20725 ) Change subject: KUDU-3527 Fix block manager test when using 64k container block alignment .. Patch Set 15: Code-Review+1 -- 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: 15 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: Mon, 29 Jan 2024 16:30:28 + Gerrit-HasComments: No
[kudu-CR] KUDU-3527 Fix block manager test when using 64k container block alignment
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20725 ) Change subject: KUDU-3527 Fix block manager test when using 64k container block alignment .. Patch Set 9: (10 comments) Overall looks good to me. Just a few nits. http://gerrit.cloudera.org:8080/#/c/20725/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20725/9//COMMIT_MSG@53 PS9, Line 53: ARM Not related to this change. Just for my understanding. On ARM, what is the out-of-the-box underlying filesystem type? Do we end up using LBM or File block manager on ARM? http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc@933 PS9, Line 933: Bolck nit: Block http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc@935 PS9, Line 935: because it can't handle a .data file Maybe we can write: because it can't handle a .data file whose corresponding .metadata file is not present. http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc@939 PS9, Line 939: smarted delete Not sure what "smarted delete" means. If you are pointing to approach of "first moving file to an intermediate/hidden state and then delete", then you may want to mention that in detailed words here, for better understanding. Or you can simply mention that comment needs to be rephrased/revisited after KUDU-3528 is fixed. http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc@959 PS9, Line 959: auto create_a_block = [&](BlockId* out, const string& test_data) { : unique_ptr block; : RETURN_NOT_OK(this->bm_->CreateBlock(this->test_block_opts_, &block)); : RETURN_NOT_OK(block->Append(test_data)); : RETURN_NOT_OK(block->Finalize()); : RETURN_NOT_OK(block->Close()); : *out = block->id(); : return Status::OK(); : }; Do you think it makes sense to move this into a class method that can be called from different tests? http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc@982 PS9, Line 982: container nit: containers http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc@982 PS9, Line 982: of remove http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc@983 PS9, Line 983: with is http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc@984 PS9, Line 984: chance remove http://gerrit.cloudera.org:8080/#/c/20725/9/src/kudu/fs/block_manager-test.cc@984 PS9, Line 984: 0.9994174 Add a one-liner to show how you arrived with this calculation. Haven't verified it, but maybe something like this: [1 - (0.75)^250] ~ 0.9994174 -- 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: 9 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: Thu, 25 Jan 2024 13:26:54 + Gerrit-HasComments: Yes
[kudu-CR] Fix CheckHolePunch for bigger than 4k blocks.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20894 ) Change subject: Fix CheckHolePunch for bigger than 4k blocks. .. Patch Set 6: Code-Review+1 (1 comment) Thank you for the change. 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: > PreAllocate always calls allocate, you are right. If you call it with non-w Thank you for the detailed summary. This is good enough detail for posterity. -- 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: 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 Gerrit-Comment-Date: Mon, 22 Jan 2024 07:08:46 + Gerrit-HasComments: Yes
[kudu-CR] Fix CheckHolePunch for bigger than 4k blocks.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20894 ) Change subject: Fix CheckHolePunch for bigger than 4k blocks. .. Patch Set 2: (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: > We use ioctl in case of xfs. (see Status PunchHole in env_posix.cc) Maybe I am missing something here. When PreAllocate (at line 92) is called, my understanding is that it calls fallocate irrespective of filesystem type. So, in your case, maybe you are getting non-zero error inside PreAllocate (which eventually returns Status::OK() anyway for EOPNOTSUPP and ENOSYS errnos). So, it proceeds further with checking of pre-punch file size and fails as expected. I am wondering when you set the kFileSize as multiple of block size on filesystem (i.e. 64K) instead of multiples of hard-coded 4K, does fallocate succeed in PreAllocate? On a side note, I was trying to run fallocate (on ext4) for non-aligned offset and length, it seemed to work. This is for the case when FALLOC_FL_KEEP_SIZE mode is set. Just trying to understand whether there is any difference in behaviour. -- 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: 2 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Thu, 18 Jan 2024 10:54:54 + 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] [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 identica
[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)
[kudu-CR] Fix CheckHolePunch for bigger than 4k blocks.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20894 ) Change subject: Fix CheckHolePunch for bigger than 4k blocks. .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/20894/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20894/2//COMMIT_MSG@12 PS2, Line 12: ContainerPreallocationTest Maybe you could add more details on what fails in this test if underlying filesystem has 64K block size. 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: Just wondering, if we keep offset at 4K which is unaligned for a 64K file-system, what is the current behaviour? Does fallocate fail for unaligned offset and length? -- 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: 2 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Comment-Date: Tue, 16 Jan 2024 10:04:34 + Gerrit-HasComments: Yes
[kudu-CR] [compaction/flush] Cleanup of compaction and flush code paths
Hello Mahesh Reddy, Alexey Serbin, Yingchun Lai, Attila Bukor, Yifan Zhang, Kudu Jenkins, Abhishek Chennaka, Ádám Bakai, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20720 to look at the new patch set (#8). Change subject: [compaction/flush] Cleanup of compaction and flush code paths .. [compaction/flush] Cleanup of compaction and flush code paths The change tries to add more comments, rename methods for better code readability and understanding. It mainly covers two codepaths: Memrowset flush and RowsetCompaction. Change-Id: Ic1c2f20230aa089baf34f418a59b6c97f7351217 --- M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.cc M src/kudu/tablet/rowset.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h 12 files changed, 207 insertions(+), 155 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/20720/8 -- To view, visit http://gerrit.cloudera.org:8080/20720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic1c2f20230aa089baf34f418a59b6c97f7351217 Gerrit-Change-Number: 20720 Gerrit-PatchSet: 8 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Ádám Bakai
[kudu-CR] [compaction/flush] Cleanup of compaction and flush code paths
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20720 ) Change subject: [compaction/flush] Cleanup of compaction and flush code paths .. Patch Set 7: (3 comments) > (1 comment) > > > > Patch Set 7: Code-Review+1 > > > > > > LGTM, looks like there's some overlap between this patch and > > Adam's patch: https://gerrit.cloudera.org/c/20787/. > > > > It doesn't show a merge conflict, so it should be safe to merge, > > right? > > IIUC, there isn't a conflict, but it's some logical duplication -- > if merging both patches, it's would be necessary to converge on the > way to get estimation for the total size of UNDO/REDO deltas. To > me, Adam's approach looks a bit cleaner. > > But indeed: I think that converging on that could be done in a > separate follow-up changelist. Agreed. At this point, my patch seems closer to being merged. I am ok with sending a follow-up patch (after Adam's patch is merged) that uses Adam's method instead. http://gerrit.cloudera.org:8080/#/c/20720/7/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: http://gerrit.cloudera.org:8080/#/c/20720/7/src/kudu/tablet/compaction.cc@1468 PS7, Line 1468: // - Append UNDO deltas to DiskRowSetWriter output > nit for here and below: please keep the style of the comments consistent th Done http://gerrit.cloudera.org:8080/#/c/20720/5/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: http://gerrit.cloudera.org:8080/#/c/20720/5/src/kudu/tablet/tablet.h@706 PS5, Line 706: & > nit: per current style guideline, the ampersand should stick to the type, n Done http://gerrit.cloudera.org:8080/#/c/20720/7/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/20720/7/src/kudu/tablet/tablet.cc@2004 PS7, Line 2004: DiskRowSetSpace drss; > If my patch gets merged, OnDiskDeltaSize can be used instead of this soluti Yes, fine by me. -- To view, visit http://gerrit.cloudera.org:8080/20720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic1c2f20230aa089baf34f418a59b6c97f7351217 Gerrit-Change-Number: 20720 Gerrit-PatchSet: 7 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Tue, 09 Jan 2024 11:59:17 + Gerrit-HasComments: Yes
[kudu-CR] [compaction/test] Add tests to generate heavy rowset compaction
Hello Tidy Bot, 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 (#6). 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 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 10MB 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/6 -- 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: 6 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [compaction] Add memory estimation unit test
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20787 ) Change subject: [compaction] Add memory estimation unit test .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/20787/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20787/6//COMMIT_MSG@9 PS6, Line 9: FLAGS_rowset_compaction_memory_estimate_enabled was always set to : false during testing, so this part of the code was not executed in : testing environment > Indeed! I recall that when I was working on 1556a353e, I verified the code Hi Alexey, Adam, Just a side note - while the test added here does go through the memory budgeting estimation code added in 1556a353e, it relies on MockDiskRowSet and its size that is set by the test to ensure it is more than the budget. That does the trick and test out the budgeting checks with mock rowsets. Additionally, I have added unit tests to test out the same budgeting checks with actual workload that generates deltas with specified memory requirements for compaction. You can check it out here: https://gerrit.cloudera.org/#/c/20816/ -- To view, visit http://gerrit.cloudera.org:8080/20787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb Gerrit-Change-Number: 20787 Gerrit-PatchSet: 6 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Wed, 03 Jan 2024 06:23:50 + Gerrit-HasComments: Yes
[kudu-CR] [compaction/test] Add tests to generate heavy rowset compaction
Hello Tidy Bot, 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 (#5). 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 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 10MB 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/5 -- 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: 5 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [compaction/test] Add tests to generate heavy rowset compaction
Hello Tidy Bot, 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 (#4). 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 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 10MB 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, 169 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/20816/4 -- 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: 4 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [compaction/test] Add tests to generate heavy rowset compaction
Hello 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 (#3). 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 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 10MB 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, 169 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/20816/3 -- 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: 3 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [WIP] [compaction/test] Add tests to generate heavy rowset compaction
Hello 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 (#2). Change subject: [WIP] [compaction/test] Add tests to generate heavy rowset compaction .. [WIP] [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. Most of the comments from above gerrit review have been addressed here. Pending items include: 1. Check if there is a better file location of these tests. 2. Ensure all comments from old patch are addressed. 3. Verify peak memory usage of compaction. Change-Id: I1996558e71c49314c6acf12faf854c796548318c --- M src/kudu/tablet/tablet_history_gc-test.cc 1 file changed, 121 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/20816/2 -- 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: 2 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [WIP] [compaction/test] Add tests to generate heavy rowset compaction
Ashwani Raina has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20816 Change subject: [WIP] [compaction/test] Add tests to generate heavy rowset compaction .. [WIP] [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. Most of the comments from above gerrit review have been addressed here. Pending items include: 1. Check if there is a better file location of these tests. 2. Ensure all comments from old patch are addressed. 3. Verify peak memory usage of compaction. Change-Id: I1996558e71c49314c6acf12faf854c796548318c --- M src/kudu/tablet/tablet_history_gc-test.cc 1 file changed, 118 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/20816/1 -- 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: newchange Gerrit-Change-Id: I1996558e71c49314c6acf12faf854c796548318c Gerrit-Change-Number: 20816 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina
[kudu-CR] [compaction] Add memory estimation unit test
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20787 ) Change subject: [compaction] Add memory estimation unit test .. Patch Set 5: Btw, I have a patch going out for review, shortly, that does something similar by adding two unit tests to exercise the rowset compaction. The difference is that here we are setting delta size programatically along with other parameters. The patch I am working on has two unit tests with and without budget restrictions and actual creation of deltas using update workload. Feel free to take a look and provide comments. -- To view, visit http://gerrit.cloudera.org:8080/20787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb Gerrit-Change-Number: 20787 Gerrit-PatchSet: 5 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Tue, 19 Dec 2023 12:08:02 + Gerrit-HasComments: No
[kudu-CR] [compaction] Add memory estimation unit test
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20787 ) Change subject: [compaction] Add memory estimation unit test .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/20787/5/src/kudu/tablet/compaction_policy.cc File src/kudu/tablet/compaction_policy.cc: http://gerrit.cloudera.org:8080/#/c/20787/5/src/kudu/tablet/compaction_policy.cc@174 PS5, Line 174: int64_t Could you also please take care of this? Change to uint64_t -- To view, visit http://gerrit.cloudera.org:8080/20787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb Gerrit-Change-Number: 20787 Gerrit-PatchSet: 5 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Tue, 19 Dec 2023 10:38:42 + Gerrit-HasComments: Yes
[kudu-CR] [compaction/flush] Cleanup of compaction and flush code paths
Hello Mahesh Reddy, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Abhishek Chennaka, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20720 to look at the new patch set (#7). Change subject: [compaction/flush] Cleanup of compaction and flush code paths .. [compaction/flush] Cleanup of compaction and flush code paths The change tries to add more comments, rename methods for better code readability and understanding. It mainly covers two codepaths: Memrowset flush and RowsetCompaction. Change-Id: Ic1c2f20230aa089baf34f418a59b6c97f7351217 --- M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.cc M src/kudu/tablet/rowset.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h 12 files changed, 207 insertions(+), 155 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/20720/7 -- To view, visit http://gerrit.cloudera.org:8080/20720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic1c2f20230aa089baf34f418a59b6c97f7351217 Gerrit-Change-Number: 20720 Gerrit-PatchSet: 7 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Yingchun Lai
[kudu-CR] [compaction] Add memory estimation unit test
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20787 ) Change subject: [compaction] Add memory estimation unit test .. Patch Set 1: (2 comments) Overall looks good to me. Just one small comment. http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy-test.cc File src/kudu/tablet/compaction_policy-test.cc: http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy-test.cc@647 PS1, Line 647: TestMemoryEstimation You could also add an ASSERT_STR_CONTAINS/ASSERT_STR_NOT_CONTAINS at the end of both conditions to verify that this rowset was/was not picked because of memory budget constraints by matching with this pattern -> "removed from compaction input due to memory constraints" In case you do that, keep in mind that this error log is throttled. So you might see this error just for the first rowset. http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy-test.cc@649 PS1, Line 649: FLAGS_rowset_compaction_memory_estimate_enabled > The idea was that what if it gets enabled by default later? Then the test w Never mind! That should be ok. -- To view, visit http://gerrit.cloudera.org:8080/20787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb Gerrit-Change-Number: 20787 Gerrit-PatchSet: 1 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Tue, 19 Dec 2023 09:32:24 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3534 [compaction] Log timestamp of two matching DELETE REDO mutations.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, Ádám Bakai, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20792 to look at the new patch set (#3). Change subject: KUDU-3534 [compaction] Log timestamp of two matching DELETE REDO mutations. .. KUDU-3534 [compaction] Log timestamp of two matching DELETE REDO mutations. This patch just adds an info message to log timestamp value in case two REDO mutations of type DELETE are found to be stamped with same time. This is an undesired condition that could possibly happen due to corruption of mutation entries. The value will give us an idea whether it is 0, garbled or close to some valid timestamp. Change-Id: I508254a83046818b81db4577bf07265b46a13c9a --- M src/kudu/tablet/compaction.cc 1 file changed, 6 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/20792/3 -- To view, visit http://gerrit.cloudera.org:8080/20792 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I508254a83046818b81db4577bf07265b46a13c9a Gerrit-Change-Number: 20792 Gerrit-PatchSet: 3 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai
[kudu-CR] KUDU-3534 [compaction] Log timestamp of two matching DELETE REDO mutations.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20792 ) Change subject: KUDU-3534 [compaction] Log timestamp of two matching DELETE REDO mutations. .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/20792/2/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: http://gerrit.cloudera.org:8080/#/c/20792/2/src/kudu/tablet/compaction.cc@483 PS2, Line 483: left_last->timestamp().ToString(), > Printing the timestamp is redundant. I checked StringifyMutationList output Good catch about redo_head! Printing timestamp was kind of deliberate to make it easy to spot the conflicting timestamp. Removed, anyway it should be easy to spot it. -- To view, visit http://gerrit.cloudera.org:8080/20792 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I508254a83046818b81db4577bf07265b46a13c9a Gerrit-Change-Number: 20792 Gerrit-PatchSet: 2 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Mon, 18 Dec 2023 19:35:10 + Gerrit-HasComments: Yes
[kudu-CR] [rpc] increase listened socket backlog up to 512
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20797 ) Change subject: [rpc] increase listened socket backlog up to 512 .. Patch Set 2: (1 comment) Overall looks good to me. Just one question. http://gerrit.cloudera.org:8080/#/c/20797/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20797/2//COMMIT_MSG@7 PS2, Line 7: increase listened socket backlog up to 512 Do you think we may also increase number of acceptor threads from default value 1? -- To view, visit http://gerrit.cloudera.org:8080/20797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6f5791acad6ea0787e23d4c71ab2a7ac4c8c1f2 Gerrit-Change-Number: 20797 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Fri, 15 Dec 2023 15:01:00 + Gerrit-HasComments: Yes
[kudu-CR] [Tool] Find file path where the block is stored on
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20594 ) Change subject: [Tool] Find file path where the block is stored on .. Patch Set 4: (12 comments) http://gerrit.cloudera.org:8080/#/c/20594/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20594/4//COMMIT_MSG@7 PS4, Line 7: stored on located http://gerrit.cloudera.org:8080/#/c/20594/4//COMMIT_MSG@10 PS4, Line 10: log only prints Can we also modify existing log to make sure it prints container id as well? http://gerrit.cloudera.org:8080/#/c/20594/4//COMMIT_MSG@12 PS4, Line 12: If we known the container, we can reads nit: If we know the container id, we can read http://gerrit.cloudera.org:8080/#/c/20594/4//COMMIT_MSG@11 PS4, Line 11: We want to known which container the block locates : in nit: We want to know the container where block is located. http://gerrit.cloudera.org:8080/#/c/20594/4//COMMIT_MSG@14 PS4, Line 14: meadata metadata http://gerrit.cloudera.org:8080/#/c/20594/4/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: http://gerrit.cloudera.org:8080/#/c/20594/4/src/kudu/fs/block_manager.h@261 PS4, Line 261: block locates in nit: block is located http://gerrit.cloudera.org:8080/#/c/20594/4/src/kudu/fs/block_manager.h@264 PS4, Line 264: stored on. nit: stored. http://gerrit.cloudera.org:8080/#/c/20594/4/src/kudu/fs/file_block_manager.h File src/kudu/fs/file_block_manager.h: http://gerrit.cloudera.org:8080/#/c/20594/4/src/kudu/fs/file_block_manager.h@a124 PS4, Line 124: : : Are these comments not applicable anymore? http://gerrit.cloudera.org:8080/#/c/20594/4/src/kudu/fs/log_block_manager.h File src/kudu/fs/log_block_manager.h: http://gerrit.cloudera.org:8080/#/c/20594/4/src/kudu/fs/log_block_manager.h@194 PS4, Line 194: bool FindBlockPath Add one liner comment about what this function does - either here or cc file. http://gerrit.cloudera.org:8080/#/c/20594/4/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/20594/4/src/kudu/tools/kudu-tool-test.cc@9254 PS4, Line 9254: existes exists http://gerrit.cloudera.org:8080/#/c/20594/4/src/kudu/tools/kudu-tool-test.cc@9269 PS4, Line 9269: block_ids[0].ToString() For better clarity, can we have "not found" sort of error message here instead of relying on non-existence of block id? http://gerrit.cloudera.org:8080/#/c/20594/4/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: http://gerrit.cloudera.org:8080/#/c/20594/4/src/kudu/tools/tool_action_local_replica.cc@1230 PS4, Line 1230: bid_str nit: use "blkid_str" instead for better readability. Similarly, "blkid" at other places. -- To view, visit http://gerrit.cloudera.org:8080/20594 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifaafd2bb634ed7fab923d9cf9eef40437442d187 Gerrit-Change-Number: 20594 Gerrit-PatchSet: 4 Gerrit-Owner: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Fri, 15 Dec 2023 14:07:23 + Gerrit-HasComments: Yes
[kudu-CR] [compaction/flush] Cleanup of compaction and flush code paths
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20720 ) Change subject: [compaction/flush] Cleanup of compaction and flush code paths .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/20720/4/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/20720/4/src/kudu/tablet/tablet.cc@2061 PS4, Line 2061: entries > nit: Add a dot at the end. Other places are the same. Done -- To view, visit http://gerrit.cloudera.org:8080/20720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic1c2f20230aa089baf34f418a59b6c97f7351217 Gerrit-Change-Number: 20720 Gerrit-PatchSet: 4 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Fri, 15 Dec 2023 10:52:29 + Gerrit-HasComments: Yes
[kudu-CR] [compaction/flush] Cleanup of compaction and flush code paths
Hello Mahesh Reddy, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Abhishek Chennaka, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20720 to look at the new patch set (#5). Change subject: [compaction/flush] Cleanup of compaction and flush code paths .. [compaction/flush] Cleanup of compaction and flush code paths The change tries to add more comments, rename methods for better code readability and understanding. It mainly covers two codepaths: Memrowset flush and RowsetCompaction. Change-Id: Ic1c2f20230aa089baf34f418a59b6c97f7351217 --- M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.cc M src/kudu/tablet/rowset.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h 12 files changed, 207 insertions(+), 156 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/20720/5 -- To view, visit http://gerrit.cloudera.org:8080/20720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic1c2f20230aa089baf34f418a59b6c97f7351217 Gerrit-Change-Number: 20720 Gerrit-PatchSet: 5 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Yingchun Lai
[kudu-CR] [compaction/flush] Cleanup of compaction and flush code paths
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20720 ) Change subject: [compaction/flush] Cleanup of compaction and flush code paths .. Patch Set 4: (3 comments) Thanks for the review. http://gerrit.cloudera.org:8080/#/c/20720/4/src/kudu/tablet/compaction.h File src/kudu/tablet/compaction.h: http://gerrit.cloudera.org:8080/#/c/20720/4/src/kudu/tablet/compaction.h@118 PS4, Line 118: const > nit: alignment Done http://gerrit.cloudera.org:8080/#/c/20720/4/src/kudu/tablet/compaction.h@121 PS4, Line 121: compaction > Also update the comments. Done http://gerrit.cloudera.org:8080/#/c/20720/4/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: http://gerrit.cloudera.org:8080/#/c/20720/4/src/kudu/tablet/compaction.cc@1388 PS4, Line 1388: // Following method processes the compaction input by reading input rows in : // blocks and for each row inside the block: : // - Apply all REDO mutations collected for the row at hand : // - Generate corresponding UNDO deltas for applied mutations : // - For a row with 'ghost' entries, merge their histories of mutations : // - Remove any ancient UNDO mutations, as those are not applicable anymore : // - Append UNDO and REDO deltas to DiskRowSetWriter output : // - Append fully or partially (resized) processed rowblock to DRS writer output > Instead of writing comments at the head of function, I'd prefer to palce th Most of the code below has these comments. I have added one liner comments to the remaining major items below. It is good to have a summary (at top) of what this method does. That way it becomes easy to skim through the logic of this method quickly. -- To view, visit http://gerrit.cloudera.org:8080/20720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic1c2f20230aa089baf34f418a59b6c97f7351217 Gerrit-Change-Number: 20720 Gerrit-PatchSet: 4 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Fri, 15 Dec 2023 10:50:58 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3534 [compaction] Log timestamp of two matching DELETE REDO mutations.
Hello Alexey Serbin, Kudu Jenkins, Abhishek Chennaka, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20792 to look at the new patch set (#2). Change subject: KUDU-3534 [compaction] Log timestamp of two matching DELETE REDO mutations. .. KUDU-3534 [compaction] Log timestamp of two matching DELETE REDO mutations. This patch just adds an info message to log timestamp value in case two REDO mutations of type DELETE are found to be stamped with same time. This is an undesired condition that could possibly happen due to corruption of mutation entries. The value will give us an idea whether it is 0, garbled or close to some valid timestamp. Change-Id: I508254a83046818b81db4577bf07265b46a13c9a --- M src/kudu/tablet/compaction.cc 1 file changed, 7 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/20792/2 -- To view, visit http://gerrit.cloudera.org:8080/20792 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I508254a83046818b81db4577bf07265b46a13c9a Gerrit-Change-Number: 20792 Gerrit-PatchSet: 2 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [compaction] Log timestamp of two matching DELETE REDO mutations.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20792 ) Change subject: [compaction] Log timestamp of two matching DELETE REDO mutations. .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/20792/1/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: http://gerrit.cloudera.org:8080/#/c/20792/1/src/kudu/tablet/compaction.cc@479 PS1, Line 479: if (0 == ret) { : // Log the timestamp value, invalid value could mean potential corruption. : LOG(INFO) << Substitute("Both DELETE REDO mutations are stamped with same timestamp i.e. $0", : left_last->timestamp().ToString()); : } > I guess it's not safe to continue from here having the invariant broken. At first, I thought of using RowToString or StringifyMutationList to print timestamp and changelist for all mutations. It would probably be a futile exercise since one problematic entry vs many problematic entries would mostly require same workaround I guess. But there is no harm in printing those mutations. I am not sure how logging handles long list of mutations. It should probably be ok as I can see it being used at couple of places where multiple mutation entries are logged. -- To view, visit http://gerrit.cloudera.org:8080/20792 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I508254a83046818b81db4577bf07265b46a13c9a Gerrit-Change-Number: 20792 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Dec 2023 20:08:51 + Gerrit-HasComments: Yes
[kudu-CR] [compaction] Log timestamp of two matching DELETE REDO mutations.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20792 ) Change subject: [compaction] Log timestamp of two matching DELETE REDO mutations. .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/20792/1/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: http://gerrit.cloudera.org:8080/#/c/20792/1/src/kudu/tablet/compaction.cc@a479 PS1, Line 479: > Do we still want to have the CHECK leading to server crash and not have any I was under the impression that it just logs the failure and moves on. I guess it is better to keep the check and add log info to it. Something like this: ++ CHECK_NE(0, ret) << Substitute("Both DELETE REDO mutations are stamped with same timestamp i.e. $0", left_last->timestamp().ToString()); ++ -- To view, visit http://gerrit.cloudera.org:8080/20792 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I508254a83046818b81db4577bf07265b46a13c9a Gerrit-Change-Number: 20792 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 14 Dec 2023 19:00:23 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3523 Use f bsize as Kudu block size instead of st blksize
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20680 ) Change subject: KUDU-3523 Use f_bsize as Kudu block size instead of st_blksize .. Patch Set 4: > > > The reason behind 64K block size on XFS could be because the > > > filesystem was created with "-b size=64K" mkfs option. Could > you > > > confirm that? > > > > > > Also, in such cases, even though user is able to create a > > > filesystem with block size greater than general page size on > > linux > > > i.e. 4K, default mount API may not work because kernel is > > compiled > > > with 4K default page size and it doesn't allow a filesystem > with > > > more than 4K block size to be mounted. > > > This could be one of the possibilities. > > > > > > Could you please share the following information: > > > 1. Output of command -> "getconf PAGE_SIZE" on the node where > > xfs > > > filesystem is hosted > > > 2. mkfs command used to create xfs filesystem (i.e. /dev/vdb) > > > 3. Check if punch hole is supported on a filesystem with block > > size > > > more than 4K. > > > > Ashwani, > > You can easily set up the environment, just use the > > cloudera-systest-base-rhel-8.8-hvm-arm-grp image > (Virginia/public) > > in cloudcat. > > This will run a 64k rhel kernel on a filesystem created by "-b > > size=4k". > > (on public aws I can only select 4k kernel for arm rhel, so it is > > trickier there). > > Use "grep -ir pagesize /proc/self/smaps" to check kernel size. > > > > There is the following 4 combinations: > > > > Case1. 4k kernel with "-b size=4k" disk: Kudu works fine. > > > > Case2. 64k kernel with "-b size=4k" disk: This is the case that > > Wang Xixu is trying to fix. To my best understanding kudu kind of > > works without encryption (maybe does not preallocated properly, > or > > there are other hidden issues, but I could start a server and use > > it). Kudu breaks with encryption. > > > > Case3. 64k kernel with "-b size=64k" disk: Status > CheckHolePunch(…)” > > in dir_util.cc fails. Kudu is absolutely broken. > > > > Case4. 4k kernel with "-b size=64k": system refuses to mount the > > disk (mount(2) system call failed: Function not implemented.). > > > > Do we need to support 64k kernel? I guess we have to. But then do > > we also want to support Case3 (it would be logical)? > > Case 4 is simply not supported from filesystem itself for good > reasons. So, we don't need to worry about that. > Case 3: We need to find out what is failing there. If it is > underlying fs that is throwing error, we may not be able to fix it. > If it is Kudu, we can look into it. It is likely that this is a > straightforward fix. > Case 2: Does Kudu with encryption break due to the same issue being > fixed here by Wang Xixu or you hit some other error? > > To answer your question about supporting 64K kernel, I guess that > would depend on whether following conditions are met: > 1. Filesystems (ext4, xfs, etc) that Kudu uses, support hole > punching irrespective of whether block size is 4K or 64K. > 2. Filesystems (ext4, xfs, etc) that Kudu uses, support mount > operation with different mount flags. > 3. Filesystems (ext4, xfs, etc) that Kudu uses, support different > block sizes on 64K kernel. > > In a nutshell, Kudu most likely doesn't need to do anything to > support 64K kernel other than ensuring there is no restriction in > code to disallow configuration with 64K page sized kernel. BTW, Kudu requires hole punching if its using LBM which is the recommended setting because it is efficient and scalable as opposite to file block manager where hole punching is not required. -- To view, visit http://gerrit.cloudera.org:8080/20680 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I04343caf5fe377a4fa9b57e6e450307e6b995928 Gerrit-Change-Number: 20680 Gerrit-PatchSet: 4 Gerrit-Owner: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Thu, 14 Dec 2023 15:14:15 + Gerrit-HasComments: No
[kudu-CR] KUDU-3523 Use f bsize as Kudu block size instead of st blksize
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20680 ) Change subject: KUDU-3523 Use f_bsize as Kudu block size instead of st_blksize .. Patch Set 4: > > The reason behind 64K block size on XFS could be because the > > filesystem was created with "-b size=64K" mkfs option. Could you > > confirm that? > > > > Also, in such cases, even though user is able to create a > > filesystem with block size greater than general page size on > linux > > i.e. 4K, default mount API may not work because kernel is > compiled > > with 4K default page size and it doesn't allow a filesystem with > > more than 4K block size to be mounted. > > This could be one of the possibilities. > > > > Could you please share the following information: > > 1. Output of command -> "getconf PAGE_SIZE" on the node where > xfs > > filesystem is hosted > > 2. mkfs command used to create xfs filesystem (i.e. /dev/vdb) > > 3. Check if punch hole is supported on a filesystem with block > size > > more than 4K. > > Ashwani, > You can easily set up the environment, just use the > cloudera-systest-base-rhel-8.8-hvm-arm-grp image (Virginia/public) > in cloudcat. > This will run a 64k rhel kernel on a filesystem created by "-b > size=4k". > (on public aws I can only select 4k kernel for arm rhel, so it is > trickier there). > Use "grep -ir pagesize /proc/self/smaps" to check kernel size. > > There is the following 4 combinations: > > Case1. 4k kernel with "-b size=4k" disk: Kudu works fine. > > Case2. 64k kernel with "-b size=4k" disk: This is the case that > Wang Xixu is trying to fix. To my best understanding kudu kind of > works without encryption (maybe does not preallocated properly, or > there are other hidden issues, but I could start a server and use > it). Kudu breaks with encryption. > > Case3. 64k kernel with "-b size=64k" disk: Status CheckHolePunch(…)” > in dir_util.cc fails. Kudu is absolutely broken. > > Case4. 4k kernel with "-b size=64k": system refuses to mount the > disk (mount(2) system call failed: Function not implemented.). > > Do we need to support 64k kernel? I guess we have to. But then do > we also want to support Case3 (it would be logical)? Case 4 is simply not supported from filesystem itself for good reasons. So, we don't need to worry about that. Case 3: We need to find out what is failing there. If it is underlying fs that is throwing error, we may not be able to fix it. If it is Kudu, we can look into it. It is likely that this is a straightforward fix. Case 2: Does Kudu with encryption break due to the same issue being fixed here by Wang Xixu or you hit some other error? To answer your question about supporting 64K kernel, I guess that would depend on whether following conditions are met: 1. Filesystems (ext4, xfs, etc) that Kudu uses, support hole punching irrespective of whether block size is 4K or 64K. 2. Filesystems (ext4, xfs, etc) that Kudu uses, support mount operation with different mount flags. 3. Filesystems (ext4, xfs, etc) that Kudu uses, support different block sizes on 64K kernel. In a nutshell, Kudu most likely doesn't need to do anything to support 64K kernel other than ensuring there is no restriction in code to disallow configuration with 64K page sized kernel. -- To view, visit http://gerrit.cloudera.org:8080/20680 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I04343caf5fe377a4fa9b57e6e450307e6b995928 Gerrit-Change-Number: 20680 Gerrit-PatchSet: 4 Gerrit-Owner: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Thu, 14 Dec 2023 14:33:37 + Gerrit-HasComments: No
[kudu-CR] KUDU-3527 Fix block manager test on 64k filesystems
Ashwani Raina 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 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/20725/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20725/5//COMMIT_MSG@7 PS5, Line 7: Fix block manager test on 64k filesystems > For f_bsize=64k, I do the following. Just for the record and to make things simpler here, you could make use of following commands to create a loop device and run mkfs.xfs on it instead of adding an additional disk: (works on Ubuntu, check for equivalent flags/commands on other distributions) 1. "losetup -f" -> Gives next available unused loop device. 2. "dd if=/dev/zero of=/mnt/file bs=4096 count=262144" - create 1GB file 3. Use the loop device from the #1 output and attach to the file -> "losetup /dev/ /mnt/file" 4. Now, you can run mkfs.xfs > "mkfs.xfs -b size=65536 /dev/ On a side note, when you say 64K kernel, you mean 64K kernel page size - right? With that, you should be able to mount the filesystem with 64K block size. I had asked about hole punching question on https://gerrit.cloudera.org/#/c/20680/ but I guess it was not fully tried out. What is the error you got when you tried to start Kudu? Basically, I am interested in knowing whether it is our code or underlying kernel code that throws error. KUDU-3523, however, is using overlayfs (abstract layer like unionfs) where Kudu is up and running. It is worth exploring how come Kudu is able to start there and not in this environment. -- 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: 5 Gerrit-Owner: Zoltan Martonka 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: Thu, 14 Dec 2023 13:54:13 + Gerrit-HasComments: Yes
[kudu-CR] [compaction] Log timestamp of two matching DELETE REDO mutations.
Ashwani Raina has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20792 Change subject: [compaction] Log timestamp of two matching DELETE REDO mutations. .. [compaction] Log timestamp of two matching DELETE REDO mutations. This patch just adds an info message to log timestamp value in case two REDO mutations of type DELETE are found to be stamped with same time. This is an undesired condition that could possibly happen due to corruption of mutation entries. The value will give us an idea whether it is 0, garbled or close to some valid timestamp. Change-Id: I508254a83046818b81db4577bf07265b46a13c9a --- M src/kudu/tablet/compaction.cc 1 file changed, 5 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/20792/1 -- To view, visit http://gerrit.cloudera.org:8080/20792 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I508254a83046818b81db4577bf07265b46a13c9a Gerrit-Change-Number: 20792 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina
[kudu-CR] KUDU-3524 Fix crash when sending periodic keep-alive requests
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20739 ) Change subject: KUDU-3524 Fix crash when sending periodic keep-alive requests .. Patch Set 5: Code-Review+1 (1 comment) Changes look good to me. Just one nit pick about adding details on why failure is intermittent. http://gerrit.cloudera.org:8080/#/c/20739/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20739/2//COMMIT_MSG@10 PS2, Line 10: StartKeepAlivePeriodicall > Kudu client fails with error intermittently. Thanks for clarification. Have you added details on why failure is intermittent? -- To view, visit http://gerrit.cloudera.org:8080/20739 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I130db970a091cdf7689245a79dc4ea445d1f739f Gerrit-Change-Number: 20739 Gerrit-PatchSet: 5 Gerrit-Owner: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Thu, 14 Dec 2023 10:58:44 + Gerrit-HasComments: Yes
[kudu-CR] [compaction] Add memory estimation unit test
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20787 ) Change subject: [compaction] Add memory estimation unit test .. Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/20787/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20787/1//COMMIT_MSG@7 PS1, Line 7: Add memory estimation unit test Maybe add more details about how changing hard limit helps cover test scenario for memory budgeting logic. http://gerrit.cloudera.org:8080/#/c/20787/1//COMMIT_MSG@9 PS1, Line 9: FLAGS_rowset_compaction_estimate_min_deltas_size_mb Did you mean rowset_compaction_memory_estimate_enabled? rowset_compaction_estimate_min_deltas_size_mb is for minimum deltas size and initialised to 64MB default. http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy-test.cc File src/kudu/tablet/compaction_policy-test.cc: http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy-test.cc@649 PS1, Line 649: FLAGS_rowset_compaction_memory_estimate_enabled Isn't this disabled by default? No need to set it to false here unless it could be true here for some other reason. http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy.cc File src/kudu/tablet/compaction_policy.cc: http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy.cc@182 PS1, Line 182: { Move above to align with other instances. http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/rowset.cc File src/kudu/tablet/rowset.cc: http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/rowset.cc@266 PS1, Line 266: for (const shared_ptr &rs : new_rowsets_) OnDiskDeltaSize used in compaction is per rowset. Here it seems to be computing total size for all rowsets inside new_rowsets_. Where is it getting used? -- To view, visit http://gerrit.cloudera.org:8080/20787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb Gerrit-Change-Number: 20787 Gerrit-PatchSet: 1 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 13 Dec 2023 14:52:50 + Gerrit-HasComments: Yes
[kudu-CR] [compaction] Fix the incorrect memory budgeting condition
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20758 ) Change subject: [compaction] Fix the incorrect memory budgeting condition .. Patch Set 1: > The patch looks good. > However, I have a general comment about this part of the code: I > executed the whole diskrowset-test.cc and since > FLAGS_rowset_compaction_memory_estimate_enabled > is set to false during test, the code is never executed during > tests. I think thats the reason that it wasn't discovered during > testing. If I set FLAGS_rowset_compaction_memory_estimate_enabled > to true, then the function throws an error because it tries to > downcast MockDiskRowset to DiskRowset, so it's not straightforward. > I'm working on a patch that at introduces a new test where this > function is executed. Yes, that and other budgeting size conditions that are specific to certain scenarios like frequent upserts resulting in accumulation of UNDO deltas. Unless the test introduces such conditions, chances are that budgeting policy won't apply. I am not sure what error it is throwing in your case but I would be happy to discuss if you are stuck and need help understanding this part of code. -- To view, visit http://gerrit.cloudera.org:8080/20758 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8928b15750f100785c510ee8086e5a6281b3a7b8 Gerrit-Change-Number: 20758 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Fri, 08 Dec 2023 08:43:57 + Gerrit-HasComments: No
[kudu-CR] KUDU-3523 Use f bsize as Kudu block size instead of st blksize
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20680 ) Change subject: KUDU-3523 Use f_bsize as Kudu block size instead of st_blksize .. Patch Set 4: The reason behind 64K block size on XFS could be because the filesystem was created with "-b size=64K" mkfs option. Could you confirm that? Also, in such cases, even though user is able to create a filesystem with block size greater than general page size on linux i.e. 4K, default mount API may not work because kernel is compiled with 4K default page size and it doesn't allow a filesystem with more than 4K block size to be mounted. This could be one of the possibilities. Could you please share the following information: 1. Output of command -> "getconf PAGE_SIZE" on the node where xfs filesystem is hosted 2. mkfs command used to create xfs filesystem (i.e. /dev/vdb) 3. Check if punch hole is supported on a filesystem with block size more than 4K. -- To view, visit http://gerrit.cloudera.org:8080/20680 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I04343caf5fe377a4fa9b57e6e450307e6b995928 Gerrit-Change-Number: 20680 Gerrit-PatchSet: 4 Gerrit-Owner: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Thu, 07 Dec 2023 14:23:53 + Gerrit-HasComments: No
[kudu-CR] [compaction] Fix the incorrect memory budgeting condition
Ashwani Raina has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20758 Change subject: [compaction] Fix the incorrect memory budgeting condition .. [compaction] Fix the incorrect memory budgeting condition Compaction budgeting code has minimum size of on-disk deltas that is used to decide whether memory budgeting can be applied or not. While comparing the actual on-disk size of deltas with minimum value the fact that minimum value is in MBytes is not taken into account and is directly compared with on-disk size which is in bytes. The fix is to convert the min size in MBs to bytes first and then compare. Change-Id: I8928b15750f100785c510ee8086e5a6281b3a7b8 --- M src/kudu/tablet/compaction_policy.cc 1 file changed, 5 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/20758/1 -- To view, visit http://gerrit.cloudera.org:8080/20758 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I8928b15750f100785c510ee8086e5a6281b3a7b8 Gerrit-Change-Number: 20758 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina
[kudu-CR] KUDU-3527 Fix block manager test on 64k filesystems
Ashwani Raina 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 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/20725/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20725/5//COMMIT_MSG@7 PS5, Line 7: Fix block manager test on 64k filesystems Have you looked at this patch -> https://gerrit.cloudera.org/#/c/20680/ Take a look if you haven't, to see if there is any correlation. http://gerrit.cloudera.org:8080/#/c/20725/5//COMMIT_MSG@13 PS5, Line 13: chanche chance http://gerrit.cloudera.org:8080/#/c/20725/5//COMMIT_MSG@18 PS5, Line 18: altough although http://gerrit.cloudera.org:8080/#/c/20725/5//COMMIT_MSG@18 PS5, Line 18: chanche chance http://gerrit.cloudera.org:8080/#/c/20725/5//COMMIT_MSG@25 PS5, Line 25: sort short http://gerrit.cloudera.org:8080/#/c/20725/5/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/20725/5/src/kudu/fs/block_manager-test.cc@936 PS5, Line 936: FLAGS_log_container_max_size = 512 I am not quite clear on why we need to increase container max size. It would be great if you could add some more details here. -- 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: 5 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Thu, 30 Nov 2023 12:29:33 + Gerrit-HasComments: Yes
[kudu-CR] [arm64] Fix CountOnes() function
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20736 ) Change subject: [arm64] Fix CountOnes() function .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/20736/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20736/2//COMMIT_MSG@7 PS2, Line 7: Fix CountOnes() function Out of curiosity - did this issue manifest into some failure? http://gerrit.cloudera.org:8080/#/c/20736/2//COMMIT_MSG@10 PS2, Line 10: greather nit: greater http://gerrit.cloudera.org:8080/#/c/20736/2//COMMIT_MSG@11 PS2, Line 11: sufix nit: suffix -- To view, visit http://gerrit.cloudera.org:8080/20736 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia4b78b5c1df9547548fc7980cec4b84349e868bc Gerrit-Change-Number: 20736 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Wed, 29 Nov 2023 16:10:19 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3524 Fix core of sending keep-alive requests periodically
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20739 ) Change subject: KUDU-3524 Fix core of sending keep-alive requests periodically .. Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/20739/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20739/2//COMMIT_MSG@10 PS2, Line 10: in kudu client will core. Does kudu client fail with error intermittently? If yes, could you also add more details on why, if possible. http://gerrit.cloudera.org:8080/#/c/20739/2//COMMIT_MSG@13 PS2, Line 13: funciton nit: function http://gerrit.cloudera.org:8080/#/c/20739/2/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: http://gerrit.cloudera.org:8080/#/c/20739/2/src/kudu/client/scanner-internal.cc@125 PS2, Line 125: there nit: remove "there" http://gerrit.cloudera.org:8080/#/c/20739/2/src/kudu/client/scanner-internal.cc@128 PS2, Line 128: ScannerKeepAliveAsync Can you please point me to definition of ScannerKeepAliveAsync? -- To view, visit http://gerrit.cloudera.org:8080/20739 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I130db970a091cdf7689245a79dc4ea445d1f739f Gerrit-Change-Number: 20739 Gerrit-PatchSet: 2 Gerrit-Owner: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Comment-Date: Wed, 29 Nov 2023 15:48:57 + Gerrit-HasComments: Yes
[kudu-CR] [compaction/flush] Cleanup of compaction and flush code paths
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20720 ) Change subject: [compaction/flush] Cleanup of compaction and flush code paths .. Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/20720/4/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/20720/4/src/kudu/tablet/tablet.cc@2001 PS4, Line 2001: size_t Tablet::GetAllDeltasSizeOnDisk(const RowSetsInCompactionOrFlush &input) { > Just curious, is there a use case where it would be useful to have separate MinorDeltaCompaction works on just REDO deltas but that generally results in low memory requirements in comparison with memory requirements expected in rowset compaction. So, having separate methods for REDO and UNDO use cases is unlikely. I guess we can approach this on need basis just in case such scenario arises in future. http://gerrit.cloudera.org:8080/#/c/20720/4/src/kudu/tablet/tablet.cc@2023 PS4, Line 2023: size_t deltas_on_disk_size = 0; > Is 'deltas_on_disk_size' going to be used in a follow up patch? IIUC it's s It is used on line no. 2265, 2276, 2282 -- To view, visit http://gerrit.cloudera.org:8080/20720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic1c2f20230aa089baf34f418a59b6c97f7351217 Gerrit-Change-Number: 20720 Gerrit-PatchSet: 4 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Thu, 23 Nov 2023 11:12:43 + Gerrit-HasComments: Yes
[kudu-CR] [compaction/flush] Cleanup of compaction and flush code paths
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20720 to look at the new patch set (#4). Change subject: [compaction/flush] Cleanup of compaction and flush code paths .. [compaction/flush] Cleanup of compaction and flush code paths The change tries to add more comments, rename methods for better code readability and understanding. It mainly covers two codepaths: Memrowset flush and RowsetCompaction. Change-Id: Ic1c2f20230aa089baf34f418a59b6c97f7351217 --- M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.cc M src/kudu/tablet/rowset.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h 12 files changed, 200 insertions(+), 152 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/20720/4 -- To view, visit http://gerrit.cloudera.org:8080/20720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic1c2f20230aa089baf34f418a59b6c97f7351217 Gerrit-Change-Number: 20720 Gerrit-PatchSet: 4 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [compaction/flush] Cleanup of compaction and flush code paths
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20720 to look at the new patch set (#3). Change subject: [compaction/flush] Cleanup of compaction and flush code paths .. [compaction/flush] Cleanup of compaction and flush code paths The change tries to add more comments, rename methods for better code readability and understanding. It mainly covers two codepaths: Memrowset flush and RowsetCompaction. Change-Id: Ic1c2f20230aa089baf34f418a59b6c97f7351217 --- M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.cc M src/kudu/tablet/rowset.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h 12 files changed, 200 insertions(+), 153 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/20720/3 -- To view, visit http://gerrit.cloudera.org:8080/20720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic1c2f20230aa089baf34f418a59b6c97f7351217 Gerrit-Change-Number: 20720 Gerrit-PatchSet: 3 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3523 Use f bsize as Kudu block size instead of st blksize
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20680 ) Change subject: KUDU-3523 Use f_bsize as Kudu block size instead of st_blksize .. Patch Set 3: Generally, both st_blksize and f_bsize are same when we are working on a native filesystems on linux e.g. ext3/ext4. The use case that you have mentioned here seems to be unusual. I noticed the filesystem type that you are using is of type overlayfs which IIUC is a unionfs/layeredfs that probably had layers of different filesystem type. I can't say for sure, but there is a possibility that in layeredfs, the st_blksize may differ from the f_bsize depending on which filesystem driver is used for get that information. I am planning to test this out on local setup. Will try to share the details once I complete the test or you can update here if you are already aware of it. I am very much interested in knowing more about the setup you are using, what all actual filesystems are constituting this setup, purpose of having a overlayfs setup, directory structure used in overlayfs setup, etc. LBM needs hole punching support from underlying filesystem (ext4 does provide that currently). However, I am not quite clear on what it would mean if Kudu data is residing on overlayfs that has multiple different filesystems underneath it and there is one filesystem that doesn't support hole-punching. Maybe it would be worth an exercise to evaluate behaviour of POSIX APIs (used in Kudu) on overlayfs mount-point. IIUC, overlayfs is just an abstraction layer and all the requests go to targeted underlying file-system so it shouldn't matter much but it is worth a try though because what all underlying filesystems are supported for Kudu is something that is not clearly visible when it is an overlayfs setup. TL;DR: You could provide more information on the following points: 1. Provide some more information about the directory setup and how it uses overlayfs. 2. Check if all the underlying filesystems support hole punching (if Kudu is using LBM). 3. Purpose of using overlayfs. 4. Can you try reproduction with same steps on a regular underlying filesystem e.g. ext4 (just to rule out other possibilities)? -- To view, visit http://gerrit.cloudera.org:8080/20680 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I04343caf5fe377a4fa9b57e6e450307e6b995928 Gerrit-Change-Number: 20680 Gerrit-PatchSet: 3 Gerrit-Owner: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Wed, 22 Nov 2023 14:15:42 + Gerrit-HasComments: No
[kudu-CR] [compaction/flush] Cleanup of compaction and flush code paths
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20720 to look at the new patch set (#2). Change subject: [compaction/flush] Cleanup of compaction and flush code paths .. [compaction/flush] Cleanup of compaction and flush code paths The change tries to add more comments, rename methods for better code readability and understanding. It mainly covers two codepaths: Memrowset flush and RowsetCompaction. Change-Id: Ic1c2f20230aa089baf34f418a59b6c97f7351217 --- M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.cc M src/kudu/tablet/rowset.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h 12 files changed, 200 insertions(+), 153 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/20720/2 -- To view, visit http://gerrit.cloudera.org:8080/20720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic1c2f20230aa089baf34f418a59b6c97f7351217 Gerrit-Change-Number: 20720 Gerrit-PatchSet: 2 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [compaction/flush] Cleanup of compaction and flush code paths
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20720 ) Change subject: [compaction/flush] Cleanup of compaction and flush code paths .. Patch Set 1: For some reason, my indentation went haywire before pushing this patch. I am correcting those now. Expect a rev shortly. -- To view, visit http://gerrit.cloudera.org:8080/20720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic1c2f20230aa089baf34f418a59b6c97f7351217 Gerrit-Change-Number: 20720 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 21 Nov 2023 18:19:10 + Gerrit-HasComments: No
[kudu-CR] [compaction/flush] Cleanup of compaction and flush code paths
Ashwani Raina has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20720 Change subject: [compaction/flush] Cleanup of compaction and flush code paths .. [compaction/flush] Cleanup of compaction and flush code paths The change tries to add more comments, rename methods for better code readability and understanding. It mainly covers two codepaths: Memrowset flush and RowsetCompaction. Change-Id: Ic1c2f20230aa089baf34f418a59b6c97f7351217 --- M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.cc M src/kudu/tablet/rowset.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h 12 files changed, 185 insertions(+), 141 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/20720/1 -- To view, visit http://gerrit.cloudera.org:8080/20720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ic1c2f20230aa089baf34f418a59b6c97f7351217 Gerrit-Change-Number: 20720 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina
[kudu-CR] [cfile] clean up on IndexBlock{Builder,Iterator,Reader}
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20690 ) Change subject: [cfile] clean up on IndexBlock{Builder,Iterator,Reader} .. Patch Set 8: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/20690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If83dd132b577a481a2ddaa09e2657639f8b92c7d Gerrit-Change-Number: 20690 Gerrit-PatchSet: 8 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 16 Nov 2023 09:38:46 + Gerrit-HasComments: No
[kudu-CR] [cfile] clean up on IndexBlock{Builder,Iterator,Reader}
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20690 ) Change subject: [cfile] clean up on IndexBlock{Builder,Iterator,Reader} .. Patch Set 7: (3 comments) Overall looks good to me. Just a few minor comments. http://gerrit.cloudera.org:8080/#/c/20690/7/src/kudu/cfile/index-test.cc File src/kudu/cfile/index-test.cc: http://gerrit.cloudera.org:8080/#/c/20690/7/src/kudu/cfile/index-test.cc@348 PS7, Line 348: TEST(TestIndexBlock, EmptyBlock) { nit: Add a one liner about what this test does Same goes for line number 378 http://gerrit.cloudera.org:8080/#/c/20690/7/src/kudu/cfile/index-test.cc@404 PS7, Line 404: 2048 How is this different from 8 keys iterations above? Testing boundary condition? http://gerrit.cloudera.org:8080/#/c/20690/7/src/kudu/cfile/index_block.cc File src/kudu/cfile/index_block.cc: http://gerrit.cloudera.org:8080/#/c/20690/7/src/kudu/cfile/index_block.cc@268 PS7, Line 268: if (PREDICT_FALSE(data_.data() + offset_in_block >= key_offsets_)) { : return Status::Corruption(Substitute( : "$0: invalid block offset at index $1", offset_in_block, idx_in_block)); : } Is it possible to cover this and other uncovered error scenarios in the unit tests? -- To view, visit http://gerrit.cloudera.org:8080/20690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If83dd132b577a481a2ddaa09e2657639f8b92c7d Gerrit-Change-Number: 20690 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 15 Nov 2023 15:22:02 + Gerrit-HasComments: Yes
[kudu-CR] [cfile] clean up on IndexBlock{Builder,Iterator,Reader}
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20690 ) Change subject: [cfile] clean up on IndexBlock{Builder,Iterator,Reader} .. Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/20690/6/src/kudu/cfile/index_block.cc File src/kudu/cfile/index_block.cc: http://gerrit.cloudera.org:8080/#/c/20690/6/src/kudu/cfile/index_block.cc@149 PS6, Line 149: max_size What does max_size mean here? Does it represent the limit for sanity check only or is it actual size of trailer data? http://gerrit.cloudera.org:8080/#/c/20690/6/src/kudu/cfile/index_block.cc@152 PS6, Line 152: fieds fields http://gerrit.cloudera.org:8080/#/c/20690/6/src/kudu/cfile/index_block.cc@153 PS6, Line 153: // IndexBlockTrailerPB message, it contains at least two required fields now: : // * int32_t num_entries: at least one byte serialized with varint encoding : // * BlockType type: enum, at least one byte serialized : // So, the total for the minimum length is 5 bytes: (1 + 1) + (1 + 1). Is it possible to add some details as comments to represent an example in some visual format? -- To view, visit http://gerrit.cloudera.org:8080/20690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If83dd132b577a481a2ddaa09e2657639f8b92c7d Gerrit-Change-Number: 20690 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 10 Nov 2023 13:07:56 + Gerrit-HasComments: Yes
[kudu-CR] [compaction] Skip memory allocation for ancient undo deltas
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20546 ) Change subject: [compaction] Skip memory allocation for ancient undo deltas .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/20546/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20546/1//COMMIT_MSG@7 PS1, Line 7: [compaction] Skip memory allocation for ancient undo deltas > Ashwani, has there been any progress in clarifying on the latter item (poss Yes, #1 and #2 were already covered and #3 has been addressed under TestGhostRowsUpatesAHM test (uploaded on Oct 27th in patch set #5) that covers the case of multiple ghosts spread across AHM along with updates/deletes/inserts and data integrity checks. The patch is good to go. Thanks! -- To view, visit http://gerrit.cloudera.org:8080/20546 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibb41636a3ac063478fe181560d4ec85dadeb0ef3 Gerrit-Change-Number: 20546 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Mon, 06 Nov 2023 08:26:05 + Gerrit-HasComments: Yes
[kudu-CR] [compaction] Skip memory allocation for ancient undo deltas
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20546 ) Change subject: [compaction] Skip memory allocation for ancient undo deltas .. Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/20546/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20546/4//COMMIT_MSG@10 PS4, Line 10: we > it I guess "compaction code doesn't" would sound better. http://gerrit.cloudera.org:8080/#/c/20546/4//COMMIT_MSG@11 PS4, Line 11: ancient > Please give more detail about the definition `ancient`? I can give a little more detail as part of response to your comment. It wouldn't be worth adding that detail in the commit message. There is a flag (tablet_history_max_age_sec) that denotes the duration for which UNDO deltas are retained. If the age of UNDO deltas are more than this flag value, it is considered to be ancient and is eventually deleted either via GC maintenance op or rowset compaction maintenance op. The use case of retaining these deltas is to cater to historical scan requests, incremental backups, etc. Default value for tablet_history_max_age_sec is a week i.e. any historical scan back upto a week can be served and any scan request for data older than one week will only serve the oldest delta present i.e. 7 days old. One could change the flag value based upon their requirement of how old a data one wants to retain. http://gerrit.cloudera.org:8080/#/c/20546/4/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: http://gerrit.cloudera.org:8080/#/c/20546/4/src/kudu/tablet/compaction.cc@1243 PS4, Line 1243: in the calle > Can you describe the caller more clear? Done. http://gerrit.cloudera.org:8080/#/c/20546/4/src/kudu/tablet/compaction.cc@1307 PS4, Line 1307: //in caller while processing ancient undo deltas. Essentially, > Maybe you can remove this comments to line 1282. The comment is relevant and desirable here because this provides details about how the workflow of "INSERT after DELETE" and "DELETE being the last mutation" work as far as compaction is concerned. Even though there is no code change here, the comments explain the existing code for posterity. -- To view, visit http://gerrit.cloudera.org:8080/20546 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibb41636a3ac063478fe181560d4ec85dadeb0ef3 Gerrit-Change-Number: 20546 Gerrit-PatchSet: 4 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Fri, 27 Oct 2023 08:25:38 + Gerrit-HasComments: Yes
[kudu-CR] [compaction] Skip memory allocation for ancient undo deltas
Hello Tidy Bot, Alexey Serbin, Zoltan Martonka, Kudu Jenkins, Wang Xixu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20546 to look at the new patch set (#5). Change subject: [compaction] Skip memory allocation for ancient undo deltas .. [compaction] Skip memory allocation for ancient undo deltas Currently, while applying REDO mutations to base row and create corresponding UNDO deltas for each REDO mutation, compaction code doesn't check whether any mutation is ancient that is anyway going to be ignored and removed from list of UNDO deltas in later stage of processing. This change checks each REDO mutation beforehand and doesn't allocate any memory if found ancient. This avoids unnecessary memory usage. Change-Id: Ibb41636a3ac063478fe181560d4ec85dadeb0ef3 --- M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/tablet-test-base.h M src/kudu/tablet/tablet_history_gc-test.cc 5 files changed, 206 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/20546/5 -- To view, visit http://gerrit.cloudera.org:8080/20546 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibb41636a3ac063478fe181560d4ec85dadeb0ef3 Gerrit-Change-Number: 20546 Gerrit-PatchSet: 5 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Zoltan Martonka
[kudu-CR] [compaction] Skip memory allocation for ancient undo deltas
Hello Tidy Bot, Alexey Serbin, Zoltan Martonka, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20546 to look at the new patch set (#4). Change subject: [compaction] Skip memory allocation for ancient undo deltas .. [compaction] Skip memory allocation for ancient undo deltas Currently, while applying REDO mutations to base row and create corresponding UNDO deltas for each REDO mutation, we don't check whether any mutation is ancient that is anyway going to be ignored and removed from list of UNDO deltas in later stage of processing. This change checks each REDO mutation beforehand and doesn't allocate any memory if found ancient. This avoids unnecessary memory usage. Change-Id: Ibb41636a3ac063478fe181560d4ec85dadeb0ef3 --- M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/tablet-test-base.h M src/kudu/tablet/tablet_history_gc-test.cc 5 files changed, 204 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/20546/4 -- To view, visit http://gerrit.cloudera.org:8080/20546 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibb41636a3ac063478fe181560d4ec85dadeb0ef3 Gerrit-Change-Number: 20546 Gerrit-PatchSet: 4 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Martonka
[kudu-CR] [compaction] Skip memory allocation for ancient undo deltas
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20546 ) Change subject: [compaction] Skip memory allocation for ancient undo deltas .. Patch Set 3: Note: Need to correct couple of things. Expect updated patch shortly. -- To view, visit http://gerrit.cloudera.org:8080/20546 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibb41636a3ac063478fe181560d4ec85dadeb0ef3 Gerrit-Change-Number: 20546 Gerrit-PatchSet: 3 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Thu, 26 Oct 2023 14:03:33 + Gerrit-HasComments: No
[kudu-CR] [compaction] Skip memory allocation for ancient undo deltas
Hello Tidy Bot, Alexey Serbin, Zoltan Martonka, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20546 to look at the new patch set (#3). Change subject: [compaction] Skip memory allocation for ancient undo deltas .. [compaction] Skip memory allocation for ancient undo deltas Currently, while applying REDO mutations to base row and create corresponding UNDO deltas for each REDO mutation, we don't check whether any mutation is ancient that is anyway going to be ignored and removed from list of UNDO deltas in later stage of processing. This change checks each REDO mutation beforehand and doesn't allocate any memory if found ancient. This avoids unnecessary memory usage. Change-Id: Ibb41636a3ac063478fe181560d4ec85dadeb0ef3 --- M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/tablet-test-base.h M src/kudu/tablet/tablet_history_gc-test.cc 5 files changed, 189 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/20546/3 -- To view, visit http://gerrit.cloudera.org:8080/20546 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibb41636a3ac063478fe181560d4ec85dadeb0ef3 Gerrit-Change-Number: 20546 Gerrit-PatchSet: 3 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Martonka
[kudu-CR] WIP [rpc] Set rpc max message size per available memory to accomodate huge response payloads
Ashwani Raina has abandoned this change. ( http://gerrit.cloudera.org:8080/20528 ) Change subject: WIP [rpc] Set rpc_max_message_size per available memory to accomodate huge response payloads .. Abandoned Fixed and merged in : https://gerrit.cloudera.org/#/c/20535/ -- To view, visit http://gerrit.cloudera.org:8080/20528 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I9c92e5469f806c827a8353fdf6de5a24a221613c Gerrit-Change-Number: 20528 Gerrit-PatchSet: 2 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [codegen] Add include directories in the path as per libc++ version
Ashwani Raina has abandoned this change. ( http://gerrit.cloudera.org:8080/19565 ) Change subject: [codegen] Add include directories in the path as per libc++ version .. Abandoned Not applicable anymore. -- To view, visit http://gerrit.cloudera.org:8080/19565 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I8e8a563f69415e7d15de406bf05cec18d8ff3c74 Gerrit-Change-Number: 19565 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai