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

fix(pageserver): add the walreceiver state to tenant timeline GET api endpoint #5196

Merged
merged 8 commits into from
Sep 7, 2023
Merged

fix(pageserver): add the walreceiver state to tenant timeline GET api endpoint #5196

merged 8 commits into from
Sep 7, 2023

Conversation

duguorong009
Copy link
Contributor

@duguorong009 duguorong009 commented Sep 4, 2023

Add a walreceiver_state field to TimelineInfo (response of GET /v1/tenant/:tenant_id/timeline/:timeline_id) and while doing that, refactor out a common Timeline::walreceiver_state(..). No OpenAPI changes, because this is an internal debugging addition.

Fixes #3115.

Copy link
Member

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

Otherwise this is looking good but I see I have failed to update the issue description and only requested this in my follow-up comment much later.

Could you move it to be part of pageserver::http::routes::timeline_detail_handler or GET /v1/tenant/:tenant/timeline/:timeline instead of a new endpoint? In pratice with prod, timeline detail is what we most often get, that way it would be easily accessible for all "existing users".

@duguorong009 duguorong009 changed the title feat(pageserver): add the http GET api for tenant timeline walreceiver state fix(pageserver): add the walreceiver state to tenant timeline GET api endpoint Sep 6, 2023
@duguorong009
Copy link
Contributor Author

Otherwise this is looking good but I see I have failed to update the issue description and only requested this in my follow-up comment much later.

Could you move it to be part of pageserver::http::routes::timeline_detail_handler or GET /v1/tenant/:tenant/timeline/:timeline instead of a new endpoint? In pratice with prod, timeline detail is what we most often get, that way it would be easily accessible for all "existing users".

@koivunej
I updated the PR as you recommend. (Thanks for tips!)
Please re-check the PR, and give me the comments.

Copy link
Member

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

Yeah apologies on the openapi changes, which were good but as we'll need this for debugging, I don't think it is important to note this here.

Let's make the new methods pub(crate) and I'll fire up a workflow run for this. I can most likely merge those commits, let's try.

pageserver/src/tenant/timeline/walreceiver.rs Outdated Show resolved Hide resolved
pageserver/src/tenant/timeline.rs Outdated Show resolved Hide resolved
@koivunej
Copy link
Member

koivunej commented Sep 7, 2023

Thanks for working on this as well!

I added those pub(crate) and can fix up if these cause any issues. pub(crate) will make it visible (compiler warning) if these functions were to become unused later.

Hmm.. Looks like I don't have to make a separate workflow for this now that I've commited to it, which is nice. Nope, still need to create one, but will handle that after seeing that my suggestions still compile.

@koivunej koivunej added the approved-for-ci-run Changes are safe to trigger CI for the PR label Sep 7, 2023
@github-actions github-actions bot removed the approved-for-ci-run Changes are safe to trigger CI for the PR label Sep 7, 2023
@vipvap vipvap mentioned this pull request Sep 7, 2023
@duguorong009
Copy link
Contributor Author

Thanks for the fixture commit! @koivunej

BTW, can you check the issue #5131 and give some comments?
Or, at least, you let @problame know?
Thanks in advance!

@duguorong009
Copy link
Contributor Author

Thanks for working on this as well!

I added those pub(crate) and can fix up if these cause any issues. pub(crate) will make it visible (compiler warning) if these functions were to become unused later.

Hmm.. Looks like I don't have to make a separate workflow for this now that I've commited to it, which is nice. Nope, still need to create one, but will handle that after seeing that my suggestions still compile.

Yes, for me, the CI run is a bit inconvenient.
Also, I just do the cargo test for local testing of PR.

@koivunej
Copy link
Member

koivunej commented Sep 7, 2023

Yes, for me, the CI run is a bit inconvenient. Also, I just do the cargo test for local testing of PR.

It's a bit inconvenient for everyone, but especially for external contributors :) cargo test is fine with rust tests.

Python tests take quite some time and requrie a beefy cpu, preferably an isolated box. I usually run them with ./scripts/pytest -n16 -vv --tb=short -k 'debug-pg14' after a cargo build_testing, but ... for these rust refactorings there should be no need.

@koivunej koivunej merged commit 706977f into neondatabase:main Sep 7, 2023
46 of 54 checks passed
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.

Create a timeline subcommand to print its walreceiver state
2 participants