Make pageSize configurable #1797

fwantastic · 2018-06-06T20:27:21Z

This will address the issue #1796. A new property called azkaban.display.numOfExecutions will be added in the azkaban.properties and be used to determine the number of rows to be displayed.

As it will default to 16 when the property does not exist, it will not have any impact on the existing page size.

This will address the issue #1796. A new property called azkaban.display.numOfExecutions will be added in the azkaban.properties and be used to determine the number of rows to be displayed.

HappyRay · 2018-06-06T20:34:54Z

Thanks.

Please see the config key naming conventions at
https://github.com/HappyRay/azkaban/blob/98497ede72ee47ddabaa2d57038171f6161e2040/az-core/src/main/java/azkaban/Constants.java#L34

fwantastic · 2018-06-07T15:36:50Z

Names have been changed to follow the naming convention!

fwantastic · 2018-06-07T15:58:17Z

Thanks for the feedback.
I do like what you suggested - azkaban.display.execution_page_size. I went for azkaban.page.size.xxx as if there's another page that will have non-execution rows and needs a different page size.

I'll check out the SaveAction plugin, I thought it only supported intellij and I'm using eclipse but I guess I was wrong!

Also added a new line at the end in the properties file

fwantastic · 2018-06-07T18:35:59Z

Installed intellij and applied the style sheet, though it didn't add a new line at the end so I manually added one. Also refactor - rename variable doesn't work properly so I had to rename them one by one as well!

HappyRay · 2018-06-28T00:46:01Z

azkaban-web-server/src/main/java/azkaban/webapp/servlet/AbstractAzkabanServlet.java

@@ -106,6 +108,7 @@ public void init(final ServletConfig config) throws ServletException {
    this.label = props.getString("azkaban.label", "");
    this.color = props.getString("azkaban.color", "#FF0000");
    this.passwordPlaceholder = props.getString("azkaban.password.placeholder", "Password");
+    this.displayExecutionPageSize = props.getInt("azkaban.display.execution_page_size", 16);


Please define new config keys in the Constants class.

HappyRay · 2018-06-28T00:46:37Z

azkaban-web-server/src/main/java/azkaban/webapp/servlet/AbstractAzkabanServlet.java

@@ -320,6 +323,8 @@ protected Page newPage(final HttpServletRequest req, final HttpServletResponse r
      page.add("triggerPlugins", this.triggerPlugins);
    }

+    page.add("displayExecutionPageSize", displayExecutionPageSize);


Make the key "displayExecutionPageSize" a constant?

HappyRay · 2018-06-28T00:47:58Z

azkaban-web-server/src/main/java/azkaban/webapp/servlet/AbstractAzkabanServlet.java

+  public int getDisplayExecutionPageSize() {
+    return displayExecutionPageSize;
+  }
+}


Add new line.

fwantastic · 2018-07-02T20:12:06Z

keys were extracted to a constant.

fwantastic · 2018-07-03T18:47:21Z

hm not sure why the build is failing. any ideas?

HappyRay · 2018-07-09T16:00:31Z

I would download the raw travis log and look for test failures.

How to find test failure from the ci log

Download the raw log
Open in an editor such as intellij:
Use the reg expression
Test > \w* FAILED
E.g.
Found
azkaban.execapp.event.JobCallbackRequestMakerTest > simulateNotOKStatusCodeTest FAILED

HappyRay · 2018-07-09T16:03:12Z

I added the tip to https://github.com/azkaban/azkaban/wiki/Developer-Tools-and-Tips

codecov · 2018-07-18T17:13:50Z

Codecov Report

Merging #1797 into master will increase coverage by 0.05%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #1797      +/-   ##
============================================
+ Coverage     32.88%   32.94%   +0.05%     
+ Complexity     2633     2632       -1     
============================================
  Files           404      404              
  Lines         29066    29068       +2     
  Branches       3699     3699              
============================================
+ Hits           9557     9575      +18     
+ Misses        18634    18614      -20     
- Partials        875      879       +4

Impacted Files	Coverage Δ	Complexity Δ
az-core/src/main/java/azkaban/Constants.java	`25% <ø> (ø)`	`1 <0> (ø)`	⬇️
...in/java/azkaban/webapp/servlet/HistoryServlet.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...azkaban/webapp/servlet/AbstractAzkabanServlet.java	`6.87% <0%> (-0.11%)`	`6 <0> (ø)`
...n/java/azkaban/execapp/jmx/JmxJobMBeanManager.java	`66.66% <0%> (-3.04%)`	`14% <0%> (-1%)`
...c/main/java/azkaban/execapp/event/FlowWatcher.java	`87.5% <0%> (-2.09%)`	`16% <0%> (-1%)`
...n/java/azkaban/flowtrigger/FlowTriggerService.java	`72.33% <0%> (-0.4%)`	`59% <0%> (-1%)`
...erver/src/main/java/azkaban/execapp/JobRunner.java	`70.62% <0%> (+0.43%)`	`87% <0%> (+1%)`	⬆️
...azkaban/execapp/event/JobCallbackRequestMaker.java	`69.79% <0%> (+20.83%)`	`7% <0%> (+1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d13ce99...4d94157. Read the comment docs.

coveralls · 2018-07-18T17:16:00Z

Pull Request Test Coverage Report for Build 3990

0 of 3 (0.0%) changed or added relevant lines in 2 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.07%) to 36.082%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
azkaban-web-server/src/main/java/azkaban/webapp/servlet/HistoryServlet.java	0	1	0.0%
azkaban-web-server/src/main/java/azkaban/webapp/servlet/AbstractAzkabanServlet.java	0	2	0.0%

Files with Coverage Reduction	New Missed Lines	%
azkaban-exec-server/src/main/java/azkaban/execapp/jmx/JmxJobMBeanManager.java	1	72.73%
azkaban-exec-server/src/main/java/azkaban/execapp/JobRunner.java	1	77.11%

Totals
Change from base Build 3982:	0.07%
Covered Lines:	10437
Relevant Lines:	28926

💛 - Coveralls

fwantastic · 2018-07-25T15:19:05Z

@HappyRay the build seems to be fine, what do you think about those code coverages?

HappyRay · 2018-07-25T16:43:00Z

az-core/src/main/java/azkaban/Constants.java

@@ -226,6 +226,20 @@
    public static final String QUEUEPROCESSING_ENABLED = "azkaban.queueprocessing.enabled";

    public static final String SESSION_TIME_TO_LIVE = "session.time.to.live";
+
+    // allowed max size of project dir in mb
+    public static final String PROJECT_DIR_MAX_SIZE = "azkaban.project_cache_max_size_in_mb";


Why does this change show up here? This seems to belong to another change.

@HappyRay there was a merge conflict while I was updating my branch to master's head. Would it be better to close this PR and open a new one?

HappyRay · 2018-07-25T16:44:46Z

az-core/src/main/java/azkaban/Constants.java

+
+  public static class PageProperties {
+    // number of rows to be displayed on the executions page.
+    public static final String DISPLAY_EXECUTION_PAGE_SIZE = "displayExecutionPageSize";


What's this for?

HappyRay · 2018-07-25T16:47:12Z

@kunkun-tang has added additional support for our new documentation system. Going forward, we would expect changes to include documentation changes as needed in the same PR whenever possible. That work is still in progress.

Could you work with him to see if it is doable now?

@kunkun-tang
could you review this change?

fwantastic · 2018-08-07T20:34:11Z

@HappyRay I'm happy to provide documentations for the changes. The last commit I made was for the merge conflicts that CI was flagging about. This PR's changes are pretty simple and straightforward.

fwantastic · 2018-08-09T15:15:53Z

to avoid any confusions, I've reverted the merge.

fwantastic · 2018-08-17T14:00:20Z

@HappyRay bump!

HappyRay · 2018-08-17T15:31:13Z

Please work with @kunkun-tang

fwantastic · 2018-08-20T13:57:00Z

@kunkun-tang could you review the changes please?!

kunkun-tang · 2018-08-20T17:26:22Z

@fwantastic Please rebase from the latest upstream master. Will start reviewing it from there. Thanks!

This will address the issue #1796. A new property called azkaban.display.numOfExecutions will be added in the azkaban.properties and be used to determine the number of rows to be displayed.

Also added a new line at the end in the properties file

… pagesize

fwantastic · 2018-08-20T19:26:11Z

@kunkun-tang I've rebased the branch and should be ready for review. I accidently committed a blank file so I removed it in the last commit

kunkun-tang

Generally LGTM. Only some trivials

kunkun-tang · 2018-08-21T01:49:41Z

azkaban-solo-server/src/main/resources/conf/azkaban.properties

@@ -41,3 +41,5 @@ jetty.connector.stats=true
 executor.connector.stats=true
 # Azkaban plugin settings
 azkaban.jobtype.plugin.dir=plugins/jobtypes
+#number of executions to be displayed
+azkaban.display.execution_page_size=100


Isn't it too large for default solo server?

kunkun-tang · 2018-08-21T01:51:04Z

az-core/src/main/java/azkaban/Constants.java

@@ -229,6 +229,9 @@

    // allowed max size of shared project dir in MB
    public static final String PROJECT_DIR_MAX_SIZE_IN_MB = "azkaban.project_cache_max_size_in_mb";
+
+    // number of rows to be displayed on the executions page.
+    public static final String DISPLAY_EXECUTION_PAGE_SIZE = "azkaban.display.execution_page_size";


This should be removed, right?

kunkun-tang · 2018-08-21T01:51:56Z

azkaban-web-server/src/main/java/azkaban/webapp/servlet/AbstractAzkabanServlet.java

@@ -369,4 +379,8 @@ protected void writeJSON(final HttpServletResponse resp, final Object obj, final
    resp.setContentType(JSON_MIME_TYPE);
    JSONUtils.toJSON(obj, resp.getOutputStream(), true);
  }
+
+  public int getDisplayExecutionPageSize() {


We may don't need to introduce public here? package-access is good enough?

kunkun-tang · 2018-08-21T01:53:07Z

azkaban-web-server/src/main/resources/azkaban/webapp/servlet/velocity/flowpage.vm

@@ -48,6 +48,8 @@
    var projectName = "${project.name}";
    var flowId = "${flowid}";
    var execId = null;
+
+    var displayExecutionPageSize = "${displayExecutionPageSize}";


Why it introduces an extra blank line?

- removing unused constants - making the default value 16 for the page size property - updating getDisplayExecutionPageSize method to package private - improving variable names

kunkun-tang · 2018-08-23T17:07:34Z

Thanks for the contribution!

kunkun-tang · 2018-08-30T18:28:57Z

Hey @fwantastic , have you tested this feature before?

fwantastic · 2018-08-30T18:41:04Z

hey @kunkun-tang, I've tested this manually on my machine but there's no test for it. was there any issues with this?

Found a UI bug in #1797 , when we deploy latest master to our staging cluster. The observation of this bug is all execution history table is missing. The cause looks like a javascript or backbone grammar issue. The fix is quite simple.

kunkun-tang · 2018-08-31T23:38:35Z

Yes. It turned out the page still missed execution entires when it is being initialized. I'm able to reproduce the issue in my local solo server.

Per my understandg , Before {size} is fully loaded, the javascript started to call it. @fwantastic . And Javascript cannot find the value.

Looks like my fix #1797 doesn't work during the initialization phase as well.

fwantastic · 2018-09-01T02:20:29Z

@kunkun-tang I cloned master but I'm unable to replicate the issue on my end. I'm running the solo server locally on win 10 + cygwin + h2. However, I think I found the issue.
In flow.js, the line you changed is actually not needed, it should be reverted to this:

this.model.set({page: 1, pageSize: this.pageSize});

In historypage.vm, this should be included in 'head' section, just like line 51 in flowpage.vm

var pageSize = "${size}";

I've made these changes super long time ago on my old project, I think I tried to make the page size configurable on executions page as well and probably I missed to include it on historypage.vm on this pull request. But I really do not understand how it's working on mine.
Could you please try this and let me know?

kunkun-tang · 2018-09-04T04:26:32Z

Looks like flowpage.vm already exists the change you mentioned.

I think I found the issue. We need to add "page" parameter in

    page.add("size", getDisplayExecutionPageSize());

in AbstractAzkabanServlet class. @fwantastic

fwantastic · 2018-09-04T13:05:56Z

@kunkun-tang but that is already added in HistoryServlet's handleHistoryPage method
at line 139:

page.add("size", pageSize);

and pageSize is declared as

final int pageSize = getIntParam(req, "size", getDisplayExecutionPageSize());

kunkun-tang · 2018-09-04T16:44:06Z

That is for History Page. Looks like flow page also needs. @fwantastic

#1797 introduced a bug that executions history cannot be displayed in executions tab give every project The issue is that javascript cannot find ${size}. The resolution is simple that we should add "size" to the context so that javascript is able to locate it.

fwantastic · 2018-09-05T14:42:52Z

@kunkun-tang Oh I was dumb.. I thought you were talking about the history page this whole time. Yes I totally forgot that this PR covers the flow page as well and I shouldn't have removed the size property in abstract servlet class. You are correct.

Make pageSize configurable

ef4a00f

This will address the issue #1796. A new property called azkaban.display.numOfExecutions will be added in the azkaban.properties and be used to determine the number of rows to be displayed.

renaming variables and properties to match the naming convention

c0bea3a

fwantastic added 2 commits June 7, 2018 14:06

renaming page size variables again to be more intuitive

8a2872d

Also added a new line at the end in the properties file

renaming the variable from the previous commit

7f3af90

HappyRay reviewed Jun 28, 2018

View reviewed changes

fwantastic added 2 commits July 2, 2018 15:46

adding a new line at the end of file

7255040

Extracting constants for display execution page size keys

d8218fb

HappyRay reviewed Jul 25, 2018

View reviewed changes

Make pageSize configurable

0736f8e

This will address the issue #1796. A new property called azkaban.display.numOfExecutions will be added in the azkaban.properties and be used to determine the number of rows to be displayed.

fwantastic added 7 commits August 20, 2018 14:52

renaming variables and properties to match the naming convention

5de9eab

renaming page size variables again to be more intuitive

e9268e2

Also added a new line at the end in the properties file

renaming the variable from the previous commit

bae2438

adding a new line at the end of file

38dc004

Extracting constants for display execution page size keys

5bf4792

Merge branch 'pagesize' of https://github.com/fwantastic/azkaban into…

fa667c0

… pagesize

removing the dummy file

e4faca7

kunkun-tang reviewed Aug 21, 2018

View reviewed changes

applying changes from code review

4d94157

- removing unused constants - making the default value 16 for the page size property - updating getDisplayExecutionPageSize method to package private - improving variable names

kunkun-tang approved these changes Aug 23, 2018

View reviewed changes

kunkun-tang merged commit a39e811 into azkaban:master Aug 23, 2018

fwantastic deleted the pagesize branch August 27, 2018 18:18

kunkun-tang mentioned this pull request Aug 30, 2018

fix PR 1797, a UI bug #1938

Merged

kunkun-tang mentioned this pull request Sep 4, 2018

add "size" parameter during initializing a new page. #1941

Merged

Make pageSize configurable #1797

Make pageSize configurable #1797

Conversation

fwantastic commented Jun 6, 2018

HappyRay commented Jun 6, 2018

fwantastic commented Jun 7, 2018

fwantastic commented Jun 7, 2018

fwantastic commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwantastic commented Jul 2, 2018

fwantastic commented Jul 3, 2018

HappyRay commented Jul 9, 2018

HappyRay commented Jul 9, 2018

codecov bot commented Jul 18, 2018 • edited Loading

Codecov Report

coveralls commented Jul 18, 2018 • edited Loading

Pull Request Test Coverage Report for Build 3990

💛 - Coveralls

fwantastic commented Jul 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HappyRay commented Jul 25, 2018

fwantastic commented Aug 7, 2018

fwantastic commented Aug 9, 2018

fwantastic commented Aug 17, 2018

HappyRay commented Aug 17, 2018

fwantastic commented Aug 20, 2018

kunkun-tang commented Aug 20, 2018

fwantastic commented Aug 20, 2018

kunkun-tang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunkun-tang commented Aug 23, 2018

kunkun-tang commented Aug 30, 2018

fwantastic commented Aug 30, 2018

kunkun-tang commented Aug 31, 2018 • edited Loading

fwantastic commented Sep 1, 2018 • edited Loading

kunkun-tang commented Sep 4, 2018 • edited Loading

fwantastic commented Sep 4, 2018 • edited Loading

kunkun-tang commented Sep 4, 2018

fwantastic commented Sep 5, 2018

codecov bot commented Jul 18, 2018 •

edited

Loading

coveralls commented Jul 18, 2018 •

edited

Loading

kunkun-tang commented Aug 31, 2018 •

edited

Loading

fwantastic commented Sep 1, 2018 •

edited

Loading

kunkun-tang commented Sep 4, 2018 •

edited

Loading

fwantastic commented Sep 4, 2018 •

edited

Loading