Skip to content

Move server navigation out of navbar in monitor#3561

Merged
ctubbsii merged 3 commits intoapache:mainfrom
DomGarguilo:separateServerDropdown
Aug 14, 2023
Merged

Move server navigation out of navbar in monitor#3561
ctubbsii merged 3 commits intoapache:mainfrom
DomGarguilo:separateServerDropdown

Conversation

@DomGarguilo
Copy link
Copy Markdown
Member

fixes #3548

This PR...

  • moves the 'manager server', 'tablet servers' and 'garbage collector' links out of the 'servers' dropdown in the navbar and into their own separate links
  • makes it so that when the manager state or goal state is SAFE_MODE the manager notification circle is yellow and when the manager state or goal state is CLEAN_STOP it is red

Here are some screenshots with various manager states:
Screenshot from 2023-06-30 15-00-42

Screenshot from 2023-06-30 15-00-59

Screenshot from 2023-06-30 15-01-18

Screenshot from 2023-06-30 15-01-24

Screenshot from 2023-06-30 15-01-30

@DomGarguilo DomGarguilo requested a review from ivakegg June 30, 2023 19:07
@DomGarguilo DomGarguilo self-assigned this Jun 30, 2023
Copy link
Copy Markdown
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • moves the 'manager server', 'tablet servers' and 'garbage collector' links out of the 'servers' dropdown in the navbar and into their own separate links

-1 to that part, as per my comments on #3548 (comment) and #3548 (comment)

  • makes it so that when the manager state or goal state is SAFE_MODE the manager notification circle is yellow and when the manager state or goal state is CLEAN_STOP it is red

+1 to that part of the change.

I wouldn't mind so much expanding the menu if it didn't take up so much space and add so much clutter to the top bar. But, even if we could find a way to make them take up less space, it's not scalable... we're adding a bunch of different server types (scan servers, compactors, etc.) and there just isn't enough space up there to separate them out. And it doesn't make sense to separate out some server types and leave others collapsed. So, I think it just needs to stay collapsed, and we need to use some other distinct visual element that propagates up to indicate this special circumstance. (Alternatively, users can just monitor the REST endpoint in any monitoring or alerting tool they wish, rather than rely on our monitor's look and feel to present that information.)

@EdColeman
Copy link
Copy Markdown
Contributor

Is this approach going to work if there are multiple managers? That's not the case now, but asking if the changes would be able to accommodate more that one manager? The answer might be that with multiple managers, a whole lot of other things will need to change, so it does not matter. But, would be desirable if it could be accommodated, or at least provide a path towards multiple managers rather than being another road block.

@ctubbsii
Copy link
Copy Markdown
Member

ctubbsii commented Jun 30, 2023

Is this approach going to work if there are multiple managers?

I'm not sure how safe mode would work with multiple managers, but assuming they each could be in safe mode, then visually on the monitor, the main change would be to add an "s" to make it plural and the dot color or other visual element could indicate that any one of the managers are in safe mode, just like things are rolled up to the single "Tablet Servers" item. So, I don't think it really affects this at all right now.

@DomGarguilo
Copy link
Copy Markdown
Member Author

In 589787c I returned the navbar servers dropdown to how it was. I also made it do that servers dropdown notification takes into account the manager state and goal state when its color is set. I also did quite a bit of refactoring to break things down into smaller methods and create constants for frequently repeated variable names.

Copy link
Copy Markdown
Contributor

@Manno15 Manno15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These latest changes look good.

@ctubbsii
Copy link
Copy Markdown
Member

@DomGarguilo can you show some updated screenshots for the recent changes, when you get a chance?

@DomGarguilo
Copy link
Copy Markdown
Member Author

DomGarguilo commented Jul 11, 2023

Here are some updated screenshots. Let me know if there are any other specific cases that you might want to see.

setting the manager goal state to SAFE_MODE:
Screenshot from 2023-07-11 10-03-58

setting the manager goal state to CLEAN_STOP:
Screenshot from 2023-07-11 10-04-39

@ctubbsii
Copy link
Copy Markdown
Member

ctubbsii commented Aug 2, 2023

Here are some updated screenshots. Let me know if there are any other specific cases that you might want to see.

@DomGarguilo Thanks for that. I personally like this, but @ivakegg said that it could go ignored if it was just a yellow dot in the rolled up version, because it's common for the Servers dot to be yellow due to some TServers being offline, rather than anything to do with the Manager. So, that's why I had proposed using a unique visual element that got rolled up when the Manager is in safe mode, so it's distinguishable from the common situation where it's merely yellow because a few tservers are offline. Yellow is still good to use, but instead of a dot, it could be a different icon.

@ctubbsii
Copy link
Copy Markdown
Member

@DomGarguilo I'm going to go ahead and include this in 2.1.2, because I think it gets us closer to what we want. But I will create a new issue for subsequent work to create a distinct visual indicator for the rollup when the manager's indicator is yellow vs. the tservers' being yellow, so you don't have to expand the menu to notice the manager is yellow.

@ctubbsii ctubbsii merged commit 12898a3 into apache:main Aug 14, 2023
asfgit pushed a commit that referenced this pull request Aug 14, 2023
When the manager is in safe mode, make its icon in the server nav menu
yellow, and roll that yellow indicator up into the top bar.
@ctubbsii
Copy link
Copy Markdown
Member

Didn't realize the PR was against main before I merged this, so I needed to cherry-pick it to 2.1 to backport it, in commit 313238b

@DomGarguilo
Copy link
Copy Markdown
Member Author

@DomGarguilo I'm going to go ahead and include this in 2.1.2, because I think it gets us closer to what we want. But I will create a new issue for subsequent work to create a distinct visual indicator for the rollup when the manager's indicator is yellow vs. the tservers' being yellow, so you don't have to expand the menu to notice the manager is yellow.

Okay sounds good. I'm working on some changes on this branch to get a different icon to display for when the manager is in a non-normal state. It's taking longer than I originally thought it would but I'm getting closer and will make a new PR when its ready.

@ctubbsii
Copy link
Copy Markdown
Member

The follow-on ticket is #3688

@DomGarguilo DomGarguilo deleted the separateServerDropdown branch March 14, 2024 15:24
@ctubbsii ctubbsii added this to the 2.1.2 milestone Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Accumulo goal state not visible on all pages

4 participants