[kudu-CR] Expose running maintenance op info

[Andrew Wong (Code Review)]

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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Expose running maintenance op info

[Sam Okrent (Code Review)]

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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Expose running maintenance op info

[Sam Okrent (Code Review)]

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(_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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Expose running maintenance op info

[Andrew Wong (Code Review)]

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(_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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Expose running maintenance op info

[Sam Okrent (Code Review)]

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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Expose running maintenance op info

[Sam Okrent (Code Review)]

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 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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Expose running maintenance op info

[Sam Okrent (Code Review)]

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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Expose running maintenance op info

[Andrew Wong (Code Review)]

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 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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Expose running maintenance op info

[Sam Okrent (Code Review)]

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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Expose running maintenance op info

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Expose running maintenance op info

[Sam Okrent (Code Review)]

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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Expose running maintenance op info

[Sam Okrent (Code Review)]

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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Expose running maintenance op info

[Sam Okrent (Code Review)]

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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Expose running maintenance op info

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Expose running maintenance op info

[Sam Okrent (Code Review)]

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();
> 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 running_instances;
 :   op->GetRunningInstances(_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 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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Expose running maintenance op info

[Todd Lipcon (Code Review)]

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 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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Expose running maintenance op info

[Andrew Wong (Code Review)]

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();
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 running_instances;
 :   op->GetRunningInstances(_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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Expose running maintenance op info

[Sam Okrent (Code Review)]

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 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Expose running maintenance op info

[Sam Okrent (Code Review)]

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