Todd Lipcon has posted comments on this change.

Change subject: Add Maintenance Manager visualizer
......................................................................


Patch Set 7:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/7570/7/www/maintenance-manager.mustache
File www/maintenance-manager.mustache:

PS7, Line 106: CompactRowSetsOp {
             :   fill: #095;
             : }
             : 
             : .FlushDeltaMemStoresOp {
             :   fill: #ec1;
             : }
             : 
             : .FlushMRSOp {
             :   fill: #16a;
             : }
             : 
             : .LogGCOp {
             :   fill: #2ae;
             : }
             : 
             : .MajorDeltaCompactionOp {
             :   fill: #c23;
             : }
             : 
             : .MinorDeltaCompactionOp {
             :   fill: #92c;
             : }
             : 
             : .UndoDeltaBlockGCOp {
maybe we can fill these in from the template?


Line 173:     } else if (millis < 1000 * 60) {
I think a better threshold here might be 300 or something... otherwise anywhere 
from 61 to 119 seconds will be reported the same, which doesn't seem that great


PS7, Line 180:   // Translates a thread id hex string into a counting-number 
identifier.
             :   function threadString(thread_id) {
             :     return "Thread " + (threads.indexOf(thread_id) + 1);
             :   }
would rather figure a way to show pids here so you can correlate with trace 
results, pstacks, etc


PS7, Line 192: html
shoudl be .text() instead of .html()


Line 192:       .append($("<h5/>").html(op.name))
why not use jQuery(popup).css(...) to set the above styles instead of the 
native DOM manipulation?
also could just use jQuery("#popup") to find the element


PS7, Line 201: var popup = document.getElementById("popup");
             :     popup.innerHTML = "";
             :     popup.style.display = "none";
jQuery("#popup").empty().hide();


PS7, Line 217: (i
var i


PS7, Line 219:  [];
an empty array here seems unexpected since it's used as an array index below


PS7, Line 246:  .
nit: inconsistent indentation


Line 328:   // and a corresponding css class (as above) should be added.
can this be passed in from the code?


-- 
To view, visit http://gerrit.cloudera.org:8080/7570
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If2e7f18ac4834791a94d935b540930b11ea14532
Gerrit-PatchSet: 7
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: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sam Okrent <samuel.okr...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to