[kudu-CR] [java] KUDU-3455 Reduce space complexity about pruning hash partitions for in-list predicate
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/19568 )
Change subject: [java] KUDU-3455 Reduce space complexity about pruning hash
partitions for in-list predicate
..
Patch Set 8:
(14 comments)
Thank you very much for the fix!
I did a quick first look, left a few comments. I'll do one more pass soon to
make sure I understand it better, but so far looks good to me.
http://gerrit.cloudera.org:8080/#/c/19568/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19568/6//COMMIT_MSG@7
PS6, Line 7: complexity about pruning hash partitions
complexity of hash partition pruning
http://gerrit.cloudera.org:8080/#/c/19568/6//COMMIT_MSG@9
PS6, Line 9: java-client
Kudu Java client
http://gerrit.cloudera.org:8080/#/c/19568/6//COMMIT_MSG@12
PS6, Line 12: a little
nit: drop 'a little' :)
http://gerrit.cloudera.org:8080/#/c/19568/6//COMMIT_MSG@15
PS6, Line 15: algorithm like 'deep first search
DFS-based algorithm
http://gerrit.cloudera.org:8080/#/c/19568/6//COMMIT_MSG@19
PS6, Line 19: pressure experiments
What are "pressure experiments"?
http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java
File java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java:
http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java@349
PS8, Line 349: public
Is it possible to keep this package-private, not exposing as public? E.g.,
take a look at shouldPruneForTests() above.
http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java@640
PS8, Line 640: public static BitSet pruneHashComponentV2ForTest(Schema schema,
: PartitionSchema.HashBucketSchema hashSchema,
: Map predicates) {
: return pruneHashComponentV2(schema, hashSchema, predicates);
: }
Is it possible to keep this package-private, not exposing as public? E.g.,
take a look at shouldPruneForTests() above.
http://gerrit.cloudera.org:8080/#/c/19568/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java
File
java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java:
http://gerrit.cloudera.org:8080/#/c/19568/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java@704
PS6, Line 704: CREATE TABLE t
Would be great to describe on the essence of the test cases below in a comment:
'carefully' is too vague to reflect on what's being tested.
http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java
File
java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java:
http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java@889
PS8, Line 889: public void testInListHashPartitionPruningUsingLargeList()
throws Exception {
It would be great to add a comment to summarize the essence of this test
scenario.
http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java@967
PS8, Line 967: BitSet newBitset
nit: add 'final' for this as well?
http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java@984
PS8, Line 984: \
nit: remove the backslash?
http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java@985
PS8, Line 985: oom
nit: OOM condition
http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java@985
PS8, Line 985: use
using
http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java@986
PS8, Line 986: How to test OOM? Leave a 'TODO' at this case.
How about:
For details on testing for the OOM condition, see the in-line TODO comment in
the end this scenario.
--
To view, visit http://gerrit.cloudera.org:8080/19568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icd6f213cb705e1b2a001562cc7cebe4164281723
Gerrit-Change-Number: 19568
Gerrit-PatchSet: 8
Gerrit-Owner: Yuqi Du
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: Yifan Zhang
Gerrit-Reviewer: Yingchun Lai
Gerrit-Reviewer: Yuqi Du
Gerrit-Comment-Date: Fri, 31 Mar 2023 05:36:27 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Hello Marton Greber, Tidy Bot, Alexey Serbin, Kudu Jenkins, Wenzhe Zhou, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19445 to look at the new patch set (#11). Change subject: KUDU-1945 Update auto incrementing counter during bootstrap .. KUDU-1945 Update auto incrementing counter during bootstrap The auto incrementing counter would be reset to zero when the tablet is being initialized. It is essential to have the correct value of the counter. There are two cases: 1. WAL segments contain insert operations In this scenario the WAL segments are replayed and since each insert operation entry has auto incrementing counter which will be used for the insert operations present in that entry, as long as we have the latest insert operation entry in the WAL segments, the auto incrementing counter is populated correctly during bootstrap. 2. WAL segments do not contain insert operations In this case, we need to fetch the highest auto incrementing counter value present in the data which is already flushed and update the in-memory auto incrementing counter appropriately. This patch accomplishes this task. There are tests for the bootstrap scenarios where 1. We have no WAL segments with an INSERT OP containing the auto incrementing column value. We rely on the auto incrementing counter present in the data directories in this case. 2. We have no WAL segments with auto incrementing column value and all the rows of the table are deleted. We reset the auto incrementing counter in this case. 3. We have non committed replicate ops containing INSERT OPs with the auto incrementing column values. Manually tested the time taken to populate the auto incrementing counter: Columns - A Non Unique Primary Key column of type INT32 - 8 INT64 columns - 5 STRING columns Rows - 500k rows with data populated Time taken in populating the counter during bootstrap: Min - 235ms Max - 466ms Median - 312ms The total time spent boostrapping the tablet was between 18-25 seconds. Change-Id: I61b305efd7d5a065a2976327567163956c0c2184 --- M src/kudu/integration-tests/auto_incrementing-itest.cc M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tserver/tablet_server-test-base.h 4 files changed, 328 insertions(+), 42 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/11 -- To view, visit http://gerrit.cloudera.org:8080/19445 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184 Gerrit-Change-Number: 19445 Gerrit-PatchSet: 11 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Hello Marton Greber, Tidy Bot, Alexey Serbin, Kudu Jenkins, Wenzhe Zhou, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19445 to look at the new patch set (#10). Change subject: KUDU-1945 Update auto incrementing counter during bootstrap .. KUDU-1945 Update auto incrementing counter during bootstrap The auto incrementing counter would be reset to zero when the tablet is being initialized. It is essential to have the correct value of the counter. There are two cases: 1. WAL segments contain insert operations In this scenario the WAL segments are replayed and since each insert operation entry has auto incrementing counter which will be used for the insert operations present in that entry, as long as we have the latest insert operation entry in the WAL segments, the auto incrementing counter is populated correctly during bootstrap. 2. WAL segments do not contain insert operations In this case, we need to fetch the highest auto incrementing counter value present in the data which is already flushed and update the in-memory auto incrementing counter appropriately. This patch accomplishes this task. There are tests for the bootstrap scenarios where 1. We have no WAL segments with an INSERT OP containing the auto incrementing column value. We rely on the auto incrementing counter present in the data directories in this case. 2. We have no WAL segments with auto incrementing column value and all the rows of the table are deleted. We reset the auto incrementing counter in this case. 3. We have non committed replicate ops containing INSERT OPs with the auto incrementing column values. Manually tested the time taken to populate the auto incrementing counter: Columns - A Non Unique Primary Key column of type INT32 - 8 INT64 columns - 5 STRING columns Rows - 500k rows with data populated Time taken in populating the counter during bootstrap: Min - 235ms Max - 466ms Median - 312ms The total time spent boostrapping the tablet was between 18-25 seconds. Change-Id: I61b305efd7d5a065a2976327567163956c0c2184 --- M src/kudu/integration-tests/auto_incrementing-itest.cc M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tserver/tablet_server-test-base.h 4 files changed, 328 insertions(+), 42 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/10 -- To view, visit http://gerrit.cloudera.org:8080/19445 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184 Gerrit-Change-Number: 19445 Gerrit-PatchSet: 10 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
..
Patch Set 9:
> Uploaded patch set 9.
It seems the new tests fails?
/home/jenkins-slave/workspace/kudu-master/0/src/kudu/integration-tests/auto_incrementing-itest.cc:310
Expected equality of these values:
Substitute("(int32 c0=$0, int64 $1=$2)", i + kNumRows,
Schema::GetAutoIncrementingColumnName(), i + 1)
Which is: "(int32 c0=200, int64 auto_incrementing_id=1)"
results[i]
Which is: "(int32 c0=200, int64 auto_incrementing_id=201)"
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 9
Gerrit-Owner: Abhishek Chennaka
Gerrit-Reviewer: Abhishek Chennaka
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou
Gerrit-Comment-Date: Fri, 31 Mar 2023 03:06:45 +
Gerrit-HasComments: No
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Hello Marton Greber, Tidy Bot, Alexey Serbin, Kudu Jenkins, Wenzhe Zhou, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19445 to look at the new patch set (#9). Change subject: KUDU-1945 Update auto incrementing counter during bootstrap .. KUDU-1945 Update auto incrementing counter during bootstrap The auto incrementing counter would be reset to zero when the tablet is being initialized. It is essential to have the correct value of the counter. There are two cases: 1. WAL segments contain insert operations In this scenario the WAL segments are replayed and since each insert operation entry has auto incrementing counter which will be used for the insert operations present in that entry, as long as we have the latest insert operation entry in the WAL segments, the auto incrementing counter is populated correctly during bootstrap. 2. WAL segments do not contain insert operations In this case, we need to fetch the highest auto incrementing counter value present in the data which is already flushed and update the in-memory auto incrementing counter appropriately. This patch accomplishes this task. There are tests for the bootstrap scenarios where 1. We have no WAL segments with an INSERT OP containing the auto incrementing column value. We rely on the auto incrementing counter present in the data directories in this case. 2. We have no WAL segments with auto incrementing column value and all the rows of the table are deleted. We reset the auto incrementing counter in this case. 3. We have non committed replicate ops containing INSERT OPs with the auto incrementing column values. Manually tested the time taken to populate the auto incrementing counter: Columns - A Non Unique Primary Key column of type INT32 - 8 INT64 columns - 5 STRING columns Rows - 500k rows with data populated Time taken in populating the counter during bootstrap: Min - 235ms Max - 466ms Median - 312ms The total time spent boostrapping the tablet was between 18-25 seconds. Change-Id: I61b305efd7d5a065a2976327567163956c0c2184 --- M src/kudu/integration-tests/auto_incrementing-itest.cc M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tserver/tablet_server-test-base.h 4 files changed, 324 insertions(+), 42 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/9 -- To view, visit http://gerrit.cloudera.org:8080/19445 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184 Gerrit-Change-Number: 19445 Gerrit-PatchSet: 9 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 ) Change subject: KUDU-1945 Update auto incrementing counter during bootstrap .. Patch Set 8: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/19445 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184 Gerrit-Change-Number: 19445 Gerrit-PatchSet: 8 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 30 Mar 2023 20:27:54 + Gerrit-HasComments: No
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 ) Change subject: KUDU-1945 Update auto incrementing counter during bootstrap .. Patch Set 8: (8 comments) http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@21 PS6, Line 21: thi > this? Done http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@21 PS6, Line 21: thi > missed addressing this one in PS7? Done http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@23 PS6, Line 23: in-memory > nit: in-memory Done http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@23 PS6, Line 23: in-memory > missing addressing this one in PS7? Done http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@49 PS6, Line 49: > nit: keep an empty line between the commit description and 'Change-Id' line Done http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc File src/kudu/integration-tests/auto_incrementing-itest.cc: http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@394 PS6, Line 394: ASSERT_EQ(results[i][k], results[i + 1][k]); : } > It would be great if you could add a comment into the code to explain why t Done http://gerrit.cloudera.org:8080/#/c/19445/7/src/kudu/integration-tests/auto_incrementing-itest.cc File src/kudu/integration-tests/auto_incrementing-itest.cc: http://gerrit.cloudera.org:8080/#/c/19445/7/src/kudu/integration-tests/auto_incrementing-itest.cc@190 PS7, Line 190: unique_ptr cluster_; > Do we need to make sure resp.status_and_schema isn't empty? If you don't w Done http://gerrit.cloudera.org:8080/#/c/19445/7/src/kudu/integration-tests/auto_incrementing-itest.cc@281 PS7, Line 281: > nit: space around + Done -- To view, visit http://gerrit.cloudera.org:8080/19445 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184 Gerrit-Change-Number: 19445 Gerrit-PatchSet: 8 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 30 Mar 2023 20:01:08 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Hello Marton Greber, Tidy Bot, Alexey Serbin, Kudu Jenkins, Wenzhe Zhou, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19445 to look at the new patch set (#8). Change subject: KUDU-1945 Update auto incrementing counter during bootstrap .. KUDU-1945 Update auto incrementing counter during bootstrap The auto incrementing counter would be reset to zero when the tablet is being initialized. It is essential to have the correct value of the counter. There are two cases: 1. WAL segments contain insert operations In this scenario the WAL segments are replayed and since each insert operation entry has auto incrementing counter which will be used for the insert operations present in that entry, as long as we have the latest insert operation entry in the WAL segments, the auto incrementing counter is populated correctly during bootstrap. 2. WAL segments do not contain insert operations In this case, we need to fetch the highest auto incrementing counter value present in the data which is already flushed and update the in-memory auto incrementing counter appropriately. This patch accomplishes this task. There are tests for the bootstrap scenarios where 1. We have no WAL segments with an INSERT OP containing the auto incrementing column value. We rely on the auto incrementing counter present in the data directories in this case. 2. We have no WAL segments with auto incrementing column value and all the rows of the table are deleted. We reset the auto incrementing counter in this case. 3. We have non committed replicate ops containing INSERT OPs with the auto incrementing column values. Manually tested the time taken to populate the auto incrementing counter: Columns - A Non Unique Primary Key column of type INT32 - 8 INT64 columns - 5 STRING columns Rows - 500k rows with data populated Time taken in populating the counter during bootstrap: Min - 235ms Max - 466ms Median - 312ms The total time spent boostrapping the tablet was between 18-25 seconds. Change-Id: I61b305efd7d5a065a2976327567163956c0c2184 --- M src/kudu/integration-tests/auto_incrementing-itest.cc M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tserver/tablet_server-test-base.h 4 files changed, 321 insertions(+), 42 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/8 -- To view, visit http://gerrit.cloudera.org:8080/19445 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184 Gerrit-Change-Number: 19445 Gerrit-PatchSet: 8 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou
[kudu-CR] [client] Improve logging in client metacache
Yuqi Du has posted comments on this change. (
http://gerrit.cloudera.org:8080/19653 )
Change subject: [client] Improve logging in client metacache
..
Patch Set 5: Code-Review+1
(2 comments)
LGTM
http://gerrit.cloudera.org:8080/#/c/19653/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19653/5//COMMIT_MSG@9
PS5, Line 9: As part for impala crashing issue
Adding this impala's jira issue at this commit message would help us to
understand this problem
http://gerrit.cloudera.org:8080/#/c/19653/5/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:
http://gerrit.cloudera.org:8080/#/c/19653/5/src/kudu/client/meta_cache.cc@465
PS5, Line 465: if (leader) {
: VLOG(1) << "Client copy of tablet " << tablet_->tablet_id()
: << " is fresh, leader uuid " <<
leader->permanent_uuid();
: } else {
: VLOG(1) << "Client copy of tablet " << tablet_->tablet_id()
: << " is fresh (no leader).";
: }
nit:
whether using Substitute("Client copy of tablet $0 is fresh, leader uuid $1")
is better?
--
To view, visit http://gerrit.cloudera.org:8080/19653
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a47f643307ef47a98b1d3481b4c03834fa239d4
Gerrit-Change-Number: 19653
Gerrit-PatchSet: 5
Gerrit-Owner: Ashwani Raina
Gerrit-Reviewer: Abhishek Chennaka
Gerrit-Reviewer: Ashwani Raina
Gerrit-Reviewer: KeDeng
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai
Gerrit-Reviewer: Yuqi Du
Gerrit-Comment-Date: Thu, 30 Mar 2023 15:51:38 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3437 Set default block cache metrics policy to reset
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/19585 ) Change subject: KUDU-3437 Set default block_cache_metrics_policy to reset .. Patch Set 5: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/19585/4/src/kudu/master/master_options.cc File src/kudu/master/master_options.cc: http://gerrit.cloudera.org:8080/#/c/19585/4/src/kudu/master/master_options.cc@43 PS4, Line 43: kReset > It seems not necessary to change this default value? Done -- To view, visit http://gerrit.cloudera.org:8080/19585 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idc32d7ab02201382debcbe36311579550353bf71 Gerrit-Change-Number: 19585 Gerrit-PatchSet: 5 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yuqi Du Gerrit-Reviewer: Zoltan Chovan Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Thu, 30 Mar 2023 15:25:20 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3437 Set default block cache metrics policy to reset
Hello Zoltan Chovan, Alexey Serbin, Yuqi Du, Attila Bukor, Kudu Jenkins, Wang Xixu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19585 to look at the new patch set (#5). Change subject: KUDU-3437 Set default block_cache_metrics_policy to reset .. KUDU-3437 Set default block_cache_metrics_policy to reset If the master has empty local directories and connects to an existing cluster, it executes Master::Init twice, which executes StartInstrumentation twice, which is only allowed if it's a test or the policy is to reset the existing CacheMetrics. According to documentation ExistingMetricsPolicy::kKeep should only be used during tests, so the default value is set to kReset which fixes the original issue. Change-Id: Idc32d7ab02201382debcbe36311579550353bf71 --- M src/kudu/master/master_options.cc M src/kudu/master/mini_master.cc 2 files changed, 3 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/19585/5 -- To view, visit http://gerrit.cloudera.org:8080/19585 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idc32d7ab02201382debcbe36311579550353bf71 Gerrit-Change-Number: 19585 Gerrit-PatchSet: 5 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yuqi Du Gerrit-Reviewer: Zoltan Chovan Gerrit-Reviewer: Ádám Bakai
[kudu-CR] [clock] add sanity check to detect wall clock jumps
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/19473 ) Change subject: [clock] add sanity check to detect wall clock jumps .. Patch Set 8: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/19473 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I630783653717d975a9b2ad668e8bd47b7796d275 Gerrit-Change-Number: 19473 Gerrit-PatchSet: 8 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Yuqi Du Gerrit-Reviewer: Zoltan Chovan Gerrit-Comment-Date: Thu, 30 Mar 2023 09:51:28 + Gerrit-HasComments: No
[kudu-CR] KUDU-3437 Set default block cache metrics policy to reset
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/19585 ) Change subject: KUDU-3437 Set default block_cache_metrics_policy to reset .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/19585/4/src/kudu/master/master_options.cc File src/kudu/master/master_options.cc: http://gerrit.cloudera.org:8080/#/c/19585/4/src/kudu/master/master_options.cc@43 PS4, Line 43: kReset It seems not necessary to change this default value? -- To view, visit http://gerrit.cloudera.org:8080/19585 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idc32d7ab02201382debcbe36311579550353bf71 Gerrit-Change-Number: 19585 Gerrit-PatchSet: 4 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yuqi Du Gerrit-Reviewer: Zoltan Chovan Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Thu, 30 Mar 2023 09:50:56 + Gerrit-HasComments: Yes
[kudu-CR] [master] Exclude tservers in MAINTENANCE MODE when leader rebalancing
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/19608 ) Change subject: [master] Exclude tservers in MAINTENANCE_MODE when leader rebalancing .. Patch Set 10: (2 comments) http://gerrit.cloudera.org:8080/#/c/19608/10/src/kudu/master/auto_leader_rebalancer-test.cc File src/kudu/master/auto_leader_rebalancer-test.cc: http://gerrit.cloudera.org:8080/#/c/19608/10/src/kudu/master/auto_leader_rebalancer-test.cc@64 PS10, Line 64: using strings::Split; > warning: using decl 'Split' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/19608/10/src/kudu/master/auto_leader_rebalancer-test.cc@65 PS10, Line 65: using strings::Substitute; > warning: using decl 'Substitute' is unused [misc-unused-using-decls] Done -- To view, visit http://gerrit.cloudera.org:8080/19608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f85a675e69fd02a62e2625881dad2ca5e27acd9 Gerrit-Change-Number: 19608 Gerrit-PatchSet: 10 Gerrit-Owner: Yuqi Du Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Yuqi Du Gerrit-Comment-Date: Thu, 30 Mar 2023 08:36:25 + Gerrit-HasComments: Yes
[kudu-CR] [master] Exclude tservers in MAINTENANCE MODE when leader rebalancing
Hello Mahesh Reddy, Tidy Bot, Alexey Serbin, Yingchun Lai, Yifan Zhang, Attila Bukor, Kudu Jenkins, Abhishek Chennaka, KeDeng, Wang Xixu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19608 to look at the new patch set (#11). Change subject: [master] Exclude tservers in MAINTENANCE_MODE when leader rebalancing .. [master] Exclude tservers in MAINTENANCE_MODE when leader rebalancing Currently, auto leader rebalancing may transfer new leaders to tservers which have entered MAINTENANCE_MODE. Because a tserver in MAINTENANCE_MODE is being maintained by administrators, maybe it is decommissioning. Since it might be decommissioned/decommissioning, it doesn't make sense for the leader rebalancer to transfer a leader to a tserver in MAINTENANCE_MODE as leaders serve many users' requests. For this reason, we should exclude such tservers. This patch fixes the problem and adds an unit test for this. Change-Id: I2f85a675e69fd02a62e2625881dad2ca5e27acd9 --- M src/kudu/master/auto_leader_rebalancer-test.cc M src/kudu/master/auto_leader_rebalancer.cc M src/kudu/master/auto_leader_rebalancer.h 3 files changed, 96 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/19608/11 -- To view, visit http://gerrit.cloudera.org:8080/19608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2f85a675e69fd02a62e2625881dad2ca5e27acd9 Gerrit-Change-Number: 19608 Gerrit-PatchSet: 11 Gerrit-Owner: Yuqi Du Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Yuqi Du
[kudu-CR] [master] Exclude tservers in MAINTENANCE MODE when leader rebalancing
Yuqi Du has posted comments on this change. (
http://gerrit.cloudera.org:8080/19608 )
Change subject: [master] Exclude tservers in MAINTENANCE_MODE when leader
rebalancing
..
Patch Set 10:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/19608/9//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19608/9//COMMIT_MSG@14
PS9, Line 14: For this reason, we should exclude
: such tservers.
> Having this patch is great, but it seems there is a race condition in this
Yes.
I want to use the 'quiesce state' of tserver to do this at next patch. But this
status is a little different from 'Maintenance mode'
Maintenance mode is a state of kudu-master, tserver don't know it is in
'Maintenance'. So a method extends the state to tserver.
Tserver's quiesce state, I will study it again and then provide a solution to
solve this problem.
http://gerrit.cloudera.org:8080/#/c/19608/9/src/kudu/master/auto_leader_rebalancer-test.cc
File src/kudu/master/auto_leader_rebalancer-test.cc:
http://gerrit.cloudera.org:8080/#/c/19608/9/src/kudu/master/auto_leader_rebalancer-test.cc@234
PS9, Line 234:
: constexpr const int currentTserverIndex = 0;
: tserver::MiniTabletServer* mini_tserver =
cluster_->mini_tablet_server(currentTserverIndex);
: // Sets the tserver state for a tserver to 'MAINTENANCE_MODE'.
: ASSERT_OK(
: master->ts_manager()->SetTServerState(mini_tserver->uuid(),
:
TServerStatePB::MAINTENANCE_MODE,
:
ChangeTServerStateRequestPB::ALLOW_MISSING_TSERVER,
: master->catalog_mana
> +1
Done
http://gerrit.cloudera.org:8080/#/c/19608/9/src/kudu/master/auto_leader_rebalancer-test.cc@251
PS9, Line 251: // it's enough to reach
> Wrap this into ASSERT_OK()?
Done
http://gerrit.cloudera.org:8080/#/c/19608/9/src/kudu/master/auto_leader_rebalancer-test.cc@253
PS9, Line 253: constexpr const int32_t retries = 20;
> nit: please add a comment on the purpose of this pause
// To make sure replica_rebalancer execute some runs and reach balanced.
Because this test case tserver only 3, so replica is balanced, the SleepFor is
not necessary, so remove it.
http://gerrit.cloudera.org:8080/#/c/19608/9/src/kudu/master/auto_leader_rebalancer-test.cc@254
PS9, Line 254: {
> Why 20? Why not 10 or 100?
It's an estimated value.
// Try to run 20 tries 'leader rebalance'. If mini_tserver not in
MAINTENANCE_MODE,
// it's enough to reach leader balanced, more tries is not necessary and less
tries
// may not reach leader rebalanced.
http://gerrit.cloudera.org:8080/#/c/19608/9/src/kudu/master/auto_leader_rebalancer-test.cc@264
PS9, Line 264: tatus.IsIllegalState()) <
> Does it make sense to check for exact Status code? And error message patte
Done
http://gerrit.cloudera.org:8080/#/c/19608/9/src/kudu/master/auto_leader_rebalancer.h
File src/kudu/master/auto_leader_rebalancer.h:
http://gerrit.cloudera.org:8080/#/c/19608/9/src/kudu/master/auto_leader_rebalancer.h@79
PS9, Line 79: std::unordered_set Is it crucial to have the set of UUIDs to be ordered? If not, consider usi
Done
http://gerrit.cloudera.org:8080/#/c/19608/9/src/kudu/master/auto_leader_rebalancer.cc
File src/kudu/master/auto_leader_rebalancer.cc:
http://gerrit.cloudera.org:8080/#/c/19608/9/src/kudu/master/auto_leader_rebalancer.cc@405
PS9, Line 405: CatalogManager::ScopedLeaderSharedLock
leader_lock(catalog_manager_);
> What if one more tablet server is put into the maintenance mode just betwee
Thanks for your advice. The problem exist indeed, unless we use a big lock
ts_manager's SetTServerState when leader rebalancer runs. A big lock is not
proper. So I strengthen the check condition and at most 1 tablet will be
influenced when a new tserver entered MAINTENANCE_MODE in extreme situations .
After this patch, a planning patch is for tserver quiesce state , which often
used by decommission with enter maintenance mode, if a tserver in quiesce state
receive a leader transferring request to it, it should reject the request.
--
To view, visit http://gerrit.cloudera.org:8080/19608
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f85a675e69fd02a62e2625881dad2ca5e27acd9
Gerrit-Change-Number: 19608
Gerrit-PatchSet: 10
Gerrit-Owner: Yuqi Du
Gerrit-Reviewer: Abhishek Chennaka
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: KeDeng
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wang Xixu <1450306...@qq.com>
Gerrit-Reviewer: Yifan Zhang
Gerrit-Reviewer: Yingchun Lai
Gerrit-Reviewer: Yuqi Du
Gerrit-Comment-Date: Thu, 30
[kudu-CR] [master] Exclude tservers in MAINTENANCE MODE when leader rebalancing
Hello Mahesh Reddy, Alexey Serbin, Yingchun Lai, Yifan Zhang, Attila Bukor, Kudu Jenkins, Abhishek Chennaka, KeDeng, Wang Xixu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19608 to look at the new patch set (#10). Change subject: [master] Exclude tservers in MAINTENANCE_MODE when leader rebalancing .. [master] Exclude tservers in MAINTENANCE_MODE when leader rebalancing Currently, auto leader rebalancing may transfer new leaders to tservers which have entered MAINTENANCE_MODE. Because a tserver in MAINTENANCE_MODE is being maintained by administrators, maybe it is decommissioning. Since it might be decommissioned/decommissioning, it doesn't make sense for the leader rebalancer to transfer a leader to a tserver in MAINTENANCE_MODE as leaders serve many users' requests. For this reason, we should exclude such tservers. This patch fixes the problem and adds an unit test for this. Change-Id: I2f85a675e69fd02a62e2625881dad2ca5e27acd9 --- M src/kudu/master/auto_leader_rebalancer-test.cc M src/kudu/master/auto_leader_rebalancer.cc M src/kudu/master/auto_leader_rebalancer.h 3 files changed, 100 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/19608/10 -- To view, visit http://gerrit.cloudera.org:8080/19608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2f85a675e69fd02a62e2625881dad2ca5e27acd9 Gerrit-Change-Number: 19608 Gerrit-PatchSet: 10 Gerrit-Owner: Yuqi Du Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Yuqi Du
[kudu-CR] jwt: Additional test
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19156 ) Change subject: jwt: Additional test .. Patch Set 6: (4 comments) http://gerrit.cloudera.org:8080/#/c/19156/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19156/6//COMMIT_MSG@11 PS6, Line 11: exires nit: expires http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/rpc-test-base.h@56 PS6, Line 56: #include "kudu/util/jwt.h" add it in alphabet order http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/mini_oidc.h File src/kudu/util/mini_oidc.h: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/mini_oidc.h@78 PS6, Line 78: indent http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/mini_oidc.cc File src/kudu/util/mini_oidc.cc: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/mini_oidc.cc@24 PS6, Line 24: #include alphabet order -- To view, visit http://gerrit.cloudera.org:8080/19156 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1977c80b70fd9628ac800671f6cf16e9fa96c0f0 Gerrit-Change-Number: 19156 Gerrit-PatchSet: 6 Gerrit-Owner: Zoltan Chovan Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Wenzhe Zhou Gerrit-Reviewer: Zoltan Chovan Gerrit-Comment-Date: Thu, 30 Mar 2023 08:00:39 + Gerrit-HasComments: Yes
[kudu-CR] [client] Improve logging in client metacache
Hello Yingchun Lai, Kudu Jenkins, Abhishek Chennaka, KeDeng, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/19653 to look at the new patch set (#5). Change subject: [client] Improve logging in client metacache .. [client] Improve logging in client metacache As part for impala crashing issue (caused due to PickLeader getting called recursively for indefinite iterations until stack overflow happens), some extra logs are added to aid in debugging for future issues that may reside in the code path mentioned above. Change-Id: I2a47f643307ef47a98b1d3481b4c03834fa239d4 --- M src/kudu/client/meta_cache.cc 1 file changed, 21 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/19653/5 -- To view, visit http://gerrit.cloudera.org:8080/19653 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2a47f643307ef47a98b1d3481b4c03834fa239d4 Gerrit-Change-Number: 19653 Gerrit-PatchSet: 5 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai
[kudu-CR] [client] Improve logging in client metacache
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/19653 ) Change subject: [client] Improve logging in client metacache .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/19653/4/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: http://gerrit.cloudera.org:8080/#/c/19653/4/src/kudu/client/meta_cache.cc@282 PS4, Line 282: :: > nit : duplicate":" Removed colon and kept just a space to make it uniform with other log message formats. -- To view, visit http://gerrit.cloudera.org:8080/19653 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a47f643307ef47a98b1d3481b4c03834fa239d4 Gerrit-Change-Number: 19653 Gerrit-PatchSet: 4 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Thu, 30 Mar 2023 07:25:06 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 ) Change subject: KUDU-1945 Update auto incrementing counter during bootstrap .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/19445/7/src/kudu/integration-tests/auto_incrementing-itest.cc File src/kudu/integration-tests/auto_incrementing-itest.cc: http://gerrit.cloudera.org:8080/#/c/19445/7/src/kudu/integration-tests/auto_incrementing-itest.cc@281 PS7, Line 281: + nit: space around + -- To view, visit http://gerrit.cloudera.org:8080/19445 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184 Gerrit-Change-Number: 19445 Gerrit-PatchSet: 7 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 30 Mar 2023 06:34:27 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 ) Change subject: KUDU-1945 Update auto incrementing counter during bootstrap .. Patch Set 7: Code-Review+1 (5 comments) It seems IWYU isn't yet happy: >>> Fixing #includes in >>> '/home/jenkins-slave/workspace/kudu-master/0/src/kudu/integration-tests/auto_incrementing-itest.cc' @@ -18,7 +18,6 @@ // Integration test for flexible partitioning (eg buckets, range partitioning // of PK subsets, etc). -#include #include #include #include @@ -35,23 +34,18 @@ #include "kudu/client/write_op.h" #include "kudu/common/common.pb.h" #include "kudu/common/partial_row.h" -#include "kudu/common/row.h" #include "kudu/common/schema.h" #include "kudu/common/wire_protocol.h" -#include "kudu/common/wire_protocol.pb.h" #include "kudu/consensus/metadata.pb.h" #include "kudu/gutil/strings/substitute.h" #include "kudu/mini-cluster/external_mini_cluster.h" #include "kudu/rpc/rpc_controller.h" -#include "kudu/tablet/tablet_replica.h" +#include "kudu/tablet/tablet.pb.h" #include "kudu/tserver/tablet_server-test-base.h" -#include "kudu/tserver/tablet_server.h" -#include "kudu/tserver/ts_tablet_manager.h" #include "kudu/tserver/tserver.pb.h" #include "kudu/util/env_util.h" #include "kudu/util/monotime.h" #include "kudu/util/path_util.h" -#include "kudu/util/slice.h" #include "kudu/util/status.h" #include "kudu/util/test_macros.h" #include "kudu/util/test_util.h" >>> Fixing #includes in >>> '/home/jenkins-slave/workspace/kudu-master/0/src/kudu/tablet/tablet.h' @@ -86,7 +86,6 @@ class AlterSchemaOpState; class CompactionPolicy; -class DiskRowSet; class HistoryGcOpts; class MemRowSet; class ParticipantOpState; IWYU would have edited 2 files on your behalf. http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@21 PS6, Line 21: the > this? missed addressing this one in PS7? http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@23 PS6, Line 23: in memory > nit: in-memory missing addressing this one in PS7? http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc File src/kudu/integration-tests/auto_incrementing-itest.cc: http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@394 PS6, Line 394: ASSERT_EQ(results[i].size(), results[i+1].size()); : for (int k = 0; k < results[i]. > That should be the case, the reason being: It would be great if you could add a comment into the code to explain why this situation is guaranteed to happen. http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@405 PS6, Line 405: > Incomplete comment? Yep, but now I forgot what I was going to comment about :) http://gerrit.cloudera.org:8080/#/c/19445/7/src/kudu/integration-tests/auto_incrementing-itest.cc File src/kudu/integration-tests/auto_incrementing-itest.cc: http://gerrit.cloudera.org:8080/#/c/19445/7/src/kudu/integration-tests/auto_incrementing-itest.cc@190 PS7, Line 190: *tablet_uuid = resp.status_and_schema(0).tablet_status().tablet_id(); Do we need to make sure resp.status_and_schema isn't empty? If you don't want to introduce if() clause, adding CHECK() is OK (not great, but at least there wouldn't reading some junk from random location in the memory). -- To view, visit http://gerrit.cloudera.org:8080/19445 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184 Gerrit-Change-Number: 19445 Gerrit-PatchSet: 7 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Thu, 30 Mar 2023 06:05:51 + Gerrit-HasComments: Yes
