You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "ctubbsii (via GitHub)" <gi...@apache.org> on 2023/05/10 11:14:12 UTC
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3389: Show manager goal state in monitor
ctubbsii commented on code in PR #3389:
URL: https://github.com/apache/accumulo/pull/3389#discussion_r1189720734
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/manager.js:
##########
@@ -25,29 +25,52 @@
var managerStatusTable, recoveryListTable;
-function refreshManagerBanner() {
+function refreshManagerBanners() {
getStatus().then(function () {
- var managerStatus = JSON.parse(sessionStorage.status).managerStatus;
+ const managerStatus = JSON.parse(sessionStorage.status).managerStatus;
// If manager status is error
if (managerStatus === 'ERROR') {
- // show banner and hide table
- $('#managerBanner').show();
+ // show the manager error banner and hide table
+ $('#managerRunningBanner').show();
$('#managerStatus_wrapper').hide();
} else {
- // otherwise, hide banner and show table
- $('#managerBanner').hide();
+ // otherwise, hide the error banner and show manager table
+ $('#managerRunningBanner').hide();
$('#managerStatus_wrapper').show();
}
});
+
+ getManager().then(function () {
+ const managerData = JSON.parse(sessionStorage.manager);
+ const managerState = managerData.managerState;
+ const managerGoalState = managerData.managerGoalState;
+
+ const isStateGoalDifferent = managerState !== managerGoalState;
Review Comment:
I think some of the logic might read better if there were fewer negations.
```suggestion
const goalStateIsCurrent = managerState == managerGoalState;
```
Also, what happens if these are the same, but both are null?
What happens if `getManager()` errors?
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/manager.js:
##########
@@ -25,29 +25,52 @@
var managerStatusTable, recoveryListTable;
-function refreshManagerBanner() {
+function refreshManagerBanners() {
getStatus().then(function () {
- var managerStatus = JSON.parse(sessionStorage.status).managerStatus;
+ const managerStatus = JSON.parse(sessionStorage.status).managerStatus;
// If manager status is error
if (managerStatus === 'ERROR') {
- // show banner and hide table
- $('#managerBanner').show();
+ // show the manager error banner and hide table
+ $('#managerRunningBanner').show();
$('#managerStatus_wrapper').hide();
} else {
- // otherwise, hide banner and show table
- $('#managerBanner').hide();
+ // otherwise, hide the error banner and show manager table
+ $('#managerRunningBanner').hide();
$('#managerStatus_wrapper').show();
}
});
+
+ getManager().then(function () {
+ const managerData = JSON.parse(sessionStorage.manager);
+ const managerState = managerData.managerState;
+ const managerGoalState = managerData.managerGoalState;
+
+ const isStateGoalDifferent = managerState !== managerGoalState;
+
+ // if the manager state is normal and the goal is the same as the state or manager is not running, return early
+ if ((managerState === 'NORMAL' && !isStateGoalDifferent) || managerState === null) {
+ $('#managerStateBanner').hide();
+ return;
+ }
+
+ // update the manager state banner message and show it
+ let bannerMessage = 'Manager state: ' + managerState;
+ if (isStateGoalDifferent) {
+ bannerMessage += '. Manager goal state: ' + managerGoalState;
+ }
+ $('#managerStateBanner .alert.alert-warning').text(bannerMessage);
Review Comment:
It seems weird to query the inner div by the alert class labels on it. Why not just give the inner-div a name? Or just select the child element?
For example, either of these should work:
(select all `<div>` elements inside the element named `#managerStateBanner`)
```suggestion
$('#managerStateBanner div').text(bannerMessage);
```
(select all `<div>` elements that are one level inside the element named `#managerStateBanner`)
```suggestion
$('#managerStateBanner > div').text(bannerMessage);
```
Using the classes to select seems fragile. If we change the presentation style in the template, we'll have to change the classes in multiple places (will have to update the JavaScript to match). Better to give a specific name, or navigate using structural elements, rather than depend on class styles.
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/manager.js:
##########
@@ -25,29 +25,52 @@
var managerStatusTable, recoveryListTable;
-function refreshManagerBanner() {
+function refreshManagerBanners() {
getStatus().then(function () {
- var managerStatus = JSON.parse(sessionStorage.status).managerStatus;
+ const managerStatus = JSON.parse(sessionStorage.status).managerStatus;
// If manager status is error
if (managerStatus === 'ERROR') {
- // show banner and hide table
- $('#managerBanner').show();
+ // show the manager error banner and hide table
+ $('#managerRunningBanner').show();
$('#managerStatus_wrapper').hide();
} else {
- // otherwise, hide banner and show table
- $('#managerBanner').hide();
+ // otherwise, hide the error banner and show manager table
+ $('#managerRunningBanner').hide();
$('#managerStatus_wrapper').show();
}
});
+
+ getManager().then(function () {
+ const managerData = JSON.parse(sessionStorage.manager);
+ const managerState = managerData.managerState;
+ const managerGoalState = managerData.managerGoalState;
+
+ const isStateGoalDifferent = managerState !== managerGoalState;
+
+ // if the manager state is normal and the goal is the same as the state or manager is not running, return early
+ if ((managerState === 'NORMAL' && !isStateGoalDifferent) || managerState === null) {
Review Comment:
Is `managerState` always `null` when the `managerStatus` is `ERROR`, like in the previous block? It seems like that's not always the case.
##########
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/manager.js:
##########
@@ -25,29 +25,52 @@
var managerStatusTable, recoveryListTable;
-function refreshManagerBanner() {
+function refreshManagerBanners() {
getStatus().then(function () {
- var managerStatus = JSON.parse(sessionStorage.status).managerStatus;
+ const managerStatus = JSON.parse(sessionStorage.status).managerStatus;
// If manager status is error
if (managerStatus === 'ERROR') {
- // show banner and hide table
- $('#managerBanner').show();
+ // show the manager error banner and hide table
+ $('#managerRunningBanner').show();
$('#managerStatus_wrapper').hide();
Review Comment:
Is `#managerStatus_wrapper` still valid? I can't find any element named that. Looking through the code for `_wrapper`, I'm wondering if this is a DataTables-specific thing that gets auto-created by DataTables.
--
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