Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/13659 )
Change subject: [maintenance] Add extra config for maintenance manager task priority ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/13659/2/src/kudu/common/common.proto File src/kudu/common/common.proto: PS2: > Do you think we should update all of the tests that xiaokai updated (https: I don't think it's necessary, we should assume that 'table extra config' is a stable feature, not needed to test it again and again. However, functional test for the new added config is needed. http://gerrit.cloudera.org:8080/#/c/13659/2/src/kudu/common/common.proto@449 PS2, Line 449: ance, it will be clamped into : // range [-FLAGS_max_priority_range, FLAGS_max_priority_ran > Hmm, but --max_priority_range can change. What's the expected behavior if i Done http://gerrit.cloudera.org:8080/#/c/13659/2/src/kudu/common/wire_protocol.h File src/kudu/common/wire_protocol.h: http://gerrit.cloudera.org:8080/#/c/13659/2/src/kudu/common/wire_protocol.h@154 PS2, Line 154: Int > Nit: Int32 to be clearer about the size. Done http://gerrit.cloudera.org:8080/#/c/13659/2/src/kudu/common/wire_protocol.cc File src/kudu/common/wire_protocol.cc: http://gerrit.cloudera.org:8080/#/c/13659/2/src/kudu/common/wire_protocol.cc@666 PS2, Line 666: Status ParseInt32Config(const string& name, const string& value, int32_t* result) { > No need for std:: prefixes here. Done http://gerrit.cloudera.org:8080/#/c/13659/2/src/kudu/common/wire_protocol.cc@669 PS2, Line 669: unable > Nit: "unable to..." because we often prepend other messages to a status. Done http://gerrit.cloudera.org:8080/#/c/13659/2/src/kudu/tablet/tablet_mm_ops.cc File src/kudu/tablet/tablet_mm_ops.cc: http://gerrit.cloudera.org:8080/#/c/13659/2/src/kudu/tablet/tablet_mm_ops.cc@96 PS2, Line 96: const auto& extra_config = tablet_->metadata()->extra_config(); : if (extra_config && extra_config->has_maintenance_priority()) { : priority = extra_config->maintenance_priority(); : } > This pattern (and the equivalent in tablet_replica_mm_ops.cc) doesn't seem I'll add an unit test to check OPs priority after alter this extra config, but I don't think it's necessary to repro this race, because there is no race after the prior patch set. -- To view, visit http://gerrit.cloudera.org:8080/13659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I966ee626ef85ce56ba4517b9e494f3ac5b044867 Gerrit-Change-Number: 13659 Gerrit-PatchSet: 3 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: XiaokaiWang <xiaokai.w...@live.com> Gerrit-Reviewer: Yao Xu <ocla...@gmail.com> Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Thu, 11 Jul 2019 04:22:24 +0000 Gerrit-HasComments: Yes
