Re: Review Request 19833: Migrated Job page to angular JS
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/#review40748 --- Ship it! My only concern is I think the controller code can be clearer, but that doesn't need to block this. I can always cut a branch with a suggested refactor once this ships. - David McLaughlin On April 18, 2014, 2 a.m., Suman Karumuri wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/ --- (Updated April 18, 2014, 2 a.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-281 https://issues.apache.org/jira/browse/AURORA-281 Repository: aurora Description --- *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. Added it here so only new code can be reviewed. *** Migrated job page to AngularJS. Removed old scheduler job page. Removed js related to the old scheduler page. Removed data tables. Added moment.js library for date time manipulation. Refactored code a bit for reusability. Stats link should still be added to the new jobs page. Since the role/env code is not committed, this diff contains that code as well. Code changes start from page 4 in diff between version 1 and 2. Diffs - build.gradle 109c193da3324bd5534b409bfabb6aeb0adda7b1 src/main/java/org/apache/aurora/scheduler/app/AppModule.java eeafc784e915137cacd5f64df1252ccbaf6c0f6c src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java ec56c649116c03ef148bac916bd6691a94685bc3 src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 19df7889f15b4cf44e386d8ce0626cc94fdcdfba src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java 2ccc6f367b9715a0abb3e0673069289ae4860087 src/main/java/org/apache/aurora/scheduler/http/ServletModule.java e3ff2571d95effcf72b2047cc5840d56143a180c src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java b35aad8db006133ba70692e43db1f083d4950914 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png 881de7976ff98955e2a5487dca66e618a0655f3d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png c608682b04a6d9b8002602450c8ef7e80ebba099 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png d300f1064b3beac1d7d5274e294494d3143e53a2 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png 6a6ded7de821619aedc71d1738c0b73463a4452e src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png fc46c5ebf0524b72a509fe2d7c1bc74995cb8a9d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc.png a88d7975fe9017e4e5f2289a94bd1ed66a5f59dc src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc_disabled.png 4e144cf0b1f786a9248a2998311e8109998d8a2d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_both.png 18670406bc01ab2721781822dd6478917745ff54 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc.png def071ed5afd264a036f6d9e75856366fd6ad153 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc_disabled.png 7824973cc60fc1841b16f2cb39323cefcdc3f942 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.bootstrap.js 420e507086f7a2af08f68787f67208546745260d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.htmlNumberType.js 885ad347f3076dcf75558ef44806dc25a8171884 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.localstorage.js 3c0b39aefbd5a82adee49f2ac7539b6974576b2f src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/jquery.dataTables.min.js 02694a4a56eb247baa2398d971927dfbd1ac3e60 src/main/resources/org/apache/aurora/scheduler/http/assets/dictionary.js 92045c41706ee9315f3522c3b5bbff7f6e7d2c64 src/main/resources/org/apache/aurora/scheduler/http/assets/util.js 5dd0d17849c7116d70925275b9c6c0715b441f14 src/main/resources/org/apache/aurora/scheduler/http/schedulerzjob.st 28b56671b2e825912a6427e609c2bbe1e7758e26
Re: Review Request 19833: Migrated Job page to angular JS
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/#review40749 --- Ship it! My only concern is I think the controller code can be clearer, but that doesn't need to block this. I can always cut a branch with a suggested refactor once this ships. - David McLaughlin On April 18, 2014, 2 a.m., Suman Karumuri wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/ --- (Updated April 18, 2014, 2 a.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-281 https://issues.apache.org/jira/browse/AURORA-281 Repository: aurora Description --- *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. Added it here so only new code can be reviewed. *** Migrated job page to AngularJS. Removed old scheduler job page. Removed js related to the old scheduler page. Removed data tables. Added moment.js library for date time manipulation. Refactored code a bit for reusability. Stats link should still be added to the new jobs page. Since the role/env code is not committed, this diff contains that code as well. Code changes start from page 4 in diff between version 1 and 2. Diffs - build.gradle 109c193da3324bd5534b409bfabb6aeb0adda7b1 src/main/java/org/apache/aurora/scheduler/app/AppModule.java eeafc784e915137cacd5f64df1252ccbaf6c0f6c src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java ec56c649116c03ef148bac916bd6691a94685bc3 src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 19df7889f15b4cf44e386d8ce0626cc94fdcdfba src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java 2ccc6f367b9715a0abb3e0673069289ae4860087 src/main/java/org/apache/aurora/scheduler/http/ServletModule.java e3ff2571d95effcf72b2047cc5840d56143a180c src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java b35aad8db006133ba70692e43db1f083d4950914 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png 881de7976ff98955e2a5487dca66e618a0655f3d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png c608682b04a6d9b8002602450c8ef7e80ebba099 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png d300f1064b3beac1d7d5274e294494d3143e53a2 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png 6a6ded7de821619aedc71d1738c0b73463a4452e src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png fc46c5ebf0524b72a509fe2d7c1bc74995cb8a9d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc.png a88d7975fe9017e4e5f2289a94bd1ed66a5f59dc src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc_disabled.png 4e144cf0b1f786a9248a2998311e8109998d8a2d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_both.png 18670406bc01ab2721781822dd6478917745ff54 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc.png def071ed5afd264a036f6d9e75856366fd6ad153 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc_disabled.png 7824973cc60fc1841b16f2cb39323cefcdc3f942 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.bootstrap.js 420e507086f7a2af08f68787f67208546745260d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.htmlNumberType.js 885ad347f3076dcf75558ef44806dc25a8171884 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.localstorage.js 3c0b39aefbd5a82adee49f2ac7539b6974576b2f src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/jquery.dataTables.min.js 02694a4a56eb247baa2398d971927dfbd1ac3e60 src/main/resources/org/apache/aurora/scheduler/http/assets/dictionary.js 92045c41706ee9315f3522c3b5bbff7f6e7d2c64 src/main/resources/org/apache/aurora/scheduler/http/assets/util.js 5dd0d17849c7116d70925275b9c6c0715b441f14 src/main/resources/org/apache/aurora/scheduler/http/schedulerzjob.st 28b56671b2e825912a6427e609c2bbe1e7758e26
Re: Review Request 19833: Migrated Job page to angular JS
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/ --- (Updated April 19, 2014, 2:04 a.m.) Review request for Aurora, David McLaughlin and Bill Farner. Changes --- Merged code with master. Implemented maxim's UI changes related to SANDBOX_DELETED state as part of the merge. Updated IsolatedSchedulerModule to garbage collect finished tasks so we can test the UI going forward. Bugs: AURORA-281 https://issues.apache.org/jira/browse/AURORA-281 Repository: aurora Description --- *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. Added it here so only new code can be reviewed. *** Migrated job page to AngularJS. Removed old scheduler job page. Removed js related to the old scheduler page. Removed data tables. Added moment.js library for date time manipulation. Refactored code a bit for reusability. Stats link should still be added to the new jobs page. Since the role/env code is not committed, this diff contains that code as well. Code changes start from page 4 in diff between version 1 and 2. Diffs (updated) - build.gradle ea488414d366ece89ba25e42b98f6ad7ddbf3378 src/main/java/org/apache/aurora/scheduler/app/AppModule.java eeafc784e915137cacd5f64df1252ccbaf6c0f6c src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java ec56c649116c03ef148bac916bd6691a94685bc3 src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 19df7889f15b4cf44e386d8ce0626cc94fdcdfba src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java f0469c157fee79c4fd2b842db7a4a37fd3d7e83e src/main/java/org/apache/aurora/scheduler/http/ServletModule.java e3ff2571d95effcf72b2047cc5840d56143a180c src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java b35aad8db006133ba70692e43db1f083d4950914 src/main/java/org/apache/aurora/scheduler/local/IsolatedSchedulerModule.java b931baa3f610095d4c46582da5d5c1e06c8b0d8e src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png 881de7976ff98955e2a5487dca66e618a0655f3d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png c608682b04a6d9b8002602450c8ef7e80ebba099 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png d300f1064b3beac1d7d5274e294494d3143e53a2 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png 6a6ded7de821619aedc71d1738c0b73463a4452e src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png fc46c5ebf0524b72a509fe2d7c1bc74995cb8a9d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc.png a88d7975fe9017e4e5f2289a94bd1ed66a5f59dc src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc_disabled.png 4e144cf0b1f786a9248a2998311e8109998d8a2d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_both.png 18670406bc01ab2721781822dd6478917745ff54 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc.png def071ed5afd264a036f6d9e75856366fd6ad153 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc_disabled.png 7824973cc60fc1841b16f2cb39323cefcdc3f942 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.bootstrap.js 420e507086f7a2af08f68787f67208546745260d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.htmlNumberType.js 885ad347f3076dcf75558ef44806dc25a8171884 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.localstorage.js 3c0b39aefbd5a82adee49f2ac7539b6974576b2f src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/jquery.dataTables.min.js 02694a4a56eb247baa2398d971927dfbd1ac3e60 src/main/resources/org/apache/aurora/scheduler/http/assets/dictionary.js 92045c41706ee9315f3522c3b5bbff7f6e7d2c64 src/main/resources/org/apache/aurora/scheduler/http/assets/util.js 5dd0d17849c7116d70925275b9c6c0715b441f14 src/main/resources/org/apache/aurora/scheduler/http/schedulerzjob.st 185f2196200e020ac8f450e3111c8b8550e0a077 src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css ade850ce624964693e9bd55946464983c4b9f8c2 src/main/resources/org/apache/aurora/scheduler/http/ui/index.html
Re: Review Request 19833: Migrated Job page to angular JS
On April 17, 2014, 12:21 a.m., David McLaughlin wrote:
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js,
lines 361-363
https://reviews.apache.org/r/19833/diff/7/?file=560830#file560830line361
Better to move these default values to getTasksForJob (is the empty
string default value even required in the Angular templating language?).
The defaults are not required, but not having defaults, makes it hard to
understand what's in the scope and what the default type should be. Further,
assigning default values makes the UI more predictable when there is no data.
So, keeping these.
On April 17, 2014, 12:21 a.m., David McLaughlin wrote:
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js,
line 403
https://reviews.apache.org/r/19833/diff/7/?file=560830#file560830line403
Why isn't this part of taskUtil?
Considered that. But left it here since this code is part of the data needed by
the controller. Summarizing task configs is sort of an independent feature that
can be used else where so moved it out of this class. Further, the rest of the
code depends on the object returned here so it makes the controller code easier
to read.
On April 17, 2014, 12:21 a.m., David McLaughlin wrote:
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js,
line 365
https://reviews.apache.org/r/19833/diff/7/?file=560830#file560830line365
Why are you passing $scope.role, $scope.environment and $scope.job into
a function which reads and writes to the $scope object anyway? Why not just
pass $scope? Or why have parameters at all if it's just a closure?
If the intent is to break a large controller into functions to make it
easier to read (which I think is a good idea), it is best to define this
function outside of the controller itself, so that the function body is
just the control flow.
I'm not sure of the idiomatic way to do this in Angular.JS, but
normally you just use JavaScript function scope to break your code into
smaller functions without polluting your API.
I mean intuitively I would just do something like:
(function() {
function getTasksForJob($scope) {
// do some side-effecty things to $scope
}
function jobController($scope, ..) {
// only control-flow code here
getTasksForJob($scope);
}
auroraUIControllers.controller('JobController', jobController);
})();
Passing the required fields explicitly makes the function signature simpler to
reason about. I don't see much to be gained by moving this functionality out of
the controller. Since we follow this pattern for other controllers, leaving it
as is here. I can get rid of the arguments if you want and just access scope
directly, but that would make it harder to know what args the function needs
during tests.
On April 17, 2014, 12:21 a.m., David McLaughlin wrote:
src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js, line
92
https://reviews.apache.org/r/19833/diff/7/?file=560833#file560833line92
I realise this code doesn't have bugs right now, but the 'this' keyword
can cause subtle problems given how easy it is to change the calling scope.
You can avoid having to worry about it here by pulling the return object
into a named variable and using that instead.
i.e.
function() {
var taskUtil = {
summarizeActiveConfigs: function(..) {
//... this will still work
taskUtil.isActiveTask(task);
// ..
},
isActiveTask: function(..) { .. }
}
return taskUtil;
}
Yeah this is better. Fixed it in the auroraClient as well.
On April 17, 2014, 12:21 a.m., David McLaughlin wrote:
src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js,
lines 132-149
https://reviews.apache.org/r/19833/diff/7/?file=560833#file560833line132
Style:
Beyond a one liner, it is best to extract these values into temporary
variables.
i.e.
var constraints = _.chain(task.constraints) // etc.
var metadata = _.chain(task.metadata)
// etc.
return {
// ..
constraints: constraints,
metadata: metadata,
// ..
}
We typically avoid spurious variables. In this case I think it's justified.
- Suman
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19833/#review40608
---
On April 16, 2014, 8:37 a.m., Suman Karumuri wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19833/
Re: Review Request 19833: Migrated Job page to angular JS
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/ --- (Updated April 18, 2014, 1:59 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Changes --- Addressed David's feedback. Bugs: AURORA-281 https://issues.apache.org/jira/browse/AURORA-281 Repository: aurora Description --- *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. Added it here so only new code can be reviewed. *** Migrated job page to AngularJS. Removed old scheduler job page. Removed js related to the old scheduler page. Removed data tables. Added moment.js library for date time manipulation. Refactored code a bit for reusability. Stats link should still be added to the new jobs page. Since the role/env code is not committed, this diff contains that code as well. Code changes start from page 4 in diff between version 1 and 2. Diffs (updated) - build.gradle 109c193da3324bd5534b409bfabb6aeb0adda7b1 src/main/java/org/apache/aurora/scheduler/app/AppModule.java eeafc784e915137cacd5f64df1252ccbaf6c0f6c src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java ec56c649116c03ef148bac916bd6691a94685bc3 src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 19df7889f15b4cf44e386d8ce0626cc94fdcdfba src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java 2ccc6f367b9715a0abb3e0673069289ae4860087 src/main/java/org/apache/aurora/scheduler/http/ServletModule.java e3ff2571d95effcf72b2047cc5840d56143a180c src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java b35aad8db006133ba70692e43db1f083d4950914 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png 881de7976ff98955e2a5487dca66e618a0655f3d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png c608682b04a6d9b8002602450c8ef7e80ebba099 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png d300f1064b3beac1d7d5274e294494d3143e53a2 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png 6a6ded7de821619aedc71d1738c0b73463a4452e src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png fc46c5ebf0524b72a509fe2d7c1bc74995cb8a9d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc.png a88d7975fe9017e4e5f2289a94bd1ed66a5f59dc src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc_disabled.png 4e144cf0b1f786a9248a2998311e8109998d8a2d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_both.png 18670406bc01ab2721781822dd6478917745ff54 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc.png def071ed5afd264a036f6d9e75856366fd6ad153 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc_disabled.png 7824973cc60fc1841b16f2cb39323cefcdc3f942 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.bootstrap.js 420e507086f7a2af08f68787f67208546745260d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.htmlNumberType.js 885ad347f3076dcf75558ef44806dc25a8171884 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.localstorage.js 3c0b39aefbd5a82adee49f2ac7539b6974576b2f src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/jquery.dataTables.min.js 02694a4a56eb247baa2398d971927dfbd1ac3e60 src/main/resources/org/apache/aurora/scheduler/http/assets/dictionary.js 92045c41706ee9315f3522c3b5bbff7f6e7d2c64 src/main/resources/org/apache/aurora/scheduler/http/assets/util.js 5dd0d17849c7116d70925275b9c6c0715b441f14 src/main/resources/org/apache/aurora/scheduler/http/schedulerzjob.st 28b56671b2e825912a6427e609c2bbe1e7758e26 src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css ade850ce624964693e9bd55946464983c4b9f8c2 src/main/resources/org/apache/aurora/scheduler/http/ui/index.html 36225d1e5147e30ba2cb4ddda96dec9f0f2f1dce src/main/resources/org/apache/aurora/scheduler/http/ui/job.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/js/app.js db6ea99aeb749fd8674613e3620dc3012872e13c src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
Re: Review Request 19833: Migrated Job page to angular JS
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/ --- (Updated April 18, 2014, 2 a.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-281 https://issues.apache.org/jira/browse/AURORA-281 Repository: aurora Description --- *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. Added it here so only new code can be reviewed. *** Migrated job page to AngularJS. Removed old scheduler job page. Removed js related to the old scheduler page. Removed data tables. Added moment.js library for date time manipulation. Refactored code a bit for reusability. Stats link should still be added to the new jobs page. Since the role/env code is not committed, this diff contains that code as well. Code changes start from page 4 in diff between version 1 and 2. Diffs - build.gradle 109c193da3324bd5534b409bfabb6aeb0adda7b1 src/main/java/org/apache/aurora/scheduler/app/AppModule.java eeafc784e915137cacd5f64df1252ccbaf6c0f6c src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java ec56c649116c03ef148bac916bd6691a94685bc3 src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 19df7889f15b4cf44e386d8ce0626cc94fdcdfba src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java 2ccc6f367b9715a0abb3e0673069289ae4860087 src/main/java/org/apache/aurora/scheduler/http/ServletModule.java e3ff2571d95effcf72b2047cc5840d56143a180c src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java b35aad8db006133ba70692e43db1f083d4950914 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png 881de7976ff98955e2a5487dca66e618a0655f3d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png c608682b04a6d9b8002602450c8ef7e80ebba099 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png d300f1064b3beac1d7d5274e294494d3143e53a2 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png 6a6ded7de821619aedc71d1738c0b73463a4452e src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png fc46c5ebf0524b72a509fe2d7c1bc74995cb8a9d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc.png a88d7975fe9017e4e5f2289a94bd1ed66a5f59dc src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc_disabled.png 4e144cf0b1f786a9248a2998311e8109998d8a2d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_both.png 18670406bc01ab2721781822dd6478917745ff54 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc.png def071ed5afd264a036f6d9e75856366fd6ad153 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc_disabled.png 7824973cc60fc1841b16f2cb39323cefcdc3f942 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.bootstrap.js 420e507086f7a2af08f68787f67208546745260d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.htmlNumberType.js 885ad347f3076dcf75558ef44806dc25a8171884 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.localstorage.js 3c0b39aefbd5a82adee49f2ac7539b6974576b2f src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/jquery.dataTables.min.js 02694a4a56eb247baa2398d971927dfbd1ac3e60 src/main/resources/org/apache/aurora/scheduler/http/assets/dictionary.js 92045c41706ee9315f3522c3b5bbff7f6e7d2c64 src/main/resources/org/apache/aurora/scheduler/http/assets/util.js 5dd0d17849c7116d70925275b9c6c0715b441f14 src/main/resources/org/apache/aurora/scheduler/http/schedulerzjob.st 28b56671b2e825912a6427e609c2bbe1e7758e26 src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css ade850ce624964693e9bd55946464983c4b9f8c2 src/main/resources/org/apache/aurora/scheduler/http/ui/index.html 36225d1e5147e30ba2cb4ddda96dec9f0f2f1dce src/main/resources/org/apache/aurora/scheduler/http/ui/job.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/js/app.js db6ea99aeb749fd8674613e3620dc3012872e13c src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7cd534479dd2f17ffd46248ce9af1f8fe89beb97
Re: Review Request 19833: Migrated Job page to angular JS
On April 17, 2014, 12:21 a.m., David McLaughlin wrote:
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js,
line 365
https://reviews.apache.org/r/19833/diff/7/?file=560830#file560830line365
Why are you passing $scope.role, $scope.environment and $scope.job into
a function which reads and writes to the $scope object anyway? Why not just
pass $scope? Or why have parameters at all if it's just a closure?
If the intent is to break a large controller into functions to make it
easier to read (which I think is a good idea), it is best to define this
function outside of the controller itself, so that the function body is
just the control flow.
I'm not sure of the idiomatic way to do this in Angular.JS, but
normally you just use JavaScript function scope to break your code into
smaller functions without polluting your API.
I mean intuitively I would just do something like:
(function() {
function getTasksForJob($scope) {
// do some side-effecty things to $scope
}
function jobController($scope, ..) {
// only control-flow code here
getTasksForJob($scope);
}
auroraUIControllers.controller('JobController', jobController);
})();
Suman Karumuri wrote:
Passing the required fields explicitly makes the function signature
simpler to reason about. I don't see much to be gained by moving this
functionality out of the controller. Since we follow this pattern for other
controllers, leaving it as is here. I can get rid of the arguments if you
want and just access scope directly, but that would make it harder to know
what args the function needs during tests.
Note that the testing reasoning is sort of irrelevant - these inline functions
cannot be tested as they are scoped inside a parent function (i.e. private) :)
- David
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19833/#review40608
---
On April 18, 2014, 2 a.m., Suman Karumuri wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19833/
---
(Updated April 18, 2014, 2 a.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Bugs: AURORA-281
https://issues.apache.org/jira/browse/AURORA-281
Repository: aurora
Description
---
*** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/.
Added it here so only new code can be reviewed. ***
Migrated job page to AngularJS.
Removed old scheduler job page.
Removed js related to the old scheduler page. Removed data tables.
Added moment.js library for date time manipulation.
Refactored code a bit for reusability.
Stats link should still be added to the new jobs page.
Since the role/env code is not committed, this diff contains that code as
well.
Code changes start from page 4 in diff between version 1 and 2.
Diffs
-
build.gradle 109c193da3324bd5534b409bfabb6aeb0adda7b1
src/main/java/org/apache/aurora/scheduler/app/AppModule.java
eeafc784e915137cacd5f64df1252ccbaf6c0f6c
src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java
ec56c649116c03ef148bac916bd6691a94685bc3
src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java
19df7889f15b4cf44e386d8ce0626cc94fdcdfba
src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java
2ccc6f367b9715a0abb3e0673069289ae4860087
src/main/java/org/apache/aurora/scheduler/http/ServletModule.java
e3ff2571d95effcf72b2047cc5840d56143a180c
src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java
b35aad8db006133ba70692e43db1f083d4950914
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css
ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png
881de7976ff98955e2a5487dca66e618a0655f3d
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png
c608682b04a6d9b8002602450c8ef7e80ebba099
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png
d300f1064b3beac1d7d5274e294494d3143e53a2
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png
6a6ded7de821619aedc71d1738c0b73463a4452e
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png
a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png
Re: Review Request 19833: Migrated Job page to angular JS
On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
A few general Javascript comments: there seems to have been an explosion of
global state and some controllers are getting pretty big. Consider using
angular's dependency injection system to keep dependencies explicit and
managed rather than relying on global bindings.
There seems to be a general lack of documentation for utility functions and
controllers. Consider factoring common utility functions into their own
documented services and injecting them.
The controllers code has grown several hundred lines but there's a lack of
tests - it may be time to bite the bullet and write some. In theory Angular
makes this easy, but I haven't done it before.
JobController is a weird controller in that respect since all the data on the
page is seeded by a single call to the scheduler backend.
Refactored some of the task config summary logic into a service as a
refactoring step. Ideally we shouldn't be doing it since the service is used
only by one controller.
Added documentation.
Tests are out of scope for this change. We will add them going forward.
On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
src/main/java/org/apache/aurora/scheduler/app/AppModule.java, line 75
https://reviews.apache.org/r/19833/diff/6/?file=549444#file549444line75
Url
Fixed.
On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
build.gradle, line 343
https://reviews.apache.org/r/19833/diff/6/?file=549443#file549443line343
Any reason this can't be dropped?
The application will default it to empty string. But, having a fake URL will
help us when testing the local UI. This is an optional argument already.
On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java, lines
134-136
https://reviews.apache.org/r/19833/diff/6/?file=549445#file549445line134
Since this is being passed to AppModule move it there. Convention is to
have modules that care about a flag define them there.
All the arguments that are passed into AppModule are defined in this way.
Leaving it as is for consistency for now.
On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js,
line 270
https://reviews.apache.org/r/19833/diff/6/?file=549474#file549474line270
Is there a reason why you don't use call-chaining here? Most of the
examples I've seen use it and I find it more readable, especially in a
dynamic language where a typo in auroraUIControllers is a runtime error.
The angularJS style guide actually recommends against chaining
https://docs.google.com/a/twitter.com/document/d/1XXMvReO8-Awi1EZXAXS4PzDzdNvV6pGcuaF4Q9821Es/pub.
The reference phone cat app is here:
https://github.com/angular/angular-phonecat/blob/master/app/js/controllers.js
Chaining for large controllers makes code unreadable and makes the code very
inflexible. A typo is not an issue since the router catches an error upon
application startup on a missing controller.
On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js,
lines 296-304
https://reviews.apache.org/r/19833/diff/6/?file=549474#file549474line296
Push this down to the template with ng-hide=showSummary?
Moving the logic into a template makes this logic untestable. So, leaving it in
the controller.
On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js,
line 328
https://reviews.apache.org/r/19833/diff/6/?file=549474#file549474line328
Inject this?
This function is called by smart table while it is rendering it's data sort of
as a filter. It doesn't have access to any AngularJs variables at that point
so, can't inject this.
On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js,
line 359
https://reviews.apache.org/r/19833/diff/6/?file=549474#file549474line359
prefer lowcasing abbreviations - URL = Url
Done.
On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js,
line 365
https://reviews.apache.org/r/19833/diff/6/?file=549474#file549474line365
This both mutates the current scope and pretends to be a function - why
not make it void or return {active: [], completed: []}
Can't make it return an object since we need AngularJS bindings to work. Can't
make it void since we can't bind to the scope variable being initialized. The
only way here is to use a superfluous scope variable which makes the code less
DRY. There is no clean solution here. A book I read on the topic recommends it
be done this way.
On April 15, 2014, 8:15 p.m., Kevin Sweeney wrote:
Re: Review Request 19833: Migrated Job page to angular JS
On April 6, 2014, 12:24 a.m., Bill Farner wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/schedulingDetail.html, line 9 https://reviews.apache.org/r/19833/diff/6/?file=549479#file549479line9 remove space before the colon, here and below Done. On April 6, 2014, 12:24 a.m., Bill Farner wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, line 511 https://reviews.apache.org/r/19833/diff/6/?file=549474#file549474line511 s/result/ranges/ Done. On April 6, 2014, 12:24 a.m., Bill Farner wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/job.html, line 62 https://reviews.apache.org/r/19833/diff/6/?file=549472#file549472line62 Can you make this an anchor instead of span? Also, the issue i pointed out with the task links not working remains. Clicking the pi link, then clicking a task ID results in: Error fetching tasks: No tasks found for query: TaskQuery(owner:null, environment:task, jobName:1396742503655-mesos-test-adhocJob0-0-5a6d0345-56a5-41a9-89ec-70224fa826fc, taskIds:null, statuses:null, instanceIds:null, slaveHosts:null, jobKeys:null) Making it an anchor makes the code more complicated since we the browser would capture the default click on an anchor. Leaving it as is for now. I can make it look like a link if you want. Fixed the /structdump link. - Suman --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/#review39642 --- On April 4, 2014, 11:52 p.m., Suman Karumuri wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/ --- (Updated April 4, 2014, 11:52 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-281 https://issues.apache.org/jira/browse/AURORA-281 Repository: aurora Description --- *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. Added it here so only new code can be reviewed. *** Migrated job page to AngularJS. Removed old scheduler job page. Removed js related to the old scheduler page. Removed data tables. Added moment.js library for date time manipulation. Refactored code a bit for reusability. Stats link should still be added to the new jobs page. Since the role/env code is not committed, this diff contains that code as well. Code changes start from page 4 in diff between version 1 and 2. Diffs - build.gradle 109c193da3324bd5534b409bfabb6aeb0adda7b1 src/main/java/org/apache/aurora/scheduler/app/AppModule.java eeafc784e915137cacd5f64df1252ccbaf6c0f6c src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java ec56c649116c03ef148bac916bd6691a94685bc3 src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 19df7889f15b4cf44e386d8ce0626cc94fdcdfba src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java 2ccc6f367b9715a0abb3e0673069289ae4860087 src/main/java/org/apache/aurora/scheduler/http/ServletModule.java e3ff2571d95effcf72b2047cc5840d56143a180c src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java b35aad8db006133ba70692e43db1f083d4950914 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png 881de7976ff98955e2a5487dca66e618a0655f3d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png c608682b04a6d9b8002602450c8ef7e80ebba099 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png d300f1064b3beac1d7d5274e294494d3143e53a2 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png 6a6ded7de821619aedc71d1738c0b73463a4452e src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png fc46c5ebf0524b72a509fe2d7c1bc74995cb8a9d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc.png a88d7975fe9017e4e5f2289a94bd1ed66a5f59dc src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc_disabled.png 4e144cf0b1f786a9248a2998311e8109998d8a2d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_both.png 18670406bc01ab2721781822dd6478917745ff54
Re: Review Request 19833: Migrated Job page to angular JS
On April 6, 2014, 12:24 a.m., Bill Farner wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/job.html, line 62 https://reviews.apache.org/r/19833/diff/6/?file=549472#file549472line62 Can you make this an anchor instead of span? Also, the issue i pointed out with the task links not working remains. Clicking the pi link, then clicking a task ID results in: Error fetching tasks: No tasks found for query: TaskQuery(owner:null, environment:task, jobName:1396742503655-mesos-test-adhocJob0-0-5a6d0345-56a5-41a9-89ec-70224fa826fc, taskIds:null, statuses:null, instanceIds:null, slaveHosts:null, jobKeys:null) Suman Karumuri wrote: Making it an anchor makes the code more complicated since we the browser would capture the default click on an anchor. Leaving it as is for now. I can make it look like a link if you want. Fixed the /structdump link. For your point about complexity with links, see here: http://docs.angularjs.org/api/ng/directive/ngHref - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/#review39642 --- On April 4, 2014, 11:52 p.m., Suman Karumuri wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/ --- (Updated April 4, 2014, 11:52 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-281 https://issues.apache.org/jira/browse/AURORA-281 Repository: aurora Description --- *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. Added it here so only new code can be reviewed. *** Migrated job page to AngularJS. Removed old scheduler job page. Removed js related to the old scheduler page. Removed data tables. Added moment.js library for date time manipulation. Refactored code a bit for reusability. Stats link should still be added to the new jobs page. Since the role/env code is not committed, this diff contains that code as well. Code changes start from page 4 in diff between version 1 and 2. Diffs - build.gradle 109c193da3324bd5534b409bfabb6aeb0adda7b1 src/main/java/org/apache/aurora/scheduler/app/AppModule.java eeafc784e915137cacd5f64df1252ccbaf6c0f6c src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java ec56c649116c03ef148bac916bd6691a94685bc3 src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 19df7889f15b4cf44e386d8ce0626cc94fdcdfba src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java 2ccc6f367b9715a0abb3e0673069289ae4860087 src/main/java/org/apache/aurora/scheduler/http/ServletModule.java e3ff2571d95effcf72b2047cc5840d56143a180c src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java b35aad8db006133ba70692e43db1f083d4950914 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png 881de7976ff98955e2a5487dca66e618a0655f3d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png c608682b04a6d9b8002602450c8ef7e80ebba099 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png d300f1064b3beac1d7d5274e294494d3143e53a2 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png 6a6ded7de821619aedc71d1738c0b73463a4452e src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png fc46c5ebf0524b72a509fe2d7c1bc74995cb8a9d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc.png a88d7975fe9017e4e5f2289a94bd1ed66a5f59dc src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc_disabled.png 4e144cf0b1f786a9248a2998311e8109998d8a2d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_both.png 18670406bc01ab2721781822dd6478917745ff54 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc.png def071ed5afd264a036f6d9e75856366fd6ad153 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc_disabled.png 7824973cc60fc1841b16f2cb39323cefcdc3f942 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.bootstrap.js 420e507086f7a2af08f68787f67208546745260d
Re: Review Request 19833: Migrated Job page to angular JS
On April 16, 2014, 12:18 a.m., David McLaughlin wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/job.html, line 30 https://reviews.apache.org/r/19833/diff/6/?file=549472#file549472line30 You don't need noMarginLeft here. The left padding is only applied to .span* that don't have a parent which is a .row. Put all divs that are part of the grid inside a div class=row And you can get rid of this CSS rule. Thanks! Fixed. On April 16, 2014, 12:18 a.m., David McLaughlin wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/job.html, line 43 https://reviews.apache.org/r/19833/diff/6/?file=549472#file549472line43 This should have consistent markup between both task containers. Consider adding an explicit .row and .span12 container divs for the smart-tables in both sections. I have intentionally dropped classes so we can allow the table the expand based on the screen size. On April 16, 2014, 12:18 a.m., David McLaughlin wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/job.html, line 62 https://reviews.apache.org/r/19833/diff/6/?file=549472#file549472line62 You can probably just use the .pull-right class from Bootstrap here. pull-right doesn't replicate the current behavior. Leaving it as is for backwards compatibility. On April 16, 2014, 12:18 a.m., David McLaughlin wrote: src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, line 440 https://reviews.apache.org/r/19833/diff/6/?file=549474#file549474line440 I'm not sure what browser compatibility we're aiming for, but if it's IE 9+ you could use native map here instead. Generally, we expect users to be using modern browsers. But in this case preferring to use map from userscore.js for code consistency and broader browser compatibility. - Suman --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/#review40481 --- On April 4, 2014, 11:52 p.m., Suman Karumuri wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/ --- (Updated April 4, 2014, 11:52 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-281 https://issues.apache.org/jira/browse/AURORA-281 Repository: aurora Description --- *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. Added it here so only new code can be reviewed. *** Migrated job page to AngularJS. Removed old scheduler job page. Removed js related to the old scheduler page. Removed data tables. Added moment.js library for date time manipulation. Refactored code a bit for reusability. Stats link should still be added to the new jobs page. Since the role/env code is not committed, this diff contains that code as well. Code changes start from page 4 in diff between version 1 and 2. Diffs - build.gradle 109c193da3324bd5534b409bfabb6aeb0adda7b1 src/main/java/org/apache/aurora/scheduler/app/AppModule.java eeafc784e915137cacd5f64df1252ccbaf6c0f6c src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java ec56c649116c03ef148bac916bd6691a94685bc3 src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 19df7889f15b4cf44e386d8ce0626cc94fdcdfba src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java 2ccc6f367b9715a0abb3e0673069289ae4860087 src/main/java/org/apache/aurora/scheduler/http/ServletModule.java e3ff2571d95effcf72b2047cc5840d56143a180c src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java b35aad8db006133ba70692e43db1f083d4950914 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png 881de7976ff98955e2a5487dca66e618a0655f3d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png c608682b04a6d9b8002602450c8ef7e80ebba099 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png d300f1064b3beac1d7d5274e294494d3143e53a2 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png 6a6ded7de821619aedc71d1738c0b73463a4452e src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png fc46c5ebf0524b72a509fe2d7c1bc74995cb8a9d
Re: Review Request 19833: Migrated Job page to angular JS
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/ --- (Updated April 16, 2014, 8:37 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-281 https://issues.apache.org/jira/browse/AURORA-281 Repository: aurora Description --- *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. Added it here so only new code can be reviewed. *** Migrated job page to AngularJS. Removed old scheduler job page. Removed js related to the old scheduler page. Removed data tables. Added moment.js library for date time manipulation. Refactored code a bit for reusability. Stats link should still be added to the new jobs page. Since the role/env code is not committed, this diff contains that code as well. Code changes start from page 4 in diff between version 1 and 2. Diffs - build.gradle 109c193da3324bd5534b409bfabb6aeb0adda7b1 src/main/java/org/apache/aurora/scheduler/app/AppModule.java eeafc784e915137cacd5f64df1252ccbaf6c0f6c src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java ec56c649116c03ef148bac916bd6691a94685bc3 src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 19df7889f15b4cf44e386d8ce0626cc94fdcdfba src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java 2ccc6f367b9715a0abb3e0673069289ae4860087 src/main/java/org/apache/aurora/scheduler/http/ServletModule.java e3ff2571d95effcf72b2047cc5840d56143a180c src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java b35aad8db006133ba70692e43db1f083d4950914 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png 881de7976ff98955e2a5487dca66e618a0655f3d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png c608682b04a6d9b8002602450c8ef7e80ebba099 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png d300f1064b3beac1d7d5274e294494d3143e53a2 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png 6a6ded7de821619aedc71d1738c0b73463a4452e src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png fc46c5ebf0524b72a509fe2d7c1bc74995cb8a9d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc.png a88d7975fe9017e4e5f2289a94bd1ed66a5f59dc src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc_disabled.png 4e144cf0b1f786a9248a2998311e8109998d8a2d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_both.png 18670406bc01ab2721781822dd6478917745ff54 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc.png def071ed5afd264a036f6d9e75856366fd6ad153 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc_disabled.png 7824973cc60fc1841b16f2cb39323cefcdc3f942 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.bootstrap.js 420e507086f7a2af08f68787f67208546745260d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.htmlNumberType.js 885ad347f3076dcf75558ef44806dc25a8171884 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.localstorage.js 3c0b39aefbd5a82adee49f2ac7539b6974576b2f src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/jquery.dataTables.min.js 02694a4a56eb247baa2398d971927dfbd1ac3e60 src/main/resources/org/apache/aurora/scheduler/http/assets/dictionary.js 92045c41706ee9315f3522c3b5bbff7f6e7d2c64 src/main/resources/org/apache/aurora/scheduler/http/assets/util.js 5dd0d17849c7116d70925275b9c6c0715b441f14 src/main/resources/org/apache/aurora/scheduler/http/schedulerzjob.st 28b56671b2e825912a6427e609c2bbe1e7758e26 src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css ade850ce624964693e9bd55946464983c4b9f8c2 src/main/resources/org/apache/aurora/scheduler/http/ui/index.html 36225d1e5147e30ba2cb4ddda96dec9f0f2f1dce src/main/resources/org/apache/aurora/scheduler/http/ui/job.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/js/app.js db6ea99aeb749fd8674613e3620dc3012872e13c src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7cd534479dd2f17ffd46248ce9af1f8fe89beb97
Re: Review Request 19833: Migrated Job page to angular JS
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/ --- (Updated April 16, 2014, 8:37 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Changes --- Incorporated code review feedback. Fixed the bug when displaying /structdump page. The back button issue on /structdump page is back. Will talk to David tomorrow to see if we can fix this. Since the issue only presents itself when the pi link is clicked, and only affects us, sending out the code review. Bugs: AURORA-281 https://issues.apache.org/jira/browse/AURORA-281 Repository: aurora Description --- *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. Added it here so only new code can be reviewed. *** Migrated job page to AngularJS. Removed old scheduler job page. Removed js related to the old scheduler page. Removed data tables. Added moment.js library for date time manipulation. Refactored code a bit for reusability. Stats link should still be added to the new jobs page. Since the role/env code is not committed, this diff contains that code as well. Code changes start from page 4 in diff between version 1 and 2. Diffs (updated) - build.gradle 109c193da3324bd5534b409bfabb6aeb0adda7b1 src/main/java/org/apache/aurora/scheduler/app/AppModule.java eeafc784e915137cacd5f64df1252ccbaf6c0f6c src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java ec56c649116c03ef148bac916bd6691a94685bc3 src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 19df7889f15b4cf44e386d8ce0626cc94fdcdfba src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java 2ccc6f367b9715a0abb3e0673069289ae4860087 src/main/java/org/apache/aurora/scheduler/http/ServletModule.java e3ff2571d95effcf72b2047cc5840d56143a180c src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java b35aad8db006133ba70692e43db1f083d4950914 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png 881de7976ff98955e2a5487dca66e618a0655f3d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png c608682b04a6d9b8002602450c8ef7e80ebba099 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png d300f1064b3beac1d7d5274e294494d3143e53a2 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png 6a6ded7de821619aedc71d1738c0b73463a4452e src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png fc46c5ebf0524b72a509fe2d7c1bc74995cb8a9d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc.png a88d7975fe9017e4e5f2289a94bd1ed66a5f59dc src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc_disabled.png 4e144cf0b1f786a9248a2998311e8109998d8a2d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_both.png 18670406bc01ab2721781822dd6478917745ff54 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc.png def071ed5afd264a036f6d9e75856366fd6ad153 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc_disabled.png 7824973cc60fc1841b16f2cb39323cefcdc3f942 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.bootstrap.js 420e507086f7a2af08f68787f67208546745260d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.htmlNumberType.js 885ad347f3076dcf75558ef44806dc25a8171884 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.localstorage.js 3c0b39aefbd5a82adee49f2ac7539b6974576b2f src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/jquery.dataTables.min.js 02694a4a56eb247baa2398d971927dfbd1ac3e60 src/main/resources/org/apache/aurora/scheduler/http/assets/dictionary.js 92045c41706ee9315f3522c3b5bbff7f6e7d2c64 src/main/resources/org/apache/aurora/scheduler/http/assets/util.js 5dd0d17849c7116d70925275b9c6c0715b441f14 src/main/resources/org/apache/aurora/scheduler/http/schedulerzjob.st 28b56671b2e825912a6427e609c2bbe1e7758e26 src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css ade850ce624964693e9bd55946464983c4b9f8c2 src/main/resources/org/apache/aurora/scheduler/http/ui/index.html 36225d1e5147e30ba2cb4ddda96dec9f0f2f1dce
Re: Review Request 19833: Migrated Job page to angular JS
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19833/#review40608
---
src/main/resources/org/apache/aurora/scheduler/http/ui/job.html
https://reviews.apache.org/r/19833/#comment73678
Repeating the comment from Bill. Make this an anchor with no href and
Angular will call preventDefault for you.
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
https://reviews.apache.org/r/19833/#comment73669
Better to move these default values to getTasksForJob (is the empty string
default value even required in the Angular templating language?).
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
https://reviews.apache.org/r/19833/#comment73637
Why are you passing $scope.role, $scope.environment and $scope.job into a
function which reads and writes to the $scope object anyway? Why not just pass
$scope? Or why have parameters at all if it's just a closure?
If the intent is to break a large controller into functions to make it
easier to read (which I think is a good idea), it is best to define this
function outside of the controller itself, so that the function body is just
the control flow.
I'm not sure of the idiomatic way to do this in Angular.JS, but normally
you just use JavaScript function scope to break your code into smaller
functions without polluting your API.
I mean intuitively I would just do something like:
(function() {
function getTasksForJob($scope) {
// do some side-effecty things to $scope
}
function jobController($scope, ..) {
// only control-flow code here
getTasksForJob($scope);
}
auroraUIControllers.controller('JobController', jobController);
})();
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
https://reviews.apache.org/r/19833/#comment73670
I feel like this might be clearer as:
if(response.error) {
$scope.error = Error fetching tasks: + response.error;
return [];
}
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
https://reviews.apache.org/r/19833/#comment73641
Why isn't this part of taskUtil?
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
https://reviews.apache.org/r/19833/#comment73668
Style: prefer ScheduleStatus.RUNNING here.
src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js
https://reviews.apache.org/r/19833/#comment73645
I realise this code doesn't have bugs right now, but the 'this' keyword can
cause subtle problems given how easy it is to change the calling scope. You can
avoid having to worry about it here by pulling the return object into a named
variable and using that instead.
i.e.
function() {
var taskUtil = {
summarizeActiveConfigs: function(..) {
//... this will still work
taskUtil.isActiveTask(task);
// ..
},
isActiveTask: function(..) { .. }
}
return taskUtil;
}
src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js
https://reviews.apache.org/r/19833/#comment73671
Can be rewritten as:
.filter(taskUtil.isActiveTask)
src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js
https://reviews.apache.org/r/19833/#comment73673
The terseness of this code makes it quite difficult to read. Would be nice
to split this into multiple lines with descriptive variable names for the
intermediate steps. Alternatively, keep it the same and add some inline
comments to explain how the data is being transformed.
src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js
https://reviews.apache.org/r/19833/#comment73676
Style:
Beyond a one liner, it is best to extract these values into temporary
variables.
i.e.
var constraints = _.chain(task.constraints) // etc.
var metadata = _.chain(task.metadata)
// etc.
return {
// ..
constraints: constraints,
metadata: metadata,
// ..
}
src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js
https://reviews.apache.org/r/19833/#comment73675
Equivalent to
.map(taskUtil.formatConstraint)
src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js
https://reviews.apache.org/r/19833/#comment73677
This code path will execute if the value is undefined.
Why not just use:
if(taskConstraint.value)
here?
I'm also assuming if taskConstraint.value exists,
taskConstraint.value.values will always exist and will always be an array?
Otherwise you should add checks for that too.
- David McLaughlin
On April 16, 2014, 8:37 a.m., Suman Karumuri wrote:
Re: Review Request 19833: Migrated Job page to angular JS
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19833/#review40443
---
A few general Javascript comments: there seems to have been an explosion of
global state and some controllers are getting pretty big. Consider using
angular's dependency injection system to keep dependencies explicit and managed
rather than relying on global bindings.
There seems to be a general lack of documentation for utility functions and
controllers. Consider factoring common utility functions into their own
documented services and injecting them.
The controllers code has grown several hundred lines but there's a lack of
tests - it may be time to bite the bullet and write some. In theory Angular
makes this easy, but I haven't done it before.
build.gradle
https://reviews.apache.org/r/19833/#comment73450
Any reason this can't be dropped?
src/main/java/org/apache/aurora/scheduler/app/AppModule.java
https://reviews.apache.org/r/19833/#comment73448
Url
src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java
https://reviews.apache.org/r/19833/#comment73451
Since this is being passed to AppModule move it there. Convention is to
have modules that care about a flag define them there.
src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java
https://reviews.apache.org/r/19833/#comment73452
Push this down to AppModule
src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java
https://reviews.apache.org/r/19833/#comment73453
Push this down to AppModule
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
https://reviews.apache.org/r/19833/#comment73457
Is there a reason why you don't use call-chaining here? Most of the
examples I've seen use it and I find it more readable, especially in a dynamic
language where a typo in auroraUIControllers is a runtime error.
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
https://reviews.apache.org/r/19833/#comment73464
Push this down to the template with ng-hide=showSummary?
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
https://reviews.apache.org/r/19833/#comment73465
Inject this?
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
https://reviews.apache.org/r/19833/#comment73469
prefer lowcasing abbreviations - URL = Url
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
https://reviews.apache.org/r/19833/#comment73472
This both mutates the current scope and pretends to be a function - why not
make it void or return {active: [], completed: []}
src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js
https://reviews.apache.org/r/19833/#comment73481
These should be documented.
src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js
https://reviews.apache.org/r/19833/#comment73480
statsUrlPrefix
Also consider making this nullable and hiding the icon behind an ng-hide if
it's null.
src/main/thrift/org/apache/aurora/gen/api.thrift
https://reviews.apache.org/r/19833/#comment73479
statsUrlPrefix. Also, is there any reason this can't be optional - seems
like null should allow the scheduler to continue operating fine without needing
a fake value.
src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java
https://reviews.apache.org/r/19833/#comment73483
Again push this into AppModule.
- Kevin Sweeney
On April 4, 2014, 4:52 p.m., Suman Karumuri wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19833/
---
(Updated April 4, 2014, 4:52 p.m.)
Review request for Aurora, Kevin Sweeney and Bill Farner.
Bugs: AURORA-281
https://issues.apache.org/jira/browse/AURORA-281
Repository: aurora
Description
---
*** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/.
Added it here so only new code can be reviewed. ***
Migrated job page to AngularJS.
Removed old scheduler job page.
Removed js related to the old scheduler page. Removed data tables.
Added moment.js library for date time manipulation.
Refactored code a bit for reusability.
Stats link should still be added to the new jobs page.
Since the role/env code is not committed, this diff contains that code as
well.
Code changes start from page 4 in diff between version 1 and 2.
Diffs
-
build.gradle 109c193da3324bd5534b409bfabb6aeb0adda7b1
src/main/java/org/apache/aurora/scheduler/app/AppModule.java
eeafc784e915137cacd5f64df1252ccbaf6c0f6c
src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java
ec56c649116c03ef148bac916bd6691a94685bc3
Re: Review Request 19833: Migrated Job page to angular JS
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/#review39908 --- pinging Kevin. - Suman Karumuri On April 4, 2014, 11:52 p.m., Suman Karumuri wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/ --- (Updated April 4, 2014, 11:52 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-281 https://issues.apache.org/jira/browse/AURORA-281 Repository: aurora Description --- *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. Added it here so only new code can be reviewed. *** Migrated job page to AngularJS. Removed old scheduler job page. Removed js related to the old scheduler page. Removed data tables. Added moment.js library for date time manipulation. Refactored code a bit for reusability. Stats link should still be added to the new jobs page. Since the role/env code is not committed, this diff contains that code as well. Code changes start from page 4 in diff between version 1 and 2. Diffs - build.gradle 109c193da3324bd5534b409bfabb6aeb0adda7b1 src/main/java/org/apache/aurora/scheduler/app/AppModule.java eeafc784e915137cacd5f64df1252ccbaf6c0f6c src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java ec56c649116c03ef148bac916bd6691a94685bc3 src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 19df7889f15b4cf44e386d8ce0626cc94fdcdfba src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java 2ccc6f367b9715a0abb3e0673069289ae4860087 src/main/java/org/apache/aurora/scheduler/http/ServletModule.java e3ff2571d95effcf72b2047cc5840d56143a180c src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java b35aad8db006133ba70692e43db1f083d4950914 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png 881de7976ff98955e2a5487dca66e618a0655f3d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png c608682b04a6d9b8002602450c8ef7e80ebba099 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png d300f1064b3beac1d7d5274e294494d3143e53a2 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png 6a6ded7de821619aedc71d1738c0b73463a4452e src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png fc46c5ebf0524b72a509fe2d7c1bc74995cb8a9d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc.png a88d7975fe9017e4e5f2289a94bd1ed66a5f59dc src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc_disabled.png 4e144cf0b1f786a9248a2998311e8109998d8a2d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_both.png 18670406bc01ab2721781822dd6478917745ff54 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc.png def071ed5afd264a036f6d9e75856366fd6ad153 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc_disabled.png 7824973cc60fc1841b16f2cb39323cefcdc3f942 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.bootstrap.js 420e507086f7a2af08f68787f67208546745260d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.htmlNumberType.js 885ad347f3076dcf75558ef44806dc25a8171884 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.localstorage.js 3c0b39aefbd5a82adee49f2ac7539b6974576b2f src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/jquery.dataTables.min.js 02694a4a56eb247baa2398d971927dfbd1ac3e60 src/main/resources/org/apache/aurora/scheduler/http/assets/dictionary.js 92045c41706ee9315f3522c3b5bbff7f6e7d2c64 src/main/resources/org/apache/aurora/scheduler/http/assets/util.js 5dd0d17849c7116d70925275b9c6c0715b441f14 src/main/resources/org/apache/aurora/scheduler/http/schedulerzjob.st 28b56671b2e825912a6427e609c2bbe1e7758e26 src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css ade850ce624964693e9bd55946464983c4b9f8c2
Re: Review Request 19833: Migrated Job page to angular JS
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/#review39909 --- pinging Kevin. - Suman Karumuri On April 4, 2014, 11:52 p.m., Suman Karumuri wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/ --- (Updated April 4, 2014, 11:52 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-281 https://issues.apache.org/jira/browse/AURORA-281 Repository: aurora Description --- *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. Added it here so only new code can be reviewed. *** Migrated job page to AngularJS. Removed old scheduler job page. Removed js related to the old scheduler page. Removed data tables. Added moment.js library for date time manipulation. Refactored code a bit for reusability. Stats link should still be added to the new jobs page. Since the role/env code is not committed, this diff contains that code as well. Code changes start from page 4 in diff between version 1 and 2. Diffs - build.gradle 109c193da3324bd5534b409bfabb6aeb0adda7b1 src/main/java/org/apache/aurora/scheduler/app/AppModule.java eeafc784e915137cacd5f64df1252ccbaf6c0f6c src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java ec56c649116c03ef148bac916bd6691a94685bc3 src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 19df7889f15b4cf44e386d8ce0626cc94fdcdfba src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java 2ccc6f367b9715a0abb3e0673069289ae4860087 src/main/java/org/apache/aurora/scheduler/http/ServletModule.java e3ff2571d95effcf72b2047cc5840d56143a180c src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java b35aad8db006133ba70692e43db1f083d4950914 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png 881de7976ff98955e2a5487dca66e618a0655f3d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png c608682b04a6d9b8002602450c8ef7e80ebba099 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png d300f1064b3beac1d7d5274e294494d3143e53a2 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png 6a6ded7de821619aedc71d1738c0b73463a4452e src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png fc46c5ebf0524b72a509fe2d7c1bc74995cb8a9d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc.png a88d7975fe9017e4e5f2289a94bd1ed66a5f59dc src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc_disabled.png 4e144cf0b1f786a9248a2998311e8109998d8a2d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_both.png 18670406bc01ab2721781822dd6478917745ff54 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc.png def071ed5afd264a036f6d9e75856366fd6ad153 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc_disabled.png 7824973cc60fc1841b16f2cb39323cefcdc3f942 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.bootstrap.js 420e507086f7a2af08f68787f67208546745260d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.htmlNumberType.js 885ad347f3076dcf75558ef44806dc25a8171884 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.localstorage.js 3c0b39aefbd5a82adee49f2ac7539b6974576b2f src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/jquery.dataTables.min.js 02694a4a56eb247baa2398d971927dfbd1ac3e60 src/main/resources/org/apache/aurora/scheduler/http/assets/dictionary.js 92045c41706ee9315f3522c3b5bbff7f6e7d2c64 src/main/resources/org/apache/aurora/scheduler/http/assets/util.js 5dd0d17849c7116d70925275b9c6c0715b441f14 src/main/resources/org/apache/aurora/scheduler/http/schedulerzjob.st 28b56671b2e825912a6427e609c2bbe1e7758e26 src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css ade850ce624964693e9bd55946464983c4b9f8c2
Re: Review Request 19833: Migrated Job page to angular JS
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/#review39642 --- Ship it! Overall LGTM. After you get a ship from Kevin, please be really careful with the final commit. Due to the accommodations made for review, i could see this one being easy to mess up. You might even push the final commit to a branch for another set of eyes before pushing to master. src/main/resources/org/apache/aurora/scheduler/http/ui/job.html https://reviews.apache.org/r/19833/#comment72189 Can you make this an anchor instead of span? Also, the issue i pointed out with the task links not working remains. Clicking the pi link, then clicking a task ID results in: Error fetching tasks: No tasks found for query: TaskQuery(owner:null, environment:task, jobName:1396742503655-mesos-test-adhocJob0-0-5a6d0345-56a5-41a9-89ec-70224fa826fc, taskIds:null, statuses:null, instanceIds:null, slaveHosts:null, jobKeys:null) src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js https://reviews.apache.org/r/19833/#comment72204 s/result/ranges/ src/main/resources/org/apache/aurora/scheduler/http/ui/schedulingDetail.html https://reviews.apache.org/r/19833/#comment72203 remove space before the colon, here and below - Bill Farner On April 4, 2014, 11:52 p.m., Suman Karumuri wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/ --- (Updated April 4, 2014, 11:52 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-281 https://issues.apache.org/jira/browse/AURORA-281 Repository: aurora Description --- *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. Added it here so only new code can be reviewed. *** Migrated job page to AngularJS. Removed old scheduler job page. Removed js related to the old scheduler page. Removed data tables. Added moment.js library for date time manipulation. Refactored code a bit for reusability. Stats link should still be added to the new jobs page. Since the role/env code is not committed, this diff contains that code as well. Code changes start from page 4 in diff between version 1 and 2. Diffs - build.gradle 109c193da3324bd5534b409bfabb6aeb0adda7b1 src/main/java/org/apache/aurora/scheduler/app/AppModule.java eeafc784e915137cacd5f64df1252ccbaf6c0f6c src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java ec56c649116c03ef148bac916bd6691a94685bc3 src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 19df7889f15b4cf44e386d8ce0626cc94fdcdfba src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java 2ccc6f367b9715a0abb3e0673069289ae4860087 src/main/java/org/apache/aurora/scheduler/http/ServletModule.java e3ff2571d95effcf72b2047cc5840d56143a180c src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java b35aad8db006133ba70692e43db1f083d4950914 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png 881de7976ff98955e2a5487dca66e618a0655f3d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png c608682b04a6d9b8002602450c8ef7e80ebba099 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png d300f1064b3beac1d7d5274e294494d3143e53a2 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png 6a6ded7de821619aedc71d1738c0b73463a4452e src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png fc46c5ebf0524b72a509fe2d7c1bc74995cb8a9d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc.png a88d7975fe9017e4e5f2289a94bd1ed66a5f59dc src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc_disabled.png 4e144cf0b1f786a9248a2998311e8109998d8a2d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_both.png 18670406bc01ab2721781822dd6478917745ff54 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc.png def071ed5afd264a036f6d9e75856366fd6ad153 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc_disabled.png
Re: Review Request 19833: Migrated Job page to angular JS
On April 1, 2014, 10:55 p.m., Bill Farner wrote:
src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java, line 134
https://reviews.apache.org/r/19833/diff/4/?file=544786#file544786line134
Can you address this TODO now? AFAICT there's nothing in the way of
that.
This option was moved from DisplayUtils.java. Punting on it for now.
On April 1, 2014, 10:55 p.m., Bill Farner wrote:
src/main/java/org/apache/aurora/scheduler/http/ServletModule.java, line 71
https://reviews.apache.org/r/19833/diff/4/?file=544789#file544789line71
revert
Done. This is automatically done by the code formatter in intellij and I have
to undo this every time. This is one of those code conventions I wish we didn't
follow.
On April 1, 2014, 10:55 p.m., Bill Farner wrote:
src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html,
line 12
https://reviews.apache.org/r/19833/diff/4/?file=544810#file544810line12
Can you put an ng-if on the span and remove the second if?
if environment
li
if job
span
I am not sure why, but doing it that way breaks the LI formatting. Leaving it
as is for now.
On April 1, 2014, 10:55 p.m., Bill Farner wrote:
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js,
line 279
https://reviews.apache.org/r/19833/diff/4/?file=544815#file544815line279
Can you wrap these in a more readable way? e.g.
{label: 'Instances',
map: 'range',
isSortable: false,
formatFunction: function (range) {
},
}
Ditto in other hashes that span multiple lines.
Done.
On April 1, 2014, 10:55 p.m., Bill Farner wrote:
src/main/resources/org/apache/aurora/scheduler/http/ui/js/filters.js, line 9
https://reviews.apache.org/r/19833/diff/4/?file=544817#file544817line9
Please be consistent about placing the + for strings that span multiple
lines.
Fixed. Code was copied from dictionary.js.
- Suman
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19833/#review39208
---
On April 1, 2014, 9:57 p.m., Suman Karumuri wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19833/
---
(Updated April 1, 2014, 9:57 p.m.)
Review request for Aurora, Kevin Sweeney and Bill Farner.
Bugs: AURORA-281
https://issues.apache.org/jira/browse/AURORA-281
Repository: aurora
Description
---
*** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/.
Added it here so only new code can be reviewed. ***
Migrated job page to AngularJS.
Removed old scheduler job page.
Removed js related to the old scheduler page. Removed data tables.
Added moment.js library for date time manipulation.
Refactored code a bit for reusability.
Stats link should still be added to the new jobs page.
Since the role/env code is not committed, this diff contains that code as
well.
Code changes start from page 4 in diff between version 1 and 2.
Diffs
-
build.gradle c2a70b0285c6afb20f6db387e50744424c572d3f
src/main/java/org/apache/aurora/scheduler/app/AppModule.java
eeafc784e915137cacd5f64df1252ccbaf6c0f6c
src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java
ec56c649116c03ef148bac916bd6691a94685bc3
src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java
19df7889f15b4cf44e386d8ce0626cc94fdcdfba
src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java
2ccc6f367b9715a0abb3e0673069289ae4860087
src/main/java/org/apache/aurora/scheduler/http/ServletModule.java
e3ff2571d95effcf72b2047cc5840d56143a180c
src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java
b35aad8db006133ba70692e43db1f083d4950914
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css
ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png
881de7976ff98955e2a5487dca66e618a0655f3d
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png
c608682b04a6d9b8002602450c8ef7e80ebba099
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png
d300f1064b3beac1d7d5274e294494d3143e53a2
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png
6a6ded7de821619aedc71d1738c0b73463a4452e
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png
a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1
Re: Review Request 19833: Migrated Job page to angular JS
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/ --- (Updated April 2, 2014, 11:26 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Changes --- Merged branch with master. Updated code per review comments. Pushed code to mansu/job_page branch for local testing. Bugs: AURORA-281 https://issues.apache.org/jira/browse/AURORA-281 Repository: aurora Description --- *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. Added it here so only new code can be reviewed. *** Migrated job page to AngularJS. Removed old scheduler job page. Removed js related to the old scheduler page. Removed data tables. Added moment.js library for date time manipulation. Refactored code a bit for reusability. Stats link should still be added to the new jobs page. Since the role/env code is not committed, this diff contains that code as well. Code changes start from page 4 in diff between version 1 and 2. Diffs (updated) - build.gradle 109c193da3324bd5534b409bfabb6aeb0adda7b1 src/main/java/org/apache/aurora/scheduler/app/AppModule.java eeafc784e915137cacd5f64df1252ccbaf6c0f6c src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java ec56c649116c03ef148bac916bd6691a94685bc3 src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 19df7889f15b4cf44e386d8ce0626cc94fdcdfba src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java 2ccc6f367b9715a0abb3e0673069289ae4860087 src/main/java/org/apache/aurora/scheduler/http/ServletModule.java e3ff2571d95effcf72b2047cc5840d56143a180c src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java b35aad8db006133ba70692e43db1f083d4950914 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png 881de7976ff98955e2a5487dca66e618a0655f3d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png c608682b04a6d9b8002602450c8ef7e80ebba099 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png d300f1064b3beac1d7d5274e294494d3143e53a2 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png 6a6ded7de821619aedc71d1738c0b73463a4452e src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png fc46c5ebf0524b72a509fe2d7c1bc74995cb8a9d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc.png a88d7975fe9017e4e5f2289a94bd1ed66a5f59dc src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc_disabled.png 4e144cf0b1f786a9248a2998311e8109998d8a2d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_both.png 18670406bc01ab2721781822dd6478917745ff54 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc.png def071ed5afd264a036f6d9e75856366fd6ad153 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc_disabled.png 7824973cc60fc1841b16f2cb39323cefcdc3f942 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.bootstrap.js 420e507086f7a2af08f68787f67208546745260d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.htmlNumberType.js 885ad347f3076dcf75558ef44806dc25a8171884 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.localstorage.js 3c0b39aefbd5a82adee49f2ac7539b6974576b2f src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/jquery.dataTables.min.js 02694a4a56eb247baa2398d971927dfbd1ac3e60 src/main/resources/org/apache/aurora/scheduler/http/assets/dictionary.js 92045c41706ee9315f3522c3b5bbff7f6e7d2c64 src/main/resources/org/apache/aurora/scheduler/http/assets/util.js 5dd0d17849c7116d70925275b9c6c0715b441f14 src/main/resources/org/apache/aurora/scheduler/http/schedulerzjob.st 28b56671b2e825912a6427e609c2bbe1e7758e26 src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css ade850ce624964693e9bd55946464983c4b9f8c2 src/main/resources/org/apache/aurora/scheduler/http/ui/index.html 36225d1e5147e30ba2cb4ddda96dec9f0f2f1dce src/main/resources/org/apache/aurora/scheduler/http/ui/job.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/js/app.js db6ea99aeb749fd8674613e3620dc3012872e13c
Re: Review Request 19833: Migrated Job page to angular JS
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/ --- (Updated April 1, 2014, 9:57 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Changes --- Updated the diff to just contain the changes related to replacing the job page. Using the --parent option, I have filtered the changes for the role/env page and excluded all the 3rd party js libs. Bugs: AURORA-281 https://issues.apache.org/jira/browse/AURORA-281 Repository: aurora Description --- *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. Added it here so only new code can be reviewed. *** Migrated job page to AngularJS. Removed old scheduler job page. Removed js related to the old scheduler page. Removed data tables. Added moment.js library for date time manipulation. Refactored code a bit for reusability. Stats link should still be added to the new jobs page. Since the role/env code is not committed, this diff contains that code as well. Code changes start from page 4 in diff between version 1 and 2. Diffs (updated) - build.gradle c2a70b0285c6afb20f6db387e50744424c572d3f src/main/java/org/apache/aurora/scheduler/app/AppModule.java eeafc784e915137cacd5f64df1252ccbaf6c0f6c src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java ec56c649116c03ef148bac916bd6691a94685bc3 src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 19df7889f15b4cf44e386d8ce0626cc94fdcdfba src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java 2ccc6f367b9715a0abb3e0673069289ae4860087 src/main/java/org/apache/aurora/scheduler/http/ServletModule.java e3ff2571d95effcf72b2047cc5840d56143a180c src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java b35aad8db006133ba70692e43db1f083d4950914 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png 881de7976ff98955e2a5487dca66e618a0655f3d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png c608682b04a6d9b8002602450c8ef7e80ebba099 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png d300f1064b3beac1d7d5274e294494d3143e53a2 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png 6a6ded7de821619aedc71d1738c0b73463a4452e src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png fc46c5ebf0524b72a509fe2d7c1bc74995cb8a9d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc.png a88d7975fe9017e4e5f2289a94bd1ed66a5f59dc src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc_disabled.png 4e144cf0b1f786a9248a2998311e8109998d8a2d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_both.png 18670406bc01ab2721781822dd6478917745ff54 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc.png def071ed5afd264a036f6d9e75856366fd6ad153 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc_disabled.png 7824973cc60fc1841b16f2cb39323cefcdc3f942 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.bootstrap.js 420e507086f7a2af08f68787f67208546745260d src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.htmlNumberType.js 885ad347f3076dcf75558ef44806dc25a8171884 src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.localstorage.js 3c0b39aefbd5a82adee49f2ac7539b6974576b2f src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/jquery.dataTables.min.js 02694a4a56eb247baa2398d971927dfbd1ac3e60 src/main/resources/org/apache/aurora/scheduler/http/assets/dictionary.js 92045c41706ee9315f3522c3b5bbff7f6e7d2c64 src/main/resources/org/apache/aurora/scheduler/http/assets/util.js 5dd0d17849c7116d70925275b9c6c0715b441f14 src/main/resources/org/apache/aurora/scheduler/http/schedulerzjob.st 28b56671b2e825912a6427e609c2bbe1e7758e26 src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css ade850ce624964693e9bd55946464983c4b9f8c2 src/main/resources/org/apache/aurora/scheduler/http/ui/index.html 36225d1e5147e30ba2cb4ddda96dec9f0f2f1dce src/main/resources/org/apache/aurora/scheduler/http/ui/job.html PRE-CREATION
Re: Review Request 19833: Migrated Job page to angular JS
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19833/#review39208
---
Looks great overall! Last request - can you push a branch to origin that
contains the proposed change? I'd like to try this out on a local scheduler.
src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java
https://reviews.apache.org/r/19833/#comment71570
Can you address this TODO now? AFAICT there's nothing in the way of that.
src/main/java/org/apache/aurora/scheduler/http/ServletModule.java
https://reviews.apache.org/r/19833/#comment71571
revert
src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html
https://reviews.apache.org/r/19833/#comment71572
Can you put an ng-if on the span and remove the second if?
if environment
li
if job
span
src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js
https://reviews.apache.org/r/19833/#comment71573
Can you wrap these in a more readable way? e.g.
{label: 'Instances',
map: 'range',
isSortable: false,
formatFunction: function (range) {
},
}
Ditto in other hashes that span multiple lines.
src/main/resources/org/apache/aurora/scheduler/http/ui/js/filters.js
https://reviews.apache.org/r/19833/#comment71574
Please be consistent about placing the + for strings that span multiple
lines.
- Bill Farner
On April 1, 2014, 9:57 p.m., Suman Karumuri wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19833/
---
(Updated April 1, 2014, 9:57 p.m.)
Review request for Aurora, Kevin Sweeney and Bill Farner.
Bugs: AURORA-281
https://issues.apache.org/jira/browse/AURORA-281
Repository: aurora
Description
---
*** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/.
Added it here so only new code can be reviewed. ***
Migrated job page to AngularJS.
Removed old scheduler job page.
Removed js related to the old scheduler page. Removed data tables.
Added moment.js library for date time manipulation.
Refactored code a bit for reusability.
Stats link should still be added to the new jobs page.
Since the role/env code is not committed, this diff contains that code as
well.
Code changes start from page 4 in diff between version 1 and 2.
Diffs
-
build.gradle c2a70b0285c6afb20f6db387e50744424c572d3f
src/main/java/org/apache/aurora/scheduler/app/AppModule.java
eeafc784e915137cacd5f64df1252ccbaf6c0f6c
src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java
ec56c649116c03ef148bac916bd6691a94685bc3
src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java
19df7889f15b4cf44e386d8ce0626cc94fdcdfba
src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java
2ccc6f367b9715a0abb3e0673069289ae4860087
src/main/java/org/apache/aurora/scheduler/http/ServletModule.java
e3ff2571d95effcf72b2047cc5840d56143a180c
src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java
b35aad8db006133ba70692e43db1f083d4950914
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css
ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png
881de7976ff98955e2a5487dca66e618a0655f3d
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png
c608682b04a6d9b8002602450c8ef7e80ebba099
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png
d300f1064b3beac1d7d5274e294494d3143e53a2
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png
6a6ded7de821619aedc71d1738c0b73463a4452e
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png
a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png
fc46c5ebf0524b72a509fe2d7c1bc74995cb8a9d
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc.png
a88d7975fe9017e4e5f2289a94bd1ed66a5f59dc
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc_disabled.png
4e144cf0b1f786a9248a2998311e8109998d8a2d
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_both.png
18670406bc01ab2721781822dd6478917745ff54
src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc.png
def071ed5afd264a036f6d9e75856366fd6ad153
Re: Review Request 19833: Migrated Job page to angular JS
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/ --- (Updated March 31, 2014, 6:09 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-281 https://issues.apache.org/jira/browse/AURORA-281 Repository: aurora Description --- *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. Added it here so only new code can be reviewed. *** Migrated job page to AngularJS. Removed old scheduler job page. Removed js related to the old scheduler page. Removed data tables. Added moment.js library for date time manipulation. Refactored code a bit for reusability. Stats link should still be added to the new jobs page. Since the role/env code is not committed, this diff contains that code as well. Code changes start from page 4 in diff between version 1 and 2. Diffs - 3rdparty/javascript/bower_components/angular-route/.bower.json PRE-CREATION 3rdparty/javascript/bower_components/angular-route/README.md PRE-CREATION 3rdparty/javascript/bower_components/angular-route/angular-route.js PRE-CREATION 3rdparty/javascript/bower_components/angular-route/angular-route.min.js PRE-CREATION 3rdparty/javascript/bower_components/angular-route/angular-route.min.js.map PRE-CREATION 3rdparty/javascript/bower_components/angular-route/bower.json PRE-CREATION 3rdparty/javascript/bower_components/momentjs/.bower.json PRE-CREATION 3rdparty/javascript/bower_components/momentjs/LICENSE PRE-CREATION 3rdparty/javascript/bower_components/momentjs/bower.json PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/ar-ma.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/ar.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/bg.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/br.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/bs.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/ca.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/cs.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/cv.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/cy.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/da.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/de.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/el.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/en-au.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/en-ca.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/en-gb.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/eo.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/es.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/et.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/eu.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/fa.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/fi.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/fo.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/fr-ca.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/fr.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/gl.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/he.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/hi.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/hr.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/hu.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/hy-am.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/id.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/is.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/it.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/ja.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/ka.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/ko.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/lb.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/lt.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/lv.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/mk.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/ml.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/mr.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/ms-my.js PRE-CREATION 3rdparty/javascript/bower_components/momentjs/lang/nb.js PRE-CREATION
Review Request 19833: Migrated Job page to angular JS
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19833/ --- Review request for Aurora, Kevin Sweeney and Bill Farner. Repository: aurora Description --- *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. Added it here so only new code can be reviewed. *** Migrated job page to AngularJS. Removed old scheduler job page. Removed js related to the old scheduler page. Removed data tables. Added moment.js library for date time manipulation. Refactored code a bit for reusability. Code changes start from page 5. Since the role/env code is not committed, this diff contains that code as well. Diffs - 3rdparty/javascript/bower_components/angular-route/.bower.json PRE-CREATION 3rdparty/javascript/bower_components/angular-route/README.md PRE-CREATION 3rdparty/javascript/bower_components/angular-route/angular-route.js PRE-CREATION 3rdparty/javascript/bower_components/angular-route/angular-route.min.js PRE-CREATION 3rdparty/javascript/bower_components/angular-route/angular-route.min.js.map PRE-CREATION 3rdparty/javascript/bower_components/angular-route/bower.json PRE-CREATION 3rdparty/javascript/bower_components/underscore/.bower.json PRE-CREATION 3rdparty/javascript/bower_components/underscore/.editorconfig PRE-CREATION 3rdparty/javascript/bower_components/underscore/.gitignore PRE-CREATION 3rdparty/javascript/bower_components/underscore/LICENSE PRE-CREATION 3rdparty/javascript/bower_components/underscore/README.md PRE-CREATION 3rdparty/javascript/bower_components/underscore/bower.json PRE-CREATION 3rdparty/javascript/bower_components/underscore/component.json PRE-CREATION 3rdparty/javascript/bower_components/underscore/package.json PRE-CREATION 3rdparty/javascript/bower_components/underscore/underscore.js PRE-CREATION src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 19df7889f15b4cf44e386d8ce0626cc94fdcdfba src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java e2f9ed0ea846c570de11b7dd85bc90aee6bc3342 src/main/java/org/apache/aurora/scheduler/http/ServletModule.java e3ff2571d95effcf72b2047cc5840d56143a180c src/main/resources/org/apache/aurora/scheduler/http/schedulerzrole.st b53f3524be052dcd5882c3e79e95e2f90aa071b8 src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css ade850ce624964693e9bd55946464983c4b9f8c2 src/main/resources/org/apache/aurora/scheduler/http/ui/error.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/home.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/index.html 36225d1e5147e30ba2cb4ddda96dec9f0f2f1dce src/main/resources/org/apache/aurora/scheduler/http/ui/jobLink.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/js/app.js db6ea99aeb749fd8674613e3620dc3012872e13c src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 7cd534479dd2f17ffd46248ce9af1f8fe89beb97 src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js d2b2017a0efc70d425fd6c89ad6caaf46cb8ded5 src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js 81cd12c4fea473192cd7e6b6dba245e4dde30b3d src/main/resources/org/apache/aurora/scheduler/http/ui/role.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/roleEnvLink.html PRE-CREATION src/main/resources/org/apache/aurora/scheduler/http/ui/roleLink.html fc25526389749e95efa87c719062dcea88935383 Diff: https://reviews.apache.org/r/19833/diff/ Testing --- ./gradlew clean build run on local laptop. Thanks, Suman Karumuri
