Re: Review Request 13606: Added a Top abstraction to the webui.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13606/ --- (Updated Jan. 22, 2014, 9:41 p.m.) Review request for mesos, Benjamin Hindman and Vinod Kone. Changes --- Rebase. Thanks for the reviews guys! Did a final test to ensure this works correctly. Let's get the additional cleanup done after we get this in, given the long-standing nature of this change. :) Repository: mesos-git Description --- This adds a Top abstraction to the webui for periodically polling the monitoring endpoint. Instead of having each controller have to deal with monitoring information during the update cycle, this allows each controller to setup asynchronous monitoring updates, stopping it when the controller becomes inactive. Diffs (updated) - src/webui/master/static/js/controllers.js 01fe64ca6b784b210a5687f4e1cda60cead8671d src/webui/master/static/js/services.js 122519a5a4de93edf1fd7a5256e565cc78c59670 src/webui/master/static/slave.html cf927dc9503556e2b01b2fec98231aa629938d34 src/webui/master/static/slave_executor.html 5c349c1ecf4c8d5e54c0e0dc4b315f8a241605c5 src/webui/master/static/slave_framework.html af52cf9f2a6e3f4d586b4890a0a49a8c353c0d80 Diff: https://reviews.apache.org/r/13606/diff/ Testing --- Ran the long-lived-framework. Thanks, Ben Mahler
Re: Review Request 13606: Added a Top abstraction to the webui.
On Jan. 17, 2014, 6:24 p.m., Thomas Rampelberg wrote: src/webui/master/static/slave.html, line 141 https://reviews.apache.org/r/13606/diff/2/?file=423537#file423537line141 Having to look up something from a root object seems like the wrong way to do things. Maybe change the ng-repeat to fetch from both monitor and frameworks (doing an _.extend on the data) at the same time? Ross Allen wrote: Maybe the controller should set a scope `statistics` variable that is `monitor.frameworks[framework.id].statistics` so the view doesn't need to repeat that lookup. The sidebar might also be appropriate as a directive. Thomas Rampelberg wrote: Good call, that would allow inheritance and reduce the code that's showing up in views. Hey Ross, are you ok with me leaving this as a TODO to cleanup later? - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13606/#review32159 --- On Jan. 15, 2014, 2:30 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13606/ --- (Updated Jan. 15, 2014, 2:30 a.m.) Review request for mesos, Benjamin Hindman and Vinod Kone. Repository: mesos-git Description --- This adds a Top abstraction to the webui for periodically polling the monitoring endpoint. Instead of having each controller have to deal with monitoring information during the update cycle, this allows each controller to setup asynchronous monitoring updates, stopping it when the controller becomes inactive. Diffs - src/webui/master/static/js/controllers.js 01fe64ca6b784b210a5687f4e1cda60cead8671d src/webui/master/static/js/services.js 122519a5a4de93edf1fd7a5256e565cc78c59670 src/webui/master/static/slave.html 134aa0b10a232a37654a4ef9ac4bb149dbbebdea src/webui/master/static/slave_executor.html 81c10cbf4dd77f65dd8c3081281740c2be1e5b56 src/webui/master/static/slave_framework.html 04a041e9a4e8e1364617d09412f0a81a160ee48a Diff: https://reviews.apache.org/r/13606/diff/ Testing --- Ran the long-lived-framework. Thanks, Ben Mahler
Re: Review Request 13606: Added a Top abstraction to the webui.
On Jan. 17, 2014, 6:24 p.m., Thomas Rampelberg wrote: src/webui/master/static/slave.html, line 141 https://reviews.apache.org/r/13606/diff/2/?file=423537#file423537line141 Having to look up something from a root object seems like the wrong way to do things. Maybe change the ng-repeat to fetch from both monitor and frameworks (doing an _.extend on the data) at the same time? Maybe the controller should set a scope `statistics` variable that is `monitor.frameworks[framework.id].statistics` so the view doesn't need to repeat that lookup. The sidebar might also be appropriate as a directive. - Ross --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13606/#review32159 --- On Jan. 15, 2014, 2:30 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13606/ --- (Updated Jan. 15, 2014, 2:30 a.m.) Review request for mesos, Benjamin Hindman and Vinod Kone. Repository: mesos-git Description --- This adds a Top abstraction to the webui for periodically polling the monitoring endpoint. Instead of having each controller have to deal with monitoring information during the update cycle, this allows each controller to setup asynchronous monitoring updates, stopping it when the controller becomes inactive. Diffs - src/webui/master/static/js/controllers.js 01fe64ca6b784b210a5687f4e1cda60cead8671d src/webui/master/static/js/services.js 122519a5a4de93edf1fd7a5256e565cc78c59670 src/webui/master/static/slave.html 134aa0b10a232a37654a4ef9ac4bb149dbbebdea src/webui/master/static/slave_executor.html 81c10cbf4dd77f65dd8c3081281740c2be1e5b56 src/webui/master/static/slave_framework.html 04a041e9a4e8e1364617d09412f0a81a160ee48a Diff: https://reviews.apache.org/r/13606/diff/ Testing --- Ran the long-lived-framework. Thanks, Ben Mahler
Re: Review Request 13606: Added a Top abstraction to the webui.
On Jan. 17, 2014, 6:24 p.m., Thomas Rampelberg wrote:
src/webui/master/static/js/services.js, line 110
https://reviews.apache.org/r/13606/diff/2/?file=423536#file423536line110
This could be something like:
_.each([ 'cpus_user_time_secs' ], function(n) { this[n] = 0.0 });
Ben Mahler wrote:
Are you suggesting we keep a list of these metric names somewhere and
operate on them? I'm inclined to directly set them here as it's a little
easier for others to read.
This is totally a style comment. The way they are right now is totally fine, I
just personally prefer to abstract the assignment out a little bit.
On Jan. 17, 2014, 6:24 p.m., Thomas Rampelberg wrote:
src/webui/master/static/js/services.js, line 121
https://reviews.apache.org/r/13606/diff/2/?file=423536#file423536line121
Because the parameters are the same here, why not just do this:
_.each(this.metrics, function(v, name) {
this.metrics[name] += stats[name];
}
Ben Mahler wrote:
Does this work if new metrics are added from the backend and we didn't
capture them above in our initialization?
It will have undefined behavior. By using the same list of variables that the
initializer is using, however, you'll get guaranteed behavior (both in
initialization and assignment).
On Jan. 17, 2014, 6:24 p.m., Thomas Rampelberg wrote:
src/webui/master/static/slave.html, line 141
https://reviews.apache.org/r/13606/diff/2/?file=423537#file423537line141
Having to look up something from a root object seems like the wrong way
to do things. Maybe change the ng-repeat to fetch from both monitor and
frameworks (doing an _.extend on the data) at the same time?
Ross Allen wrote:
Maybe the controller should set a scope `statistics` variable that is
`monitor.frameworks[framework.id].statistics` so the view doesn't need to
repeat that lookup. The sidebar might also be appropriate as a directive.
Good call, that would allow inheritance and reduce the code that's showing up
in views.
- Thomas
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13606/#review32159
---
On Jan. 15, 2014, 2:30 a.m., Ben Mahler wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13606/
---
(Updated Jan. 15, 2014, 2:30 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Repository: mesos-git
Description
---
This adds a Top abstraction to the webui for periodically polling the
monitoring endpoint.
Instead of having each controller have to deal with monitoring information
during the update cycle, this allows each controller to setup asynchronous
monitoring updates, stopping it when the controller becomes inactive.
Diffs
-
src/webui/master/static/js/controllers.js
01fe64ca6b784b210a5687f4e1cda60cead8671d
src/webui/master/static/js/services.js
122519a5a4de93edf1fd7a5256e565cc78c59670
src/webui/master/static/slave.html 134aa0b10a232a37654a4ef9ac4bb149dbbebdea
src/webui/master/static/slave_executor.html
81c10cbf4dd77f65dd8c3081281740c2be1e5b56
src/webui/master/static/slave_framework.html
04a041e9a4e8e1364617d09412f0a81a160ee48a
Diff: https://reviews.apache.org/r/13606/diff/
Testing
---
Ran the long-lived-framework.
Thanks,
Ben Mahler
Re: Review Request 13606: Added a Top abstraction to the webui.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13606/#review32148 --- Ship it! Ship It! - Ross Allen On Jan. 15, 2014, 2:30 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13606/ --- (Updated Jan. 15, 2014, 2:30 a.m.) Review request for mesos, Benjamin Hindman and Vinod Kone. Repository: mesos-git Description --- This adds a Top abstraction to the webui for periodically polling the monitoring endpoint. Instead of having each controller have to deal with monitoring information during the update cycle, this allows each controller to setup asynchronous monitoring updates, stopping it when the controller becomes inactive. Diffs - src/webui/master/static/js/controllers.js 01fe64ca6b784b210a5687f4e1cda60cead8671d src/webui/master/static/js/services.js 122519a5a4de93edf1fd7a5256e565cc78c59670 src/webui/master/static/slave.html 134aa0b10a232a37654a4ef9ac4bb149dbbebdea src/webui/master/static/slave_executor.html 81c10cbf4dd77f65dd8c3081281740c2be1e5b56 src/webui/master/static/slave_framework.html 04a041e9a4e8e1364617d09412f0a81a160ee48a Diff: https://reviews.apache.org/r/13606/diff/ Testing --- Ran the long-lived-framework. Thanks, Ben Mahler
Re: Review Request 13606: Added a Top abstraction to the webui.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13606/#review32149 --- I am going to have Thomas look at the JS as well. Since I wrote most of the JS, I'd like to get another set of eyes on it. - Ross Allen On Jan. 15, 2014, 2:30 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13606/ --- (Updated Jan. 15, 2014, 2:30 a.m.) Review request for mesos, Benjamin Hindman and Vinod Kone. Repository: mesos-git Description --- This adds a Top abstraction to the webui for periodically polling the monitoring endpoint. Instead of having each controller have to deal with monitoring information during the update cycle, this allows each controller to setup asynchronous monitoring updates, stopping it when the controller becomes inactive. Diffs - src/webui/master/static/js/controllers.js 01fe64ca6b784b210a5687f4e1cda60cead8671d src/webui/master/static/js/services.js 122519a5a4de93edf1fd7a5256e565cc78c59670 src/webui/master/static/slave.html 134aa0b10a232a37654a4ef9ac4bb149dbbebdea src/webui/master/static/slave_executor.html 81c10cbf4dd77f65dd8c3081281740c2be1e5b56 src/webui/master/static/slave_framework.html 04a041e9a4e8e1364617d09412f0a81a160ee48a Diff: https://reviews.apache.org/r/13606/diff/ Testing --- Ran the long-lived-framework. Thanks, Ben Mahler
Re: Review Request 13606: Added a Top abstraction to the webui.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13606/#review32159
---
src/webui/master/static/js/controllers.js
https://reviews.apache.org/r/13606/#comment60859
The `update` function looks almost identical between SlaveCtrl and
SlaveFrameworkCtrl. Can we make this its own function and get rid of the
cut/paste?
src/webui/master/static/js/services.js
https://reviews.apache.org/r/13606/#comment60861
This could be something like:
_.each([ 'cpus_user_time_secs' ], function(n) { this[n] = 0.0 });
src/webui/master/static/js/services.js
https://reviews.apache.org/r/13606/#comment60862
Because the parameters are the same here, why not just do this:
_.each(this.metrics, function(v, name) {
this.metrics[name] += stats[name];
}
src/webui/master/static/slave.html
https://reviews.apache.org/r/13606/#comment60867
Having to look up something from a root object seems like the wrong way to
do things. Maybe change the ng-repeat to fetch from both monitor and frameworks
(doing an _.extend on the data) at the same time?
- Thomas Rampelberg
On Jan. 15, 2014, 2:30 a.m., Ben Mahler wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13606/
---
(Updated Jan. 15, 2014, 2:30 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Repository: mesos-git
Description
---
This adds a Top abstraction to the webui for periodically polling the
monitoring endpoint.
Instead of having each controller have to deal with monitoring information
during the update cycle, this allows each controller to setup asynchronous
monitoring updates, stopping it when the controller becomes inactive.
Diffs
-
src/webui/master/static/js/controllers.js
01fe64ca6b784b210a5687f4e1cda60cead8671d
src/webui/master/static/js/services.js
122519a5a4de93edf1fd7a5256e565cc78c59670
src/webui/master/static/slave.html 134aa0b10a232a37654a4ef9ac4bb149dbbebdea
src/webui/master/static/slave_executor.html
81c10cbf4dd77f65dd8c3081281740c2be1e5b56
src/webui/master/static/slave_framework.html
04a041e9a4e8e1364617d09412f0a81a160ee48a
Diff: https://reviews.apache.org/r/13606/diff/
Testing
---
Ran the long-lived-framework.
Thanks,
Ben Mahler
Re: Review Request 13606: Added a Top abstraction to the webui.
On Jan. 17, 2014, 6:24 p.m., Thomas Rampelberg wrote:
src/webui/master/static/js/services.js, line 121
https://reviews.apache.org/r/13606/diff/2/?file=423536#file423536line121
Because the parameters are the same here, why not just do this:
_.each(this.metrics, function(v, name) {
this.metrics[name] += stats[name];
}
Does this work if new metrics are added from the backend and we didn't capture
them above in our initialization?
On Jan. 17, 2014, 6:24 p.m., Thomas Rampelberg wrote:
src/webui/master/static/js/services.js, line 110
https://reviews.apache.org/r/13606/diff/2/?file=423536#file423536line110
This could be something like:
_.each([ 'cpus_user_time_secs' ], function(n) { this[n] = 0.0 });
Are you suggesting we keep a list of these metric names somewhere and operate
on them? I'm inclined to directly set them here as it's a little easier for
others to read.
On Jan. 17, 2014, 6:24 p.m., Thomas Rampelberg wrote:
src/webui/master/static/js/controllers.js, line 389
https://reviews.apache.org/r/13606/diff/2/?file=423535#file423535line389
The `update` function looks almost identical between SlaveCtrl and
SlaveFrameworkCtrl. Can we make this its own function and get rid of the
cut/paste?
Quite possibly, but I'll punt on the cleanup for now. :)
- Ben
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13606/#review32159
---
On Jan. 15, 2014, 2:30 a.m., Ben Mahler wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13606/
---
(Updated Jan. 15, 2014, 2:30 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Repository: mesos-git
Description
---
This adds a Top abstraction to the webui for periodically polling the
monitoring endpoint.
Instead of having each controller have to deal with monitoring information
during the update cycle, this allows each controller to setup asynchronous
monitoring updates, stopping it when the controller becomes inactive.
Diffs
-
src/webui/master/static/js/controllers.js
01fe64ca6b784b210a5687f4e1cda60cead8671d
src/webui/master/static/js/services.js
122519a5a4de93edf1fd7a5256e565cc78c59670
src/webui/master/static/slave.html 134aa0b10a232a37654a4ef9ac4bb149dbbebdea
src/webui/master/static/slave_executor.html
81c10cbf4dd77f65dd8c3081281740c2be1e5b56
src/webui/master/static/slave_framework.html
04a041e9a4e8e1364617d09412f0a81a160ee48a
Diff: https://reviews.apache.org/r/13606/diff/
Testing
---
Ran the long-lived-framework.
Thanks,
Ben Mahler
Re: Review Request 13606: Added a Top abstraction to the webui.
On Aug. 16, 2013, 6:54 p.m., Ross Allen wrote: src/webui/master/static/js/controllers.js, line 636 https://reviews.apache.org/r/13606/diff/1/?file=341590#file341590line636 These `if (!$scope.monitor)` blocks can be handled in the Top service. Ross and I added a started() call here rather than relying on the $scope.monitor. On Aug. 16, 2013, 6:54 p.m., Ross Allen wrote: src/webui/master/static/slave.html, line 177 https://reviews.apache.org/r/13606/diff/1/?file=341591#file341591line177 This division should be moved to the statistics object as well. This is not a division, although it looks like one. :) - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13606/#review25244 --- On Aug. 15, 2013, 10:14 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13606/ --- (Updated Aug. 15, 2013, 10:14 p.m.) Review request for mesos, Benjamin Hindman and Ross Allen. Repository: mesos-git Description --- This adds a Top abstraction to the webui for periodically polling the monitoring endpoint. Instead of having each controller have to deal with monitoring information during the update cycle, this allows each controller to setup asynchronous monitoring updates, stopping it when the controller becomes inactive. Diffs - src/webui/master/static/js/controllers.js 305aeaa195183c188e8ed949ccd67b49cbd757a0 src/webui/master/static/slave.html e1dabc0b04dec404a0aabd52d0387a0af70ad40d src/webui/master/static/slave_executor.html 519847c0de077717c88c4d35804ad40d339d4246 src/webui/master/static/slave_framework.html 947bab12eec8c3be5c3893b873b4b6378c8dcbbb Diff: https://reviews.apache.org/r/13606/diff/ Testing --- Ran the long-lived-framework. Thanks, Ben Mahler
Re: Review Request 13606: Added a Top abstraction to the webui.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13606/ --- (Updated Jan. 15, 2014, 2:30 a.m.) Review request for mesos, Benjamin Hindman and Vinod Kone. Changes --- Paired with Ross and updated this to be an Angular service. Repository: mesos-git Description --- This adds a Top abstraction to the webui for periodically polling the monitoring endpoint. Instead of having each controller have to deal with monitoring information during the update cycle, this allows each controller to setup asynchronous monitoring updates, stopping it when the controller becomes inactive. Diffs (updated) - src/webui/master/static/js/controllers.js 01fe64ca6b784b210a5687f4e1cda60cead8671d src/webui/master/static/js/services.js 122519a5a4de93edf1fd7a5256e565cc78c59670 src/webui/master/static/slave.html 134aa0b10a232a37654a4ef9ac4bb149dbbebdea src/webui/master/static/slave_executor.html 81c10cbf4dd77f65dd8c3081281740c2be1e5b56 src/webui/master/static/slave_framework.html 04a041e9a4e8e1364617d09412f0a81a160ee48a Diff: https://reviews.apache.org/r/13606/diff/ Testing --- Ran the long-lived-framework. Thanks, Ben Mahler
Re: Review Request 13606: Added a Top abstraction to the webui.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13606/#review25241 --- src/webui/master/static/js/controllers.js https://reviews.apache.org/r/13606/#comment49537 This should be an Angular Service (http://docs.angularjs.org/guide/dev_guide.services.understanding_services) to ensure it's a singleton and to reuse some of the code that is repeated in each of the controllers below. src/webui/master/static/js/controllers.js https://reviews.apache.org/r/13606/#comment49536 Since we are the only consumers of this Top function, you don't need to protect from calls without the `new` keyword. - Ross Allen On Aug. 15, 2013, 10:14 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13606/ --- (Updated Aug. 15, 2013, 10:14 p.m.) Review request for mesos, Benjamin Hindman and Ross Allen. Repository: mesos-git Description --- This adds a Top abstraction to the webui for periodically polling the monitoring endpoint. Instead of having each controller have to deal with monitoring information during the update cycle, this allows each controller to setup asynchronous monitoring updates, stopping it when the controller becomes inactive. Diffs - src/webui/master/static/js/controllers.js 305aeaa195183c188e8ed949ccd67b49cbd757a0 src/webui/master/static/slave.html e1dabc0b04dec404a0aabd52d0387a0af70ad40d src/webui/master/static/slave_executor.html 519847c0de077717c88c4d35804ad40d339d4246 src/webui/master/static/slave_framework.html 947bab12eec8c3be5c3893b873b4b6378c8dcbbb Diff: https://reviews.apache.org/r/13606/diff/ Testing --- Ran the long-lived-framework. Thanks, Ben Mahler
Re: Review Request 13606: Added a Top abstraction to the webui.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13606/#review25244 --- src/webui/master/static/js/controllers.js https://reviews.apache.org/r/13606/#comment49545 Can this be moved to a Statistics object to prevent duplicating this structure below for each slave? On instantiation it can initialize each of these values on itself. src/webui/master/static/js/controllers.js https://reviews.apache.org/r/13606/#comment49540 These `if (!$scope.monitor)` blocks can be handled in the Top service. src/webui/master/static/slave.html https://reviews.apache.org/r/13606/#comment49541 Can something like `cpus_total_usage` be added to the statistics object so addition is kept out of the template? src/webui/master/static/slave.html https://reviews.apache.org/r/13606/#comment49543 This division should be moved to the statistics object as well. - Ross Allen On Aug. 15, 2013, 10:14 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13606/ --- (Updated Aug. 15, 2013, 10:14 p.m.) Review request for mesos, Benjamin Hindman and Ross Allen. Repository: mesos-git Description --- This adds a Top abstraction to the webui for periodically polling the monitoring endpoint. Instead of having each controller have to deal with monitoring information during the update cycle, this allows each controller to setup asynchronous monitoring updates, stopping it when the controller becomes inactive. Diffs - src/webui/master/static/js/controllers.js 305aeaa195183c188e8ed949ccd67b49cbd757a0 src/webui/master/static/slave.html e1dabc0b04dec404a0aabd52d0387a0af70ad40d src/webui/master/static/slave_executor.html 519847c0de077717c88c4d35804ad40d339d4246 src/webui/master/static/slave_framework.html 947bab12eec8c3be5c3893b873b4b6378c8dcbbb Diff: https://reviews.apache.org/r/13606/diff/ Testing --- Ran the long-lived-framework. Thanks, Ben Mahler
