Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18169 )
Change subject: [tool] fix a command bug, cmd: kudu wal dump ... ...................................................................... Patch Set 3: (11 comments) done http://gerrit.cloudera.org:8080/#/c/18169/2/src/kudu/common/wire_protocol-test-util.h File src/kudu/common/wire_protocol-test-util.h: http://gerrit.cloudera.org:8080/#/c/18169/2/src/kudu/common/wire_protocol-test-util.h@40 PS2, Line 40: co > nit: this can be const Done http://gerrit.cloudera.org:8080/#/c/18169/2/src/kudu/common/wire_protocol-test-util.h@41 PS2, Line 41: nst > nit: const auto&? Done http://gerrit.cloudera.org:8080/#/c/18169/2/src/kudu/common/wire_protocol-test-util.h@41 PS2, Line 41: f > nit: remove two spaces here Done http://gerrit.cloudera.org:8080/#/c/18169/2/src/kudu/common/wire_protocol-test-util.h@58 PS2, Line 58: {{"string_v > nit: you should be able to instantiate this inline as "...&row, { { "string Done http://gerrit.cloudera.org:8080/#/c/18169/2/src/kudu/common/wire_protocol-test-util.h@65 PS2, Line 65: const Schema& schema, : > nit: align spaces with the ( Done http://gerrit.cloudera.org:8080/#/c/18169/2/src/kudu/common/wire_protocol-test-util.h@77 PS2, Line 77: {{"string_v > nit: same here Done http://gerrit.cloudera.org:8080/#/c/18169/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/18169/2/src/kudu/tools/kudu-tool-test.cc@2119 PS2, Line 2119: Schema schema(GetSimpleTestSchema()); : Schema schema_with_ids(SchemaBuilder(schema).Build()); > nit: since neither of these are const, we just use more standard variable n Done http://gerrit.cloudera.org:8080/#/c/18169/2/src/kudu/tools/kudu-tool-test.cc@2142 PS2, Line 2142: > nit: remove a couple spaces here, and elsewhere. Done http://gerrit.cloudera.org:8080/#/c/18169/2/src/kudu/tools/tool_action_common.cc File src/kudu/tools/tool_action_common.cc: http://gerrit.cloudera.org:8080/#/c/18169/2/src/kudu/tools/tool_action_common.cc@357 PS2, Line 357: if (!replicate.has_alter_schema_request()) { > Would it be better to return an error status instead of core? Done http://gerrit.cloudera.org:8080/#/c/18169/2/src/kudu/tools/tool_action_common.cc@358 PS2, Line 358: LOG(ERROR) << "read an ALTER_SCHEMA_OP log entry, but has no alter_schema_request"; > Why not use tablet_schema directly? tablet_schema is not a new object. directly use tablet_schema, the result is not correct. http://gerrit.cloudera.org:8080/#/c/18169/2/src/kudu/tools/tool_action_common.cc@359 PS2, Line 359: return Status::RuntimeError("ALTER_SCHEMA_OP log entry has no alter_schema_request"); > Return the error status of SchemaFromPB? Done -- To view, visit http://gerrit.cloudera.org:8080/18169 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27acc71597d038cafbbe687117bddb1ce16576c0 Gerrit-Change-Number: 18169 Gerrit-PatchSet: 3 Gerrit-Owner: Yuqi Du <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Sat, 29 Jan 2022 08:35:19 +0000 Gerrit-HasComments: Yes
