-
Notifications
You must be signed in to change notification settings - Fork 304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PAYARA-2871 Improve listing batch jobs memory usage in admin console #2965
Conversation
Jenkins test please |
Quick build and test failed! |
Jenkins test please |
Quick build and test passed! |
schema = batchRuntimeConfiguration.getSchemaName(); | ||
tableNames = getSharedTableMap(); | ||
schemaTableNames = getSharedSchemaTableMap(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting has gone a bit wonky here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks overall very good! Just one or two things that need addressing.
*/ | ||
public class BatchHandlers { | ||
|
||
private static final int DEFAULT_OFFSET_VALUE = 20; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small thing, but for readability can this be renamed to DEFAULT_OFFSET_INCREMENT
or something like that? It doesn't function as a default offset, but as an increment value
*/ | ||
|
||
<f:verbatim> | ||
<script type="text/javascript"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an unusual way of adding CSS. Usually this would be done by using CSS selectors carefully, is this not possible here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have looked at different ways to do it, but IMO this is the cleanest way and makes it easily maintainable too.
@@ -79,24 +79,39 @@ | |||
@Param(primary = true, optional = true) | |||
String jobName; | |||
|
|||
@Param(name = "offset", optional = true, defaultValue = "0") | |||
String offSetValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure to add validation to these to prevent negative errors etc! May require adding to both the proxy and the main command. Will probably need a rework sometime in the future, but I would just validate both for now.
Jenkins test please |
Quick build and test passed! |
PAYARA-2871 Improve listing batch jobs memory usage in admin console
No description provided.