[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...

2018-01-05 Thread srishtyagrawal
Github user srishtyagrawal commented on the issue: https://github.com/apache/storm/pull/2505 @revans2 ---

[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...

2018-01-05 Thread revans2
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...

2018-01-05 Thread srishtyagrawal
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 pa

[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...

2018-01-07 Thread HeartSaVioR
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 al

[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...

2018-01-08 Thread revans2
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

[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...

2018-01-08 Thread revans2
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...

2018-01-08 Thread srishtyagrawal
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

[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...

2018-01-08 Thread HeartSaVioR
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

[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...

2018-01-08 Thread revans2
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 differ

[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...

2018-01-08 Thread srishtyagrawal
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 tempor

[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...

2018-01-09 Thread srishtyagrawal
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...

2018-01-10 Thread srishtyagrawal
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...

2018-01-10 Thread revans2
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...

2018-01-12 Thread srishtyagrawal
Github user srishtyagrawal commented on the issue: https://github.com/apache/storm/pull/2505 @revans2 @HeartSaVioR can you merge this PR? ---

[GitHub] storm issue #2505: STORM-2877: Add an option to configure pagination in Stor...

2018-01-17 Thread srishtyagrawal
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. ---