Adar Dembo 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 2: (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://gerrit.cloudera.org/c/13796) every time a new extra config is added? http://gerrit.cloudera.org:8080/#/c/13659/2/src/kudu/common/common.proto@449 PS2, Line 449: valid priority level is ranged : // in [-FLAGS_max_priority_range, FLAGS_max_priority_range] Hmm, but --max_priority_range can change. What's the expected behavior if it's changed such that maintenance_priority is no longer within the range? Looking at the rest of the patch, it looks like we clamp maintenance_priority to the range defined by [-range, range]. Perhaps you could rephrase this comment to explain that? 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. 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 ParseIntConfig(const std::string& name, const std::string& value, int32_t* result) { No need for std:: prefixes here. 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. 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: if (tablet_->metadata()->extra_config() && : tablet_->metadata()->extra_config()->has_maintenance_priority()) { : priority = tablet_->metadata()->extra_config()->maintenance_priority(); : } This pattern (and the equivalent in tablet_replica_mm_ops.cc) doesn't seem correct from a synchronization perspective. There are three accesses to extra_config() and each one acquires TabletMetadata.data_lock_, which means the returned value could be different each time, reflecting the results of a concurrent AlterSchema. Seems like the right thing would be to call extra_config once, store a copy of the extra config locally, then operate on it. It's also a little concerning that this wasn't surfaced in a unit test. Could you try to repro this race with the code as-is via a new test? -- 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: 2 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Reviewer: Yao Xu <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Wed, 10 Jul 2019 04:23:02 +0000 Gerrit-HasComments: Yes
