Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[admin] Get leaders info request #4259

Merged
merged 6 commits into from
Apr 14, 2022
Merged

Conversation

VadimPlh
Copy link
Contributor

@VadimPlh VadimPlh commented Apr 12, 2022

Cover letter

This debug API to understand what current node thinks about leaders.

Because it is debug request we only execute it on one core, without merging ans.

"path": "/v1/debug/partition_leaders_table",
            "operations": [
                {
                    "method": "GET",
                    "summary": "Get information about leaders for node",

Release notes

  • New admin api request /v1/debug/partition_leaders_table

@VadimPlh VadimPlh force-pushed the get-leaders-info branch 3 times, most recently from 5d5f4d8 to 39a8be1 Compare April 12, 2022 13:26
@VadimPlh VadimPlh marked this pull request as ready for review April 12, 2022 13:27
@VadimPlh VadimPlh requested a review from jcsp April 12, 2022 16:46
Copy link
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

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

everything looks good, but I think it's worth discussing if this needs to go under the /debug path. this information is largely available already via Kafka's MetadataRequest so it is effectively already not merely debug-level information and could live at a more first-class path (if it isn't all already available via Kafka API).

just as a simple example (we don't need to do this exactly), GET /v1/partitions returns nearly the exact set of information for local partitions, but could be updated to return everything for GET /v1/partitions?query=all or GET /v1/cluster/partitions.

wdyt?

EDIT: I was thinking that Kafka's MetadataRequest didn't return the leader info for an all-topics request but @mmaslankaprv showed me where that's happening. That request uses the leader table info. So it may be sufficient for this case.

@jcsp
Copy link
Contributor

jcsp commented Apr 13, 2022

I think there's value in having a debug endpoint that is specifically peeking at a particular data structure, asking "who does the leaders table think is leader?" rather than the more generic "who is leader?" that might be answered different ways, such as asking the local raft groups. In that context, maybe the endpoint should be /v1/debug/partition_leaders_table to reflect that it's really more of a direct data structure dump

@VadimPlh
Copy link
Contributor Author

In that context, maybe the endpoint should be /v1/debug/partition_leaders_table to reflect that it's really more of a direct data structure dump

I like it. It is more clearly what will be returned

@VadimPlh
Copy link
Contributor Author

everything looks good, but I think it's worth discussing if this needs to go under the /debug

I think it should be in /debug path.
It contains some internal info, as I understand. User should use kafka's MetadataRequestto get info about leaders IMO.

@dotnwat
Copy link
Member

dotnwat commented Apr 13, 2022

It contains some internal info

This is true of many admin APIs. Maybe it's worth while describing some of the options for discussion on slack? The only reason I'm paying this so much attention (I mean, /debug really is fine, too) is that we will always be adding admin APIs. And if we don't have some sort of design approach to the API then it's going to become messy.

EDIT:

As @jcsp says I think there's value in having a debug endpoint that is specifically peeking at a particular data structure, asking "who does the leaders table think is leader?" I think that is a reasonable way to characterize our approach to /debug. Although, I think it will be a fine line in some ways. For example /v1/partitions is primarily a dump of the partition table. But fair point that /debug is much less governed by nice-ness of API and geared more towards raw internal state even if a 'nice' API also is :)

Copy link
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

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

lgtm. there is a field that is set twice in the admin server but other than that 👍

src/v/cluster/partition_leaders_table.h Outdated Show resolved Hide resolved
src/v/redpanda/admin_server.cc Outdated Show resolved Hide resolved
@VadimPlh VadimPlh merged commit 09155a8 into redpanda-data:dev Apr 14, 2022
@VadimPlh VadimPlh linked an issue Apr 16, 2022 that may be closed by this pull request
Comment on lines +2562 to +2564
info.previous_leader = leader_info.previous_leader.has_value()
? leader_info.previous_leader.value()
: -1;
Copy link
Member

Choose a reason for hiding this comment

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

@VadimPlh what is previous leader and why is it exposed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is from partition_leaders_table::leader_meta:

        // previous leader id, this is empty if and only if there were no leader
        // elected for the topic before

Do we have problems with this?

Copy link
Member

Choose a reason for hiding this comment

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

no it's ok, just surprising that we track it and that we want to expose it through http. thanks for explaining.

This pull request was closed.
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.

admin_api: Added reset/get leaders_table requests
4 participants