You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by srishtyagrawal <gi...@git.apache.org> on 2018/01/05 19:16:46 UTC
[GitHub] storm pull request #2505: STORM-2877: Add an option to configure pagination ...
GitHub user srishtyagrawal opened a pull request:
https://github.com/apache/storm/pull/2505
STORM-2877: Add an option to configure pagination in Storm UI
The current pagination default value for Storm UI is hard-coded to be 20. Pagination has been introduced in Storm 1.x. Having 20 items in the list restricts searching through the page. It will be beneficial to have a configuration option, `ui.pagination`, which will set the pagination value when Storm UI loads. This option can be added to storm.yaml along with other configurations.
Changed the value of `ui.pagination` in `storm.yaml` to test the following cases :
| Value in `storm.yaml` | Result
| ------------- |:-------------:|
| No `ui.pagination` | defaults to 20 entries |
| `“All”` | Throws error, expects integer |
| `-10` | List doesn't have any items in it |
| `30` | Populates list with 30 entries as default |
| `10` | Populates list with 10 entries as default|
| `10` | Populates list with 10 entries as default|
| `-1` | Equivalent of All from the drop-down options|
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/srishtyagrawal/storm STORM-2877
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2505.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2505
----
commit 3b1bfdbdbe98a141e20f475c8bc08d562a7bda0a
Author: Srishty Agrawal <sa...@...>
Date: 2018-01-03T22:51:25Z
STORM-2877: Add an option to configure pagination in Storm UI
----
---
[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...
Posted by srishtyagrawal <gi...@git.apache.org>.
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2505
@HeartSaVioR and @revans2 thanks for your comments. I had not noticed that the pagination values are saved per topology. It was good to understand the motive behind storing these values temporarily rather than in a cookie. I will make the changes in index.html to eliminate the additional call and update the PR.
---
[GitHub] storm pull request #2505: STORM-2877: Add an option to configure pagination ...
Posted by srishtyagrawal <gi...@git.apache.org>.
Github user srishtyagrawal commented on a diff in the pull request:
https://github.com/apache/storm/pull/2505#discussion_r160570956
--- Diff: storm-server/src/main/java/org/apache/storm/DaemonConfig.java ---
@@ -307,6 +307,12 @@
@isString
public static final String UI_CENTRAL_LOGGING_URL = "ui.central.logging.url";
+ /**
+ * Storm UI drop-down pagination value.
--- End diff --
Done!
---
[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2505
I am still +1.
---
[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...
Posted by srishtyagrawal <gi...@git.apache.org>.
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2505
Thanks @revans2, @HeartSaVioR and whoever merged this PR! I will create backport PRs soon.
---
[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2505
@HeartSaVioR It is totally possible to do. The problem we ran into was that I would change something in one tab (like searching for a specific keyword), and then refresh a different tab for a different topology and it would pick up the changes.
It was just not a good user experience because users would not know why something on one page/tab impacted things on another page/tab. That is why we started to make the setting topology specific and then made them session based to avoid leaks.
I will see if I can come up with a simpler way to deal with this.
---
[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...
Posted by srishtyagrawal <gi...@git.apache.org>.
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2505
@revans2 I have made the changes suggested by you. I have also built and tested the code against values mentioned in the first comment (the functionality is same as mentioned in the table).
---
[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2505
@HeartSaVioR That should be very simple to do. Right now we are storing the data in session storage.
https://github.com/apache/storm/blob/7ecb3d73e8e909c01d39e03a7a7ed45a2fb81859/storm-core/src/ui/public/component.html#L147-L153
We could switch it over to something more permanent, that would survive reboots. The reason we didn't do that before is because we store the data per-topology for some pages.
https://github.com/apache/storm/blob/7ecb3d73e8e909c01d39e03a7a7ed45a2fb81859/storm-core/src/ui/public/component.html#L140
And keeping that data permanently would be bad for users as it would be a memory leak for them.
We could switch it to permanent for non-topology specific pages (a few lines of change). Also if you really want to we could add a new page to switch the defaults for this browser on all topology pages, but that feels a bit more difficult to get right, and harder to maintain if the page layout changes.
---
[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2505
Could we allow users to modify the entries on the fly (dropdown) and cache it via cookie so that the change is applied to only that user (specifically such browser)? For me page length is more alike personalization and specific value may not make all of users (in a cluster) happy.
---
[GitHub] storm pull request #2505: STORM-2877: Add an option to configure pagination ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2505#discussion_r160239743
--- Diff: storm-server/src/main/java/org/apache/storm/DaemonConfig.java ---
@@ -307,6 +307,12 @@
@isString
public static final String UI_CENTRAL_LOGGING_URL = "ui.central.logging.url";
+ /**
+ * Storm UI drop-down pagination value.
--- End diff --
Could you also add in some example values that can be set in the javadocs. Just so people know what to do.
---
[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2505
@srishtyagrawal yes my concern was mostly with the number of times that the REST calls were being hit. If you can reduce it so no new rest calls come in, that would be great.
---
[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...
Posted by srishtyagrawal <gi...@git.apache.org>.
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2505
@revans2 @HeartSaVioR can you merge this PR?
---
[GitHub] storm pull request #2505: STORM-2877: Add an option to configure pagination ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2505
---
[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...
Posted by srishtyagrawal <gi...@git.apache.org>.
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2505
@revans2 if I understand correctly your concern is that every time a table is paginated for a given page, there will be a call to get the cluster configuration. I verified that for multiple paginated tables within the same page there will be only 1 call to fetch cluster configuration.
<img width="1439" alt="screen shot 2018-01-05 at 5 27 52 pm" src="https://user-images.githubusercontent.com/564198/34634933-04284538-f23e-11e7-865b-a473c2af50d7.png">
In the above picture, there are 2 tables which have pagination (because of the low minEntriesToShowPagination value) and there is just 1 call to configuration (cluster/configuration endpoint).
I noticed that `index.html` already has a getJSON call to fetch cluster/configuration for `Nimbus Configuration` table so I can make changes to not introduce another one for fetching the pagination value.
---
[GitHub] storm pull request #2505: STORM-2877: Add an option to configure pagination ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2505#discussion_r160736089
--- Diff: storm-core/src/ui/public/index.html ---
@@ -106,92 +106,92 @@ <h2>Nimbus Configuration</h2>
$.blockUI({ message: '<img src="images/spinner.gif" /> <h3>Loading summary...</h3>'});
});
$(document).ready(function() {
- $.extend( $.fn.dataTable.defaults, {
- stateSave: true,
- lengthMenu: [[20,40,60,100,-1], [20, 40, 60, 100, "All"]],
- pageLength: 20
- });
-
- $.ajaxSetup({
- "error":function(jqXHR,textStatus,response) {
- var errorJson = jQuery.parseJSON(jqXHR.responseText);
- getStatic("/templates/json-error-template.html", function(template) {
- $("#json-response-error").append(Mustache.render($(template).filter("#json-error-template").html(),errorJson));
- });
- }
- });
- var uiUser = $("#ui-user");
- var clusterSummary = $("#cluster-summary");
- var clusterResources = $("#cluster-resources");
- var nimbusSummary = $("#nimbus-summary");
- var ownerSummary = $("#owner-summary");
- var topologySummary = $("#topology-summary");
- var supervisorSummary = $("#supervisor-summary");
- var config = $("#nimbus-configuration");
-
- getStatic("/templates/index-page-template.html", function(indexTemplate) {
- $.getJSON("/api/v1/cluster/summary",function(response,status,jqXHR) {
- getStatic("/templates/user-template.html", function(template) {
- uiUser.append(Mustache.render($(template).filter("#user-template").html(),response));
- $('#ui-user [data-toggle="tooltip"]').tooltip()
- });
-
- clusterSummary.append(Mustache.render($(indexTemplate).filter("#cluster-summary-template").html(),response));
- $('#cluster-summary [data-toggle="tooltip"]').tooltip();
-
- clusterResources.append(Mustache.render($(indexTemplate).filter("#cluster-resources-template").html(),response));
- $('#cluster-resources [data-toggle="tooltip"]').tooltip();
+ $.getJSON("/api/v1/cluster/configuration",function(response,status,jqXHR) {
--- End diff --
nit: Could we rename this configuration response and status? They are over-ridden by the other ajax calls and I would prefer that it is not confusing in the code which variable is being accessed at which time.
---
[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2505
@srishtyagrawal I am fine with the change in principal, but I am not happy with an implementation that adds many new called to get the cluster configuration. One for each table on the page.
---
[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...
Posted by srishtyagrawal <gi...@git.apache.org>.
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2505
nit has been addressed and tested.
@revans2, @HeartSaVioR I have also squashed the commits into 1.
---
[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...
Posted by srishtyagrawal <gi...@git.apache.org>.
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2505
@HeartSaVioR, I agree that saving pagination for a user makes more sense. Currently, this information is saved per session for a user. [This is being done using stateSave functionality](https://datatables.net/examples/basic_init/state_save.html).
Do we still want a cookie to store the pagination value?
---
[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2505
@srishtyagrawal No, I wasn't aware of stateSave hence thought about implementing session. Please ignore about cookie.
@revans2 Thanks for the info. I see how it will work.
For me it would be ideal if we can extract paging size from state data and store/load it globally (in stateSaveCallback/stateLoadCallback), so table in topology B starts from length 50 if I changed the length from somewhere else. It will require understanding of state data in DataTables (and upgrading DataTables might break the functionality) and I'm not sure even it is possible, so please treat this as just a 2 cents.
---
[GitHub] storm pull request #2505: STORM-2877: Add an option to configure pagination ...
Posted by srishtyagrawal <gi...@git.apache.org>.
Github user srishtyagrawal commented on a diff in the pull request:
https://github.com/apache/storm/pull/2505#discussion_r160776471
--- Diff: storm-core/src/ui/public/index.html ---
@@ -106,92 +106,92 @@ <h2>Nimbus Configuration</h2>
$.blockUI({ message: '<img src="images/spinner.gif" /> <h3>Loading summary...</h3>'});
});
$(document).ready(function() {
- $.extend( $.fn.dataTable.defaults, {
- stateSave: true,
- lengthMenu: [[20,40,60,100,-1], [20, 40, 60, 100, "All"]],
- pageLength: 20
- });
-
- $.ajaxSetup({
- "error":function(jqXHR,textStatus,response) {
- var errorJson = jQuery.parseJSON(jqXHR.responseText);
- getStatic("/templates/json-error-template.html", function(template) {
- $("#json-response-error").append(Mustache.render($(template).filter("#json-error-template").html(),errorJson));
- });
- }
- });
- var uiUser = $("#ui-user");
- var clusterSummary = $("#cluster-summary");
- var clusterResources = $("#cluster-resources");
- var nimbusSummary = $("#nimbus-summary");
- var ownerSummary = $("#owner-summary");
- var topologySummary = $("#topology-summary");
- var supervisorSummary = $("#supervisor-summary");
- var config = $("#nimbus-configuration");
-
- getStatic("/templates/index-page-template.html", function(indexTemplate) {
- $.getJSON("/api/v1/cluster/summary",function(response,status,jqXHR) {
- getStatic("/templates/user-template.html", function(template) {
- uiUser.append(Mustache.render($(template).filter("#user-template").html(),response));
- $('#ui-user [data-toggle="tooltip"]').tooltip()
- });
-
- clusterSummary.append(Mustache.render($(indexTemplate).filter("#cluster-summary-template").html(),response));
- $('#cluster-summary [data-toggle="tooltip"]').tooltip();
-
- clusterResources.append(Mustache.render($(indexTemplate).filter("#cluster-resources-template").html(),response));
- $('#cluster-resources [data-toggle="tooltip"]').tooltip();
+ $.getJSON("/api/v1/cluster/configuration",function(response,status,jqXHR) {
--- End diff --
Done!
---
[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...
Posted by srishtyagrawal <gi...@git.apache.org>.
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2505
@revans2
---