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

Move compute_ctl structs used in HTTP API and spec file to separate crate. #3937

Merged
merged 1 commit into from
Apr 9, 2023

Conversation

hlinnaka
Copy link
Contributor

@hlinnaka hlinnaka commented Apr 3, 2023

This is in preparation of using compute_ctl to launch postgres nodes in the neon_local control plane. And seems like a good idea to separate the public interfaces anyway.

One non-mechanical change here is that we now use a RwLock rather than atomics to protect the ComputeNode::metrics field. We were not using atomics for performance but for convenience here, and an RwLock is now more convenient.

(extracted to separate PR from PR #3886)

@hlinnaka hlinnaka requested a review from a team as a code owner April 3, 2023 13:54
@hlinnaka hlinnaka requested review from ololobus, lubennikovaav and LizardWizzard and removed request for a team April 3, 2023 13:54
Copy link
Member

@ololobus ololobus left a comment

Choose a reason for hiding this comment

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

Moving structures in a separate crate sounds as good first step. Also moving metrics under the same lock should be fine too, as actually they are set now only during initial startup, so it's basically the same writer all the time

Yet, I made state to be under Mutex in the #3923 because of two reasons:

  1. Condvar was super convenient for my use case
  2. Write lock in the RwLock anyway blocks all readers, IIUC. And we don't have a case when there are a lot of concurrent readers

What do you think?

We should also agree on the order of PRs, probably :) Yours seems to be important to write better tests in the neon repo. Mine is needed to unblock control-plane work on reconfiguration without restart and maintaining the pool of pre-created computes. So not sure, just your refactoring looks relatively huge in general (if we consider all needed PRs)

@hlinnaka hlinnaka force-pushed the move-compute-spec branch from b410af5 to 8e06018 Compare April 5, 2023 16:49
@github-actions
Copy link

github-actions bot commented Apr 5, 2023

Test results for f0b2e07:


debug build: 212 tests run: 202 passed, 0 failed, 10 (full report)


release build: 212 tests run: 202 passed, 0 failed, 10 (full report)


@hlinnaka
Copy link
Contributor Author

hlinnaka commented Apr 5, 2023

Moving structures in a separate crate sounds as good first step. Also moving metrics under the same lock should be fine too, as actually they are set now only during initial startup, so it's basically the same writer all the time

Yet, I made state to be under Mutex in the #3923 because of two reasons:

1. Condvar was super convenient for my use case

2. Write lock in the `RwLock` anyway blocks all readers, IIUC. And we don't have a case when there are a lot of concurrent readers

What do you think?

Sure, Mutex is fine.

We should also agree on the order of PRs, probably :) Yours seems to be important to write better tests in the neon repo. Mine is needed to unblock control-plane work on reconfiguration without restart and maintaining the pool of pre-created computes. So not sure, just your refactoring looks relatively huge in general (if we consider all needed PRs)

First to push wins ;-).

I suggest:

  1. Move compute_ctl structs used in HTTP API and spec file to separate crate. #3937
  2. Rename "Postgres nodes" in control_plane to endpoints. #3938
  3. Implement live reconfiguration in the compute_ctl #3923

I can rebase #3923 over those two, I think the conflicts are pretty mechanical

@hlinnaka hlinnaka force-pushed the move-compute-spec branch from 8e06018 to 939ea29 Compare April 6, 2023 20:25
@hlinnaka
Copy link
Contributor Author

hlinnaka commented Apr 6, 2023

Rebased, fixing conflicts with commit e42982f.

I think this is ready to be merged. Please review and approve if you agree.

Copy link
Member

@ololobus ololobus left a comment

Choose a reason for hiding this comment

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

LGTM, one comment / suggestion about files layout and this text

One non-mechanical change here is that we now use a RwLock rather than atomics to protect the ComputeNode::metrics field. We were not using atomics for performance but for convenience here, and an RwLock is now more convenient.

should be updated as we use Mutex now.

libs/compute_api/src/models.rs Outdated Show resolved Hide resolved
@hlinnaka hlinnaka force-pushed the move-compute-spec branch from 939ea29 to 1f2df18 Compare April 9, 2023 18:49
…rate.

This is in preparation of using compute_ctl to launch postgres nodes
in the neon_local control plane. And seems like a good idea to
separate the public interfaces anyway.

One non-mechanical change here is that the 'metrics' field is moved
under the Mutex, instead of using atomics. We were not using atomics
for performance but for convenience here, and it seems more clear to
not use atomics in the model for the HTTP response type.
@hlinnaka hlinnaka force-pushed the move-compute-spec branch from 1f2df18 to f0b2e07 Compare April 9, 2023 18:54
@hlinnaka hlinnaka merged commit f0b2e07 into main Apr 9, 2023
@hlinnaka hlinnaka deleted the move-compute-spec branch April 9, 2023 19:12
@github-actions github-actions bot temporarily deployed to dev-eu-west-1 April 9, 2023 19:33 Inactive
@github-actions github-actions bot temporarily deployed to dev-us-east-2 April 9, 2023 19:33 Inactive
@github-actions github-actions bot temporarily deployed to dev-us-east-2 April 9, 2023 19:33 Inactive
@github-actions github-actions bot temporarily deployed to dev-eu-west-1 April 9, 2023 19:33 Inactive
@github-actions github-actions bot temporarily deployed to dev-us-east-2 April 9, 2023 19:33 Inactive
@github-actions github-actions bot temporarily deployed to dev-eu-west-1 April 9, 2023 19:33 Inactive
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.

2 participants