You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/04/28 20:58:45 UTC
[GitHub] [accumulo] DomGarguilo opened a new pull request, #2663: Highlight rows with recovering tservers in the tserver status table
DomGarguilo opened a new pull request, #2663:
URL: https://github.com/apache/accumulo/pull/2663
Fixes #2566
This PR highlights the rows of the tservers that are recovering. It also adds a caption when there are highlighted rows.
Since recovery happens so quick, to test this I hardcoded an entry into the list of recovering servers which can be done by adding back the code removed in feba7196386185e98b2e80bae6df7cf8feca01cd.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2663: Highlight rows with recovering tservers in the tserver status table
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2663:
URL: https://github.com/apache/accumulo/pull/2663#discussion_r862017114
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/tservers.ftl:
##########
@@ -49,6 +49,7 @@
</thead>
<tbody></tbody>
</table>
+ <span id="recovery-caption" style="background-color: gold;"></span>
Review Comment:
I can't get this working for some reason. I was able to apply the class to the caption, but the datatable will not show the change and I'm not sure why.
Edit: Adding the class only seems to apply to even rows, even if I remove the existing `odd` and `even` classes from them so not too sure whats going on there yet.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a diff in pull request #2663: Highlight rows with recovering tservers in the tserver status table
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2663:
URL: https://github.com/apache/accumulo/pull/2663#discussion_r861665318
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/tservers.js:
##########
@@ -161,8 +161,39 @@ function clearDeadTServers(server) {
}
/**
- * Generates the tserver table
+ * Generates the tserver table and highlights rows that have recovering tservers
*/
function refreshTServersTable() {
- if(tserversList) tserversList.ajax.reload(null, false ); // user paging is not reset on reload
+ if (tserversList) tserversList.ajax.reload(null, false); // user paging is not reset on reload
+
+ getRecoveryList().then(function () {
+
+ // reset background for all rows
+ const table = $('#tservers').DataTable();
+ table.rows().every(function (index) {
+ $(table.row(index).node()).css('background-color', '');
+ });
+
+ const recoveryList = []
+ JSON.parse(sessionStorage.recoveryList).recoveryList.forEach(entry => {
+ recoveryList.push(entry.server);
+ });
+
+ if (recoveryList.length === 0) {
+ $('#recovery-caption').text('');
Review Comment:
And then show it when there is recovery happening.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2663: Highlight rows with recovering tservers in the tserver status table
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2663:
URL: https://github.com/apache/accumulo/pull/2663#discussion_r861388097
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/tservers.js:
##########
@@ -161,8 +161,39 @@ function clearDeadTServers(server) {
}
/**
- * Generates the tserver table
+ * Generates the tserver table and highlights rows that have recovering tservers
*/
function refreshTServersTable() {
- if(tserversList) tserversList.ajax.reload(null, false ); // user paging is not reset on reload
+ if (tserversList) tserversList.ajax.reload(null, false); // user paging is not reset on reload
+
+ getRecoveryList().then(function () {
+
+ // reset background for all rows
+ const table = $('#tservers').DataTable();
+ table.rows().every(function (index) {
+ $(table.row(index).node()).css('background-color', '');
+ });
+
+ const recoveryList = []
+ JSON.parse(sessionStorage.recoveryList).recoveryList.forEach(entry => {
+ recoveryList.push(entry.server);
+ });
+
+ if (recoveryList.length === 0) {
+ $('#recovery-caption').text('');
+ return;
+ } else {
+ $('#recovery-caption').text('Highlighted rows represent recovering tservers');
+ }
+
+
+ console.log('List of recovering tservers to be highlighted: ' + recoveryList);
+
+ // highlight rows if thier tserver is recovering
Review Comment:
```suggestion
// highlight rows if their tserver is recovering
```
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/tservers.ftl:
##########
@@ -49,6 +49,7 @@
</thead>
<tbody></tbody>
</table>
+ <span id="recovery-caption" style="background-color: gold;"></span>
Review Comment:
Instead of using a custom inline style, would it be possible to use the built-in style from bootstrap? https://getbootstrap.com/docs/4.0/utilities/colors/#background-color
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2663: Highlight rows with recovering tservers in the tserver status table
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2663:
URL: https://github.com/apache/accumulo/pull/2663#discussion_r862017114
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/tservers.ftl:
##########
@@ -49,6 +49,7 @@
</thead>
<tbody></tbody>
</table>
+ <span id="recovery-caption" style="background-color: gold;"></span>
Review Comment:
I can't get this working for some reason. I was able to apply the class to the caption, but the datatable will not show the change and I'm not sure why.
Edit: For some odd reason the class only seems to apply to even rows, even if I remove the existing `odd` and `even` classes from them, so not too sure whats going on there yet.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2663: Highlight rows with recovering tservers in the tserver status table
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2663:
URL: https://github.com/apache/accumulo/pull/2663#discussion_r861788951
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/tservers.ftl:
##########
@@ -49,6 +49,7 @@
</thead>
<tbody></tbody>
</table>
+ <span id="recovery-caption" style="background-color: gold;"></span>
Review Comment:
Yea good idea ill change it to `.bg-warning`
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2663: Highlight rows with recovering tservers in the tserver status table
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2663:
URL: https://github.com/apache/accumulo/pull/2663#discussion_r862017311
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/tservers.js:
##########
@@ -161,8 +161,39 @@ function clearDeadTServers(server) {
}
/**
- * Generates the tserver table
+ * Generates the tserver table and highlights rows that have recovering tservers
*/
function refreshTServersTable() {
- if(tserversList) tserversList.ajax.reload(null, false ); // user paging is not reset on reload
+ if (tserversList) tserversList.ajax.reload(null, false); // user paging is not reset on reload
+
+ getRecoveryList().then(function () {
+
+ // reset background for all rows
+ const table = $('#tservers').DataTable();
+ table.rows().every(function (index) {
+ $(table.row(index).node()).css('background-color', '');
+ });
+
+ const recoveryList = []
+ JSON.parse(sessionStorage.recoveryList).recoveryList.forEach(entry => {
+ recoveryList.push(entry.server);
+ });
+
+ if (recoveryList.length === 0) {
+ $('#recovery-caption').text('');
Review Comment:
Addressed in [0ddc643](https://github.com/apache/accumulo/pull/2663/commits/0ddc643adcd20c1133da6c48e51e6c0a3729c298)
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2663: Highlight rows with recovering tservers in the tserver status table
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2663:
URL: https://github.com/apache/accumulo/pull/2663#discussion_r861790232
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/tservers.js:
##########
@@ -161,8 +161,39 @@ function clearDeadTServers(server) {
}
/**
- * Generates the tserver table
+ * Generates the tserver table and highlights rows that have recovering tservers
*/
function refreshTServersTable() {
- if(tserversList) tserversList.ajax.reload(null, false ); // user paging is not reset on reload
+ if (tserversList) tserversList.ajax.reload(null, false); // user paging is not reset on reload
+
+ getRecoveryList().then(function () {
+
+ // reset background for all rows
+ const table = $('#tservers').DataTable();
+ table.rows().every(function (index) {
+ $(table.row(index).node()).css('background-color', '');
+ });
+
+ const recoveryList = []
+ JSON.parse(sessionStorage.recoveryList).recoveryList.forEach(entry => {
+ recoveryList.push(entry.server);
+ });
+
+ if (recoveryList.length === 0) {
+ $('#recovery-caption').text('');
Review Comment:
Yea good idea. Maybe i can just leave the text within the span and then `.hide()` and `.show()` the span when needed instead of changing the text.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2663: Highlight rows with recovering tservers in the tserver status table
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2663:
URL: https://github.com/apache/accumulo/pull/2663#discussion_r865041622
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/tservers.ftl:
##########
@@ -49,6 +49,7 @@
</thead>
<tbody></tbody>
</table>
+ <span id="recovery-caption" style="background-color: gold;"></span>
Review Comment:
:shrug:
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on pull request #2663: Highlight rows with recovering tservers in the tserver status table
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on PR #2663:
URL: https://github.com/apache/accumulo/pull/2663#issuecomment-1113568941
I refactored the logic to highlight the rows quite a bit in [0ddc643](https://github.com/apache/accumulo/pull/2663/commits/0ddc643adcd20c1133da6c48e51e6c0a3729c298). It now resides in a "rowCallback" function as part of the DataTable constructor(?).
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a diff in pull request #2663: Highlight rows with recovering tservers in the tserver status table
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2663:
URL: https://github.com/apache/accumulo/pull/2663#discussion_r861663750
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/tservers.js:
##########
@@ -161,8 +161,39 @@ function clearDeadTServers(server) {
}
/**
- * Generates the tserver table
+ * Generates the tserver table and highlights rows that have recovering tservers
*/
function refreshTServersTable() {
- if(tserversList) tserversList.ajax.reload(null, false ); // user paging is not reset on reload
+ if (tserversList) tserversList.ajax.reload(null, false); // user paging is not reset on reload
+
+ getRecoveryList().then(function () {
+
+ // reset background for all rows
+ const table = $('#tservers').DataTable();
+ table.rows().every(function (index) {
+ $(table.row(index).node()).css('background-color', '');
+ });
+
+ const recoveryList = []
+ JSON.parse(sessionStorage.recoveryList).recoveryList.forEach(entry => {
+ recoveryList.push(entry.server);
+ });
+
+ if (recoveryList.length === 0) {
+ $('#recovery-caption').text('');
Review Comment:
You may need to hide the span as well if you don't want to see the blank space.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo merged pull request #2663: Highlight rows with recovering tservers in the tserver status table
Posted by GitBox <gi...@apache.org>.
DomGarguilo merged PR #2663:
URL: https://github.com/apache/accumulo/pull/2663
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2663: Highlight rows with recovering tservers in the tserver status table
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2663:
URL: https://github.com/apache/accumulo/pull/2663#issuecomment-1113803106
While I think this is a neat change. I do wonder if it's there isn't more to do here. For one, table's tablets are spread across a cluster. So, if one tserver dies, and its tablets go to other tservers, there's going to be a *lot* of yellow. That's probably fine... it's better than no insight whatsoever. Other things that might be useful is a count of how many tablets a tserver is hosting, vs. how many of those are waiting on recovery.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2663: Highlight rows with recovering tservers in the tserver status table
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2663:
URL: https://github.com/apache/accumulo/pull/2663#discussion_r862017114
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/tservers.ftl:
##########
@@ -49,6 +49,7 @@
</thead>
<tbody></tbody>
</table>
+ <span id="recovery-caption" style="background-color: gold;"></span>
Review Comment:
I can't get this working for some reason. I was able to apply the class to the caption, but the datatable will not show the change and I'm not sure why.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org