You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Sam Okrent (Code Review)" <ge...@cloudera.org> on 2017/07/28 23:14:57 UTC
[kudu-CR] Expose running maintenance op info
Sam Okrent has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7537
Change subject: Expose running maintenance op info
......................................................................
Expose running maintenance op info
Previously, the maintenance manager stored the number of instances
a given operation has running, but information like the start time
of an instance was not made available until the instance completed.
However, the start times of running instances can be helpful
information to display on the web UI.
This commit adds a collection of running instances to the
MaintenanceOp object so that each operation can record its own
running instances. Additionally, instances of operations now
store the id of the thread on which they run.
It also switches the /maintenance-manager page of the tserver
web UI over to a mustache template.
Change-Id: Ide228d7e70deae3ae89d108cbd270f3f0f2580ca
---
M src/kudu/tserver/tserver-path-handlers.cc
M src/kudu/tserver/tserver-path-handlers.h
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
M src/kudu/util/maintenance_manager.proto
A www/maintenance-manager.mustache
7 files changed, 224 insertions(+), 70 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/7537/1
--
To view, visit http://gerrit.cloudera.org:8080/7537
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ide228d7e70deae3ae89d108cbd270f3f0f2580ca
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
[kudu-CR] Expose running maintenance op info
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: Expose running maintenance op info
......................................................................
Patch Set 2:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/7537/2/src/kudu/tserver/tserver-path-handlers.h
File src/kudu/tserver/tserver-path-handlers.h:
PS2, Line 26: #include "kudu/util/easy_json.h"
Should be able to forward declare this.
http://gerrit.cloudera.org:8080/#/c/7537/2/src/kudu/util/maintenance_manager-test.cc
File src/kudu/util/maintenance_manager-test.cc:
PS2, Line 314: manager_->UnregisterOp(&op1);
Why is this necessary?
http://gerrit.cloudera.org:8080/#/c/7537/2/src/kudu/util/maintenance_manager.cc
File src/kudu/util/maintenance_manager.cc:
PS2, Line 513: vector<OpInstance> running_instances;
: op->GetRunningInstances(&running_instances);
May be easier in this case to return the vector directly.
http://gerrit.cloudera.org:8080/#/c/7537/2/src/kudu/util/maintenance_manager.h
File src/kudu/util/maintenance_manager.h:
PS2, Line 149: Holds the information regarding a recently completed operation.
Should this comment be updated?
PS2, Line 207: Note: op_instance is owned by the caller
Ah, I see. In that case, can you also make this note down by the definition of running_instances_?
Also, is there an expected caller besides the MaintenanceManager?
PS2, Line 208: std::thread::id thread_id
nit: const ref here and below?
PS2, Line 214: OpInstance*
Where is this output used?
PS2, Line 216: instances
nit: out_instances
PS2, Line 245: nstances of this op
Who owns the instances? Are there any lifetime guarantees we need to know about?
PS2, Line 245: t they're ru
nit: typo
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Expose running maintenance op info
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has submitted this change and it was merged.
Change subject: Expose running maintenance op info
......................................................................
Expose running maintenance op info
Previously, the maintenance manager stored the number of instances
a given operation has running, but information like the start time
of an instance was not made available until the instance completed.
However, the start times of running instances can be helpful
information to display on the web UI.
This commit adds a collection of running instances to the
MaintenanceManager so the information mentioned above can be
accessed while operations are running. Additionally, instances of
operations now store the id of the thread on which they run.
Change-Id: Ide228d7e70deae3ae89d108cbd270f3f0f2580ca
Reviewed-on: http://gerrit.cloudera.org:8080/7537
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Jean-Daniel Cryans <jd...@apache.org>
---
M src/kudu/tserver/tserver-path-handlers.cc
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
M src/kudu/util/maintenance_manager.proto
5 files changed, 114 insertions(+), 46 deletions(-)
Approvals:
Jean-Daniel Cryans: Looks good to me, approved
Andrew Wong: Looks good to me, but someone else must approve
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7537
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ide228d7e70deae3ae89d108cbd270f3f0f2580ca
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Expose running maintenance op info
Posted by "Sam Okrent (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7537
to look at the new patch set (#2).
Change subject: Expose running maintenance op info
......................................................................
Expose running maintenance op info
Previously, the maintenance manager stored the number of instances
a given operation has running, but information like the start time
of an instance was not made available until the instance completed.
However, the start times of running instances can be helpful
information to display on the web UI.
This commit adds a collection of running instances to the
MaintenanceOp object so that each operation can record its own
running instances. Additionally, instances of operations now
store the id of the thread on which they run.
It also switches the /maintenance-manager page of the tserver
web UI over to a mustache template.
Change-Id: Ide228d7e70deae3ae89d108cbd270f3f0f2580ca
---
M src/kudu/tserver/tserver-path-handlers.cc
M src/kudu/tserver/tserver-path-handlers.h
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
M src/kudu/util/maintenance_manager.proto
A www/maintenance-manager.mustache
7 files changed, 225 insertions(+), 71 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/7537/2
--
To view, visit http://gerrit.cloudera.org:8080/7537
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide228d7e70deae3ae89d108cbd270f3f0f2580ca
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Expose running maintenance op info
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: Expose running maintenance op info
......................................................................
Patch Set 8: Code-Review+1
--
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: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Expose running maintenance op info
Posted by "Sam Okrent (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7537
to look at the new patch set (#7).
Change subject: Expose running maintenance op info
......................................................................
Expose running maintenance op info
Previously, the maintenance manager stored the number of instances
a given operation has running, but information like the start time
of an instance was not made available until the instance completed.
However, the start times of running instances can be helpful
information to display on the web UI.
This commit adds a collection of running instances to the
MaintenanceManager so the information mentioned above can be
accessed while operations are running. Additionally, instances of
operations now store the id of the thread on which they run.
Change-Id: Ide228d7e70deae3ae89d108cbd270f3f0f2580ca
---
M src/kudu/tserver/tserver-path-handlers.cc
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
M src/kudu/util/maintenance_manager.proto
5 files changed, 114 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/7537/7
--
To view, visit http://gerrit.cloudera.org:8080/7537
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide228d7e70deae3ae89d108cbd270f3f0f2580ca
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Expose running maintenance op info
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Expose running maintenance op info
......................................................................
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7537/4/src/kudu/tserver/tserver-path-handlers.cc
File src/kudu/tserver/tserver-path-handlers.cc:
PS4, Line 629: op_pb.duration_millis() / 1000.0),
: HumanReadableElapsedTime::ToShortString(
: op_pb.millis_since_start()));
why is one of these /1000 and the other not?
http://gerrit.cloudera.org:8080/#/c/7537/3/src/kudu/util/maintenance_manager.h
File src/kudu/util/maintenance_manager.h:
Line 153: // Name of operation.
> Added a sentinel value and a comment.
MonoDelta's default constructor already puts it into an "uninitialized" state so you don't need a special sentinel here. You can just use .Initialized() to check if it has been initialized.
Line 319:
> This was actually preexisting, I just moved it down a couple lines. Do you
ah, that's ok then
http://gerrit.cloudera.org:8080/#/c/7537/4/src/kudu/util/maintenance_manager.h
File src/kudu/util/maintenance_manager.h:
PS4, Line 160:
nit: indentation is off
Line 162: pb->set_thread_id(ss.str());
ah, I see that std::to_string won't work here. But if this is an opaque number that isn't really useful to users, I think we'd be better off using Thread::current_thread()->name()
--
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Expose running maintenance op info
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: Expose running maintenance op info
......................................................................
Patch Set 7:
(2 comments)
Couple more nits.
http://gerrit.cloudera.org:8080/#/c/7537/7/src/kudu/tserver/tserver-path-handlers.cc
File src/kudu/tserver/tserver-path-handlers.cc:
PS7, Line 52: using kudu::MaintenanceManagerStatusPB_OpInstancePB;
nit: fix ordering
http://gerrit.cloudera.org:8080/#/c/7537/7/src/kudu/util/maintenance_manager-test.cc
File src/kudu/util/maintenance_manager-test.cc:
PS7, Line 307: MaintenanceManagerStatusPB status_pb;
: manager_->GetMaintenanceManagerStatusDump(&status_pb);
: ASSERT_EQ(status_pb.running_operations_size(), 2);
: const MaintenanceManagerStatusPB_OpInstancePB& instance1 = status_pb.running_operations(0);
: const MaintenanceManagerStatusPB_OpInstancePB& instance2 = status_pb.running_operations(1);
: ASSERT_EQ(instance1.name(), op.name());
: ASSERT_NE(instance1.thread_id(), instance2.thread_id());
:
nit: can you move this back a couple of spaces?
--
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: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Expose running maintenance op info
Posted by "Sam Okrent (Code Review)" <ge...@cloudera.org>.
Sam Okrent has posted comments on this change.
Change subject: Expose running maintenance op info
......................................................................
Patch Set 2:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/7537/2/src/kudu/tserver/tserver-path-handlers.h
File src/kudu/tserver/tserver-path-handlers.h:
PS2, Line 26: #include "kudu/util/easy_json.h"
> Should be able to forward declare this.
Done
http://gerrit.cloudera.org:8080/#/c/7537/2/src/kudu/util/maintenance_manager-test.cc
File src/kudu/util/maintenance_manager-test.cc:
PS2, Line 314: manager_->UnregisterOp(&op1);
> Why is this necessary?
The next assertion checks that running instances are removed from the collection after completion, so this call waits for the instances to complete. I can leave a comment saying so.
http://gerrit.cloudera.org:8080/#/c/7537/2/src/kudu/util/maintenance_manager.cc
File src/kudu/util/maintenance_manager.cc:
PS2, Line 513: vector<OpInstance> running_instances;
: op->GetRunningInstances(&running_instances);
> May be easier in this case to return the vector directly.
Done
http://gerrit.cloudera.org:8080/#/c/7537/2/src/kudu/util/maintenance_manager.h
File src/kudu/util/maintenance_manager.h:
PS2, Line 149: Holds the information regarding a recently completed operation.
> Should this comment be updated?
Done
PS2, Line 207: Note: op_instance is owned by the caller
> Ah, I see. In that case, can you also make this note down by the definition
Done
PS2, Line 208: std::thread::id thread_id
> nit: const ref here and below?
Done
PS2, Line 214: OpInstance*
> Where is this output used?
Done
PS2, Line 216: instances
> nit: out_instances
Done
PS2, Line 245: nstances of this op
> Who owns the instances? Are there any lifetime guarantees we need to know a
The instances are created by the caller of AddRunningInstance(), and need to exist until RemoveRunningInstance() is called with the same thread_id. The only way it happens in the code right now is that the same thread calls both methods within the same function, and the instance is allocated by that thread on the stack, and that's probably the only scenario that makes sense. So I can update the comments to reflect that and change the return type of RemoveRunningInstance to void.
PS2, Line 245: t they're ru
> nit: typo
Maps thread ids to instances of this op that they (the threads) are running? Is there a problem with that?
PS2, Line 246: std::map<std::thread::id, OpInstance*> running_instances_;
: Mutex running_instances_lock_;
> hrm, instead of adding them to MaintenanceOp, why not just have this map be
So this way requires taking the Maintenance Manager lock before calling Perform() in LaunchOp(), which I was hoping to avoid because it might slow things down. So I was trying to get around that by adding a kind of atomic map in each MaintenanceOp that could use a different lock, but thinking about it again that doesn't make a ton of sense since the same thing could be accomplished by adding the running_instances_ map and lock to the MaintenanceManager itself. So the design you mentioned sounds a lot better.
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Expose running maintenance op info
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Expose running maintenance op info
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon 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_operations?
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 MakeScopedCleanup to ensure that all this stuff happens (decrementing the running count, removing from the map, registering in the histogram).
Otherwise there's some risk that if someone adds an early return above we'll orphan a pointer to a stack-variable in the map, have 'running_ops_' set wrong forever, etc.
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)) ?
Is the thread-id human-interpretable? Or is there a better name we should be using to identify the thread? I'm not sure what you get when you stringify the std::thread::thread_id
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() - running_op->start_mono_time
Line 517: completed_pb->set_thread_id(ss.str());
same as above
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? extract a function for it?
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?
PS3, Line 318: OpInstance*>
since this is a raw pointer it's worth noting who is the "owner" here of the values
Line 319: uint64_t running_ops_;
use a signed 'int' here since it's easier to DCHECK_GE(running_ops_, 0)
https://google.github.io/styleguide/cppguide.html#Integer_Types
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Expose running maintenance op info
Posted by "Sam Okrent (Code Review)" <ge...@cloudera.org>.
Sam Okrent has posted comments on this change.
Change subject: Expose running maintenance op info
......................................................................
Patch Set 5:
(4 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
Done
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?
Done
PS5, Line 332: std::map<std::thread::id, OpInstance*> running_instances_;
> Maybe mention locking requirements?
Done
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 inst
Done
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Expose running maintenance op info
Posted by "Sam Okrent (Code Review)" <ge...@cloudera.org>.
Sam Okrent has posted comments on this change.
Change subject: Expose running maintenance op info
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7537/4/src/kudu/tserver/tserver-path-handlers.cc
File src/kudu/tserver/tserver-path-handlers.cc:
PS4, Line 629: op_pb.duration_millis() / 1000.0),
: HumanReadableElapsedTime::ToShortString(
: op_pb.millis_since_start()));
> why is one of these /1000 and the other not?
Ah, because this all changes in the next commit and I was being lazy. FIxing it.
http://gerrit.cloudera.org:8080/#/c/7537/4/src/kudu/util/maintenance_manager.h
File src/kudu/util/maintenance_manager.h:
PS4, Line 160:
> nit: indentation is off
Done
Line 162: pb->set_thread_id(ss.str());
> ah, I see that std::to_string won't work here. But if this is an opaque num
So it looks to me like every thread in the Maintenance Manager thread pool gets assigned the same name ("MaintenanceMgr [worker]"), and the thread _id's need to be unique. Do you think it's a problem to leave it as it is and then translate the id's to a sequence of natural numbers when they're displayed on the web UI?
--
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Expose running maintenance op info
Posted by "Sam Okrent (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7537
to look at the new patch set (#8).
Change subject: Expose running maintenance op info
......................................................................
Expose running maintenance op info
Previously, the maintenance manager stored the number of instances
a given operation has running, but information like the start time
of an instance was not made available until the instance completed.
However, the start times of running instances can be helpful
information to display on the web UI.
This commit adds a collection of running instances to the
MaintenanceManager so the information mentioned above can be
accessed while operations are running. Additionally, instances of
operations now store the id of the thread on which they run.
Change-Id: Ide228d7e70deae3ae89d108cbd270f3f0f2580ca
---
M src/kudu/tserver/tserver-path-handlers.cc
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
M src/kudu/util/maintenance_manager.proto
5 files changed, 114 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/7537/8
--
To view, visit http://gerrit.cloudera.org:8080/7537
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide228d7e70deae3ae89d108cbd270f3f0f2580ca
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Expose running maintenance op info
Posted by "Sam Okrent (Code Review)" <ge...@cloudera.org>.
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 <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Expose running maintenance op info
Posted by "Sam Okrent (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7537
to look at the new patch set (#3).
Change subject: Expose running maintenance op info
......................................................................
Expose running maintenance op info
Previously, the maintenance manager stored the number of instances
a given operation has running, but information like the start time
of an instance was not made available until the instance completed.
However, the start times of running instances can be helpful
information to display on the web UI.
This commit adds a collection of running instances to the
MaintenanceManager so the information mentioned above can be
accessed while operations are running. Additionally, instances of
operations now store the id of the thread on which they run.
It also switches the /maintenance-manager page of the tserver
web UI over to a mustache template.
Change-Id: Ide228d7e70deae3ae89d108cbd270f3f0f2580ca
---
M src/kudu/tserver/tserver-path-handlers.cc
M src/kudu/tserver/tserver-path-handlers.h
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
M src/kudu/util/maintenance_manager.proto
A www/maintenance-manager.mustache
7 files changed, 192 insertions(+), 71 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/7537/3
--
To view, visit http://gerrit.cloudera.org:8080/7537
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide228d7e70deae3ae89d108cbd270f3f0f2580ca
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Expose running maintenance op info
Posted by "Sam Okrent (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7537
to look at the new patch set (#6).
Change subject: Expose running maintenance op info
......................................................................
Expose running maintenance op info
Previously, the maintenance manager stored the number of instances
a given operation has running, but information like the start time
of an instance was not made available until the instance completed.
However, the start times of running instances can be helpful
information to display on the web UI.
This commit adds a collection of running instances to the
MaintenanceManager so the information mentioned above can be
accessed while operations are running. Additionally, instances of
operations now store the id of the thread on which they run.
Change-Id: Ide228d7e70deae3ae89d108cbd270f3f0f2580ca
---
M src/kudu/tserver/tserver-path-handlers.cc
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
M src/kudu/util/maintenance_manager.proto
5 files changed, 114 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/7537/6
--
To view, visit http://gerrit.cloudera.org:8080/7537
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide228d7e70deae3ae89d108cbd270f3f0f2580ca
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Expose running maintenance op info
Posted by "Sam Okrent (Code Review)" <ge...@cloudera.org>.
Sam Okrent has posted comments on this change.
Change subject: Expose running maintenance op info
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7537/7/src/kudu/tserver/tserver-path-handlers.cc
File src/kudu/tserver/tserver-path-handlers.cc:
PS7, Line 52: using kudu::MaintenanceManagerStatusPB_OpInstancePB;
> nit: fix ordering
Done
http://gerrit.cloudera.org:8080/#/c/7537/7/src/kudu/util/maintenance_manager-test.cc
File src/kudu/util/maintenance_manager-test.cc:
PS7, Line 307: MaintenanceManagerStatusPB status_pb;
: manager_->GetMaintenanceManagerStatusDump(&status_pb);
: ASSERT_EQ(status_pb.running_operations_size(), 2);
: const MaintenanceManagerStatusPB_OpInstancePB& instance1 = status_pb.running_operations(0);
: const MaintenanceManagerStatusPB_OpInstancePB& instance2 = status_pb.running_operations(1);
: ASSERT_EQ(instance1.name(), op.name());
: ASSERT_NE(instance1.thread_id(), instance2.thread_id());
:
> nit: can you move this back a couple of spaces?
The other ASSERT_EVENTUALLY blocks are formatted like this, so it does follow the style of the file.
--
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: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Expose running maintenance op info
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Expose running maintenance op info
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7537/2/src/kudu/util/maintenance_manager.h
File src/kudu/util/maintenance_manager.h:
PS2, Line 246: std::map<std::thread::id, OpInstance*> running_instances_;
: Mutex running_instances_lock_;
hrm, instead of adding them to MaintenanceOp, why not just have this map be resident in the MaintenanceManager itself?
There are potentially many thousands of MaintenanceOp objects, but only one MaintenanceManager, so it seems like a simpler lifecycle to keep the running (and past) ops in there, potentially with a pointer to the MaintenanceOp and whatever other info you want to record about its status, etc
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Expose running maintenance op info
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Expose running maintenance op info
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7537/2/src/kudu/util/maintenance_manager.h
File src/kudu/util/maintenance_manager.h:
PS2, Line 245: nstances of this op
> The instances are created by the caller of AddRunningInstance(), and need t
yea, I wouldn't worry about contention on that lock, since you'd only be holding it while adding/removing entries (not while _running_ the op). Given that we only start/finish ops a few times per second (maybe 10-20 if you have lots of threads) it's still super low-contention as far as these things go.
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Expose running maintenance op info
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: Expose running maintenance op info
......................................................................
Patch Set 8: Code-Review+2
--
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: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Expose running maintenance op info
Posted by "Sam Okrent (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7537
to look at the new patch set (#4).
Change subject: Expose running maintenance op info
......................................................................
Expose running maintenance op info
Previously, the maintenance manager stored the number of instances
a given operation has running, but information like the start time
of an instance was not made available until the instance completed.
However, the start times of running instances can be helpful
information to display on the web UI.
This commit adds a collection of running instances to the
MaintenanceManager so the information mentioned above can be
accessed while operations are running. Additionally, instances of
operations now store the id of the thread on which they run.
It also switches the /maintenance-manager page of the tserver
web UI over to a mustache template.
Change-Id: Ide228d7e70deae3ae89d108cbd270f3f0f2580ca
---
M src/kudu/tserver/tserver-path-handlers.cc
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
M src/kudu/util/maintenance_manager.proto
5 files changed, 109 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/7537/4
--
To view, visit http://gerrit.cloudera.org:8080/7537
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide228d7e70deae3ae89d108cbd270f3f0f2580ca
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Expose running maintenance op info
Posted by "Sam Okrent (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7537
to look at the new patch set (#5).
Change subject: Expose running maintenance op info
......................................................................
Expose running maintenance op info
Previously, the maintenance manager stored the number of instances
a given operation has running, but information like the start time
of an instance was not made available until the instance completed.
However, the start times of running instances can be helpful
information to display on the web UI.
This commit adds a collection of running instances to the
MaintenanceManager so the information mentioned above can be
accessed while operations are running. Additionally, instances of
operations now store the id of the thread on which they run.
Change-Id: Ide228d7e70deae3ae89d108cbd270f3f0f2580ca
---
M src/kudu/tserver/tserver-path-handlers.cc
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
M src/kudu/util/maintenance_manager.proto
5 files changed, 111 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/7537/5
--
To view, visit http://gerrit.cloudera.org:8080/7537
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide228d7e70deae3ae89d108cbd270f3f0f2580ca
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>