[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_,
));
: 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
[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 : 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 ) {
> 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
[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
> Thank you for putting together the explanation above, iterating over the sa
Yes, both #1 and #2 are covered under the newly added test.
As for #3, it is partially covered as well where the test makes sure that scan
on old timestamps return the expected data for both cases i.e.
Before compaction:
scan@ts[after_insert] -> 0
scan@ts[after_update1] -> 1
scan@ts[after_update2] -> 2
After compaction, scan ops return 2 because there is no UNDO delta present
anymore so the only data changes that are present are the latest one:
scan@ts[after_insert] -> 2
scan@ts[after_update1] -> 2
scan@ts[after_update2] -> 2
Additionally, there is one existing test i.e. TestGhostRowsNotRevived that
creates ghost rows, turns them ancient and ensures those are not to be found
after compaction job.
I thought about adding metrics but IMO it turns out to be an overkill just to
get the information on how many times memory allocation was skipped. It will
require passing of tablet object's metrics to standalone methods like
FlushCompactionInput, MergeDuplicatedRowHistory, ApplyMutationsAndGenerateUndos
along with changing the callers everywhere including flush operation workflows.
I would rather not add metrics. FWIW, I did add metrics code in the test and
verified that it was working fine.
Excerpts:
>>>
// Assert that there was no memory allocation for UNDO delta for corresponding
ancient REDOs
int expected_total_mutations = kNumRowsets * rows_per_rowset_ * 2;
ASSERT_EQ(expected_total_mutations,
metrics->ancient_redo_compact_mem_alloc_skipped->value());
---
if (history_gc_opts.IsAncientHistory(redo_mut->timestamp())) {
if (metrics)
metrics->ancient_redo_compact_mem_alloc_skipped->Increment();
continue;
}
<<<
Let me see if there is something else on the lines of multiple ghosts spread
across a AHM mark that needs to be covered.
--
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: Alexey Serbin
Gerrit-Reviewer: Ashwani Raina
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Martonka
Gerrit-Comment-Date: Tue, 24 Oct 2023 15:51:24 +
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 > From what I can see, the TabletHistoryGcTest.TestNoGenerateUndoOnMajorDelta There is no update in functionality here. The only difference is that we take note of "ancient" REDO mutations quite early in the processing rather than in later part of compaction. Without this change, compaction does take into consideration all the ancient REDO mutations and allocates memory for their corresponding UNDO deltas only to ignore those in the later stage of processing. With this change, compaction doesn't take into consideration all the ancient REDO mutations from get go and hence no memory is allocated for converting those mutations to UNDO deltas. So, the behaviour remains the same i.e. after flushing updates on a row, the mutations that have become ancient, should have no corresponding UNDO mutations present. I am not quite clear on what outcome you are expecting from the path mentioned above. Considering above scenario, here are my thoughts. When a lot of REDO deltas are generated, those can be converted into UNDO deltas via either MajorDeltaCompaction or RowSet Compaction while those REDO deltas are not ancient. However, if there is a change in AHM, to say 1 sec after a restart, and a compaction maintenance op kicks in afterwards, it is going to process those REDO deltas and treat them as ancient. As those are ancient, compaction will not have any UNDO delta for such REDO mutations. Eventually, what we will be left with is the list of only those mutations that are not ancient. Even if there are some REDO deltas that were minor-compacted, it won't matter because those will have ancient timestamp on resultant REDO delta as well. The newly added test does verify by ensuring that after rowset compaction is complete, any history scan (beyond AHM) should yield the latest row data value i.e. 2 because all the REDO mutations have been applied and there is no corresponding UNDO delta for those (that represent first INSERT and subsequent UPDATE to value 2). Similarly, if the same set of history scan is done before compaction was invoked, it gives the old mutations i.e. row value as 0, 1. These test essentially ensure that there are no change in functionalities and expectations as far as handling of ancient REDO mutation goes. One test already ensures that by verifying for MajorDeltaCompaction. The new one that I added ensures the same for rowset compaction. As part of manual unit testing, I had added some stats/counter to measure the number of times memory allocation was skipped. This was more of a manual verification. Maybe I can add that metric and verify at the end of the test. -- 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: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Sun, 22 Oct 2023 20:27:50 + Gerrit-HasComments: Yes
[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 (#2). 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, 121 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/20546/2 -- 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: 2 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 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 > I think it's necessary to add a test at least to verify that the data is st Thanks for the review, Alexey. Apologies for being late on this. There is one test (TestNoGenerateUndoOnMajorDeltaCompaction) that does cover this code path already. It updates rows couple of times and then flushes the DMS thereby creating REDO deltas for those updates. Later, it moves the clock forwards to make those deltas ancient. Subsequent reads on the rows are verified for both the updates. Lastly, major delta compaction is run that results in this execution of exactly this piece of code where REDO mutations are processed and converted to UNDO deltas. However, in this specific case, we end with AHM case because by now all these deltas are ancient. So, what we have eventually is this: Base row data filled with latest update and no REDO mutations and just one UNDO delta for DELETE for first insert op. The only difference here is that REDO was applied from major delta compaction instead of rowset compaction. Since ApplyMutationsAndGenerateUndos is applicable to both Major and RowSet compaction, it doesn't make much difference except that major compaction won't touch the already generated UNDO deltas as it only works on currently existing REDO deltas and applies those to base row. However, rowset compaction does take into consideration existing UNDO deltas which in this specific test case would be only 1 in number (for the first insert). Either way, we are only working on the existing REDO deltas here and make sure that we don't allocate memory for their corresponding in-memory UNDO delta object if the REDO mutation is ancient. So, in a nutshell, the existing test already verifies that there is no UNDO delta created for ancient REDO mutations and also make sure that those REDO updates are applied to the base row thereby verifying that correct data is applied on the row. I hope my explanation above didn't end up creating more confusion :-) If you want, I can add one similar test that does the similar steps and verify at the end the following: 1. There is no ancient UNDO delta as well as REDO delta present. 2. The row has latest value applied to it. If you have something different and extra in mind with regard to fuzz tests, I am open to discussion. -- 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: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Thu, 19 Oct 2023 11:51:05 + Gerrit-HasComments: Yes
[kudu-CR] [CLI] Set rpc max message size to accommodate huge response payloads
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20535 to look at the new patch set (#4). Change subject: [CLI] Set rpc_max_message_size to accommodate huge response payloads .. [CLI] Set rpc_max_message_size to accommodate huge response payloads It has been observed that for certain Kudu CLI commands response payload size is too big to fit in the current default RPC message size limit of 50MiB. This patch adds logic to set the value of RPC message max size for Kudu CLI based on maximum available memory or maximum possible RPC message size limit of 2GiB. This would help accommodate that extra heavy response payload. Along with increasing default value of rpc_max_message_size, default value of tablet_transaction_memory_limit_mb is also set to at least same value as rpc_max_message_size to pass the group validation check. This change of default flag values is only applicable to Kudu CLI tool. Change-Id: Ic27b494bc1fde46c2a095c7291fc840a98429068 --- M src/kudu/tools/tool_main.cc M src/kudu/util/process_memory.cc M src/kudu/util/process_memory.h 3 files changed, 51 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/35/20535/4 -- To view, visit http://gerrit.cloudera.org:8080/20535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic27b494bc1fde46c2a095c7291fc840a98429068 Gerrit-Change-Number: 20535 Gerrit-PatchSet: 4 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [CLI] Set rpc max message size to accommodate huge response payloads
Ashwani Raina has posted comments on this change. (
http://gerrit.cloudera.org:8080/20535 )
Change subject: [CLI] Set rpc_max_message_size to accommodate huge response
payloads
..
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20535/2/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
http://gerrit.cloudera.org:8080/#/c/20535/2/src/kudu/tools/tool_main.cc@264
PS2, Line 264: int64_t max_memory_available =
kudu::process_memory::MaxMemoryAvailable();
> I guess the observed behavior might be related to the fact that current tes
That's a possibility. Would need to verify that.
http://gerrit.cloudera.org:8080/#/c/20535/3/src/kudu/util/process_memory.cc
File src/kudu/util/process_memory.cc:
http://gerrit.cloudera.org:8080/#/c/20535/3/src/kudu/util/process_memory.cc@219
PS3, Line 219: int64_t MaxMemoryAvailable() {
: int64_t total_ram;
: CHECK_OK(Env::Default()->GetTotalRAMBytes(_ram));
: // We will use 80% of system RAM to align with default hard
limit.
: total_ram = total_ram * 4;
: total_ram /= 5;
:
: return total_ram;
: }
> Once this new function is introduced, does it make sense to use it in DoIni
Yeah, that makes sense.
Thanks for pointing that out.
--
To view, visit http://gerrit.cloudera.org:8080/20535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic27b494bc1fde46c2a095c7291fc840a98429068
Gerrit-Change-Number: 20535
Gerrit-PatchSet: 3
Gerrit-Owner: Ashwani Raina
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Ashwani Raina
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 10 Oct 2023 15:21:51 +
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@12 PS1, Line 12: removed from list of UNDO deltas in later stage of processing > What happens with such REDO deltas then? IIRC, UNDO deltas then garbage co Lifecycle of REDO deltas would remain the same as before. For each row index in a block (under processing), after the mutations are applied and REDOs are converted to UNDOs, whatever is left in REDO list is applied to the corresponding row. Usually, they may be nothing left in the REDO list, so at the end of compaction, we would only have list of UNDO deltas and base data containing the latest version. So, the life of a REDO delta would start at the time of delta memstore flush to disk up till compaction kicks in and converts them to UNDO deltas. As for in-memory representation of the REDO delta is concerned, its lifecycle will also remain the same i.e. will get allocated at the time allocating row block memory and deallocated after completion of each iteration of row block processing for compaction. -- 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: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Tue, 10 Oct 2023 15:17:54 + Gerrit-HasComments: Yes
[kudu-CR] [compaction] Skip memory allocation for ancient undo deltas
Ashwani Raina has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20546 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 3 files changed, 57 insertions(+), 15 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/20546/1 -- 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: newchange Gerrit-Change-Id: Ibb41636a3ac063478fe181560d4ec85dadeb0ef3 Gerrit-Change-Number: 20546 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina
