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 <cstdint> #include <memory> #include <ostream> #include <string> @@ -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 <achenn...@cloudera.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <greber...@gmail.com> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Thu, 30 Mar 2023 06:05:51 +0000 Gerrit-HasComments: Yes
