Andrew Wong has posted comments on this change. Change subject: Expose running maintenance op info ......................................................................
Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/7537/5/src/kudu/util/maintenance_manager.cc File src/kudu/util/maintenance_manager.cc: PS5, Line 501: MaintenanceManagerStatusPB_OpInstancePB* running_pb = out_pb->add_running_operations(); : running_op->DumpToPB(running_pb); How about making DumpToPB return the pb directly and maybe changing this to something like: *out_pb->add_running_operations() = std::move(running_op->DumpToPB()) ? Same below. http://gerrit.cloudera.org:8080/#/c/7537/2/src/kudu/util/maintenance_manager.h File src/kudu/util/maintenance_manager.h: PS2, Line 245: > Maps thread ids to instances of this op that they (the threads) are running Ah my mistake, misread that. http://gerrit.cloudera.org:8080/#/c/7537/5/src/kudu/util/maintenance_manager.h File src/kudu/util/maintenance_manager.h: PS5, Line 155: Value is negative if instance is still running. Can you add a similar comment to the .proto? PS5, Line 332: std::map<std::thread::id, OpInstance*> running_instances_; Maybe mention locking requirements? http://gerrit.cloudera.org:8080/#/c/7537/5/src/kudu/util/maintenance_manager.proto File src/kudu/util/maintenance_manager.proto: PS5, Line 37: required int32 duration_millis = 3; Did you consider making this optional in the case of currently-running instances? Might not be necessary, but might be worth thinking about if you haven't. -- To view, visit http://gerrit.cloudera.org:8080/7537 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ide228d7e70deae3ae89d108cbd270f3f0f2580ca Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sam Okrent <samuel.okr...@cloudera.com> Gerrit-Reviewer: Andrew Wong <andrew.w...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sam Okrent <samuel.okr...@cloudera.com> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
