DomGarguilo commented on code in PR #3612:
URL: https://github.com/apache/accumulo/pull/3612#discussion_r1263855863
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/tables.ftl:
##########
@@ -91,7 +114,7 @@
</script>
<div class="row">
<div class="col-xs-12">
- <h3>Table Overview</h3>
+ <h3>Table OVERVIEW</h3>
Review Comment:
Was this changed due to preference? It doesn't look like other parts of the
monitor use all caps like this.
##########
server/monitor/src/main/java/org/apache/accumulo/monitor/rest/tables/TableInformation.java:
##########
@@ -60,12 +60,12 @@ public class TableInformation {
// running scans with queued in parenthesis
public String scansCombo;
- private int queuedMajorCompactions;
- private int runningMajorCompactions;
- private int queuedMinorCompactions;
- private int runningMinorCompactions;
- private int queuedScans;
- private int runningScans;
+ public int queuedMajorCompactions;
+ public int runningMajorCompactions;
+ public int queuedMinorCompactions;
+ public int runningMinorCompactions;
+ public int queuedScans;
+ public int runningScans;
Review Comment:
Why change these to public? It doesn't look like they are used outside of
this class.
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/table.ftl:
##########
@@ -50,6 +50,12 @@
<th class="percent" title="The recent index cache hit
rate.">Index Cache<br />Hit Rate </th>
<th class="percent" title="The recent data cache hit
rate.">Data Cache<br />Hit Rate </th>
<th class="big-num" title="The Unix one minute load average.
The average number of processes in the run queue over a one minute
interval.">OS Load </th>
+ <th title="Running Scans">Running Scans</th>
+ <th title="Queued Scans">Queued Scans</th>
+ <th title="Running MinC">Running MinC</th>
+ <th title="Queued MinC">Queued MinC</th>
+ <th title="Running MajC">Running MajC</th>
+ <th title="Queued MajC">Queued MajC</th>
Review Comment:
```suggestion
```
I did some testing and it seems that these columns do not need to be added
to the html since they are made invisible anyways in the js. DataTables can
still use the values as expected without there being columns present in the
html.
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/table.js:
##########
@@ -133,10 +148,30 @@ function initTableServerTable(tableID) {
},
{
"data": "osload"
+ },
+ {
+ "data": "scansRunning"
Review Comment:
```suggestion
"data": "scansRunning",
"visible": false
```
I think it makes things easier to follow if the visibility is defined here,
along with the `data`, instead of all at once with the line below:
`tableServersTable.columns([13, 14, 15, 16, 17, 18]).visible(false);`.
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/table.js:
##########
@@ -80,6 +80,21 @@ function initTableServerTable(tableID) {
}
return data;
}
+ },
+ {
+ "targets": [7],
+ "type": "numeric",
+ "orderData": [13, 14]
+ },
Review Comment:
I think it would be helpful to add comments to clear up what this new logic
does. Maybe a general one and then a specific one per-column like in this
example:
```suggestion
// ensure these columns are sorted by the numeric values that comprise
the combined string instead of sorting them alphabetically by the string itself
// sort scans first by number of running, then by number of queued
{
"targets": [7],
"type": "numeric",
"orderData": [13, 14]
},
```
##########
server/monitor/src/main/java/org/apache/accumulo/monitor/rest/tservers/TabletServerInformation.java:
##########
@@ -64,17 +64,17 @@ public class TabletServerInformation {
// New variables
public String ip;
- private Integer scansRunning;
- private Integer scansQueued;
+ public Integer scansRunning;
+ public Integer scansQueued;
// combo string with running value and number queued in parenthesis
public String minorCombo;
public String majorCombo;
public String scansCombo;
- private Integer minorRunning;
- private Integer minorQueued;
+ public Integer minorRunning;
+ public Integer minorQueued;
- private Integer majorRunning;
- private Integer majorQueued;
+ public Integer majorRunning;
+ public Integer majorQueued;
Review Comment:
Same here. Do they need to be changed to public for some reason?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
