Sam Okrent has posted comments on this change. Change subject: Expose running maintenance op info ......................................................................
Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/7537/3/src/kudu/tserver/tserver-path-handlers.cc File src/kudu/tserver/tserver-path-handlers.cc: Line 623: EasyJson completed_ops = output->Set("completed_operations", EasyJson::kArray); > hrm, shouldn't you have a loop like this which also goes over pb.running_op Yep I fixed that in the next commit. This should really all be in the next commit, I'm moving it. http://gerrit.cloudera.org:8080/#/c/7537/3/src/kudu/util/maintenance_manager.cc File src/kudu/util/maintenance_manager.cc: Line 456: running_instances_.erase(thread_id); > Sort of a pre-existing condition, I think it would be safer to use MakeScop Done PS3, Line 499: std::stringstream ss; : ss << running_op->thread_id; : running_pb->set_thread_id(ss.str()); > how about just set_thread_id(std::to_string(running_op->thread_id)) ? std::thread::id is actually a lightweight class, not a number, so i I understand correctly std::to_string() isn't an option. It stringifies to a hex string. I could just call each thread "Thread 1", "Thread 2", etc., and that conversion could happen in frontend js when they're being displayed, or right here. Line 504: MonoDelta delta(MonoTime::Now().GetDeltaSince(running_op->start_mono_time)); > I think we overloaded operator- here so you can just do MonoTime::Now() - r Done Line 517: completed_pb->set_thread_id(ss.str()); > same as above Done Line 522: completed_pb->set_millis_since_start(delta.ToMilliseconds()); > all the code in this block duplicates the code in the for loop above, right Done http://gerrit.cloudera.org:8080/#/c/7537/3/src/kudu/util/maintenance_manager.h File src/kudu/util/maintenance_manager.h: Line 153: MonoDelta duration; > what's this in the case that it is still running? Added a sentinel value and a comment. PS3, Line 318: OpInstance*> > since this is a raw pointer it's worth noting who is the "owner" here of th Done Line 319: uint64_t running_ops_; > use a signed 'int' here since it's easier to DCHECK_GE(running_ops_, 0) This was actually preexisting, I just moved it down a couple lines. Do you still want me to change it? -- 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: 3 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
