Skip to content

Support to show leader of coordinators in Services web console view#10418

Closed
FrankChen021 wants to merge 3 commits intoapache:masterfrom
FrankChen021:show_leader
Closed

Support to show leader of coordinators in Services web console view#10418
FrankChen021 wants to merge 3 commits intoapache:masterfrom
FrankChen021:show_leader

Conversation

@FrankChen021
Copy link
Copy Markdown
Member

This PR resolves #10329, which was proposed to show which coordinator node is the current leader on web console so that it saves us some time to find leader when addressing problems of druid clusters.

Description

In this PR, only backend service is involved to meet this need.

The ServersTable now requests leader information of coordinators and overlords and marks corresponding node as leader in the Tier field before returning the query results.

With this PR, Web console automatically shows the leader information in the Service view
image


This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@vogievetsky
Copy link
Copy Markdown
Contributor

If this is modifying the behaviour of the system table I think it would be better to surface leader as an 'is_leader' column (and then the console can format it as it wants by asking the right query). No need to overload the tier column I think. More columns = better!

@FrankChen021
Copy link
Copy Markdown
Member Author

Hi @vogievetsky What you suggestion depends on how we define tier. The initial tier comes from historical nodes that serve different data retention rules.

From the user's view on all druid nodes, I think tier could also used to describe different roles of nodes with same node type, which means for coordinators, the division of leader and non-leader also fit into this definition. And for the back-end implementation, this PR chooses to put the information in tier field for simplicity.

BTW, I'll check and fix the integration tests failure later.

@stale
Copy link
Copy Markdown

stale Bot commented Dec 20, 2020

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

@stale stale Bot added the stale label Dec 20, 2020
@jihoonson jihoonson removed the stale label Jan 5, 2021
@jihoonson
Copy link
Copy Markdown
Contributor

Oops, sorry @FrankChen021 I missed this PR before. It seems like the same functionality is implemented in #10680. Could you check if that's the case?

@clintropolis
Copy link
Copy Markdown
Member

Oops, sorry @FrankChen021 I missed this PR before. It seems like the same functionality is implemented in #10680. Could you check if that's the case?

My bad, I totally missed this as well, I think this functionality should be covered by the thing I did by side-effect in my PR, sorry @FrankChen021!

@FrankChen021
Copy link
Copy Markdown
Member Author

@jihoonson @clintropolis Thank you for your information. Changes in this PR has been covered by the PR you mentioned above.

The difference is that the leader indicator in this PR is returned in tier column which means, for the web console, there's no work to be done to display the leader information.

So, based on #10680, we still have to do some work on the web console side. I wonder what's your suggestion for the web console to show the leader indicator, is the indicator displayed in a new column or displayed in tier column ?

@clintropolis
Copy link
Copy Markdown
Member

So, based on #10680, we still have to do some work on the web console side. I wonder what's your suggestion for the web console to show the leader indicator, is the indicator displayed in a new column or displayed in tier column ?

Ah that is a good point, I didn't consider the 'services' tab of the web-console, currently you can only determine leadership after #10680 by querying sys.servers directly in the query tab, so it doesn't really resolve #10329 without additional work to the web-console. I'm not sure what would be the best UX to indicate which servers are the leaders on the 'services' tab, I guess the most straightforward way would be to just add the column, but I would imagine there are alternative ways this could be done? I think re-using 'tier' is sort of confusing, which is why I ended up just added a new column to the underlying table. @vogievetsky any ideas?

@FrankChen021
Copy link
Copy Markdown
Member Author

Considering that leader is only available for overlord and coordinator nodes, and the number of these two nodes are far less than that of other kind of nodes, adding another new column on the 'Services' tab would leave the most of cells blank, which I think is not a good UX.

I wonder if we could could change the column name 'Tier' on the 'Services' tab to another name(I have not come up with an appropriate name), so that it could hold not only the tier information of historical nodes, but also other information from different kind of nodes. @clintropolis @vogievetsky

@clintropolis
Copy link
Copy Markdown
Member

Considering that leader is only available for overlord and coordinator nodes, and the number of these two nodes are far less than that of other kind of nodes, adding another new column on the 'Services' tab would leave the most of cells blank, which I think is not a good UX.

I wonder if we could could change the column name 'Tier' on the 'Services' tab to another name(I have not come up with an appropriate name), so that it could hold not only the tier information of historical nodes, but also other information from different kind of nodes. @clintropolis @vogievetsky

What about the detail column? I think that might be more appropriate than reworking the tier column, unless there is something better I'm not thinking of...

@vogievetsky
Copy link
Copy Markdown
Contributor

I think replacing the tier column with detail and having it show both pieces of info works. I think having another Is leader column would work also. I am 👍 to both ideas

@FrankChen021
Copy link
Copy Markdown
Member Author

I think we can reach an agreement that 'Tier' should be replaced by 'Detail' on 'Service' tab on web console to show tier information of historical nodes and leader of coordinators. @clintropolis @vogievetsky

I'll make some changes to this PR later this week.

@clintropolis
Copy link
Copy Markdown
Member

I think we can reach an agreement that 'Tier' should be replaced by 'Detail' on 'Service' tab on web console to show tier information of historical nodes and leader of coordinators. @clintropolis @vogievetsky

I think details is already on the 'service' tab; for historicals it contains load/drop queue information, and most recently completed task information for indexing stuffs. I don't see any reason to remove the tier and fold it into details along with the other stuff, lots of servers probably have that column populated, I was just suggesting that coordinators and overlords could make use of details instead of it being an empty column

@FrankChen021
Copy link
Copy Markdown
Member Author

I think details is already on the 'service' tab; for historicals it contains load/drop queue information, and most recently completed task information for indexing stuffs. I don't see any reason to remove the tier and fold it into details along with the other stuff, lots of servers probably have that column populated, I was just suggesting that coordinators and overlords could make use of details instead of it being an empty column

Sorry, I misunderstood it.

@FrankChen021
Copy link
Copy Markdown
Member Author

This PR is replaced by 10951

@FrankChen021 FrankChen021 deleted the show_leader branch March 9, 2021 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support to show leader coordinator in Services web console view

4 participants