compute_ctl: Break up main() #7577

sharnoff · 2024-05-01T19:15:24Z

Problem

compute_ctl's main() is "pretty gnarly". This makes changes to control flow quite difficult.

I hit this while working on #7434 and had an idea of how it could be improved, thus this PR.

Summary of changes

First commit: Small changes to reduce the diff of the second commit.

Second commit: Breaks up main() into a handful of component functions, aligned with distinct sections of the existing control flow. Each new function gets the previous one's return values as its arguments. The new functions destructure their args so the names are all the same.

Third commit: Move the tracing startup context guard out of where it was previously used, to make things more simple. Taken from #7600.

Checklist before requesting a review

I have performed a self-review of my code.
If it is a core feature, I have added thorough tests.
Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

Do not forget to reformat commit message to not include the above checklist

github-actions · 2024-05-01T19:55:43Z

3072 tests run: 2939 passed, 0 failed, 133 skipped (full report)

Flaky tests (6)

Postgres 16

test_ancestor_detach_reparents_earlier[True]: release
test_vm_bit_clear_on_heap_lock: debug

Postgres 15

test_location_conf_churn[2]: debug
test_synthetic_size_while_deleting: debug
test_vm_bit_clear_on_heap_lock: debug

Postgres 14

test_detached_receives_flushes_while_being_detached[True]: debug

Code coverage* (full report)

functions: 31.2% (6258 of 20054 functions)
lines: 46.7% (46951 of 100591 lines)

* collected from Rust tests only

_{The comment gets automatically updated with the latest test results
7f45a29 at 2024-05-07T21:06:51.186Z :recycle:}

compute_tools/src/bin/compute_ctl.rs

- On a non-pooled start, do not reset the 'start_time' after launching the HTTP service. In a non-pooled start, it's fair to include that in the total startup time. - When setting wait_for_spec_ms and resetting start_time, call Utc::now() only once. It's a waste of cycles to call it twice, but also, it ensures the time between setting wait_for_spec_ms and resetting start_time is included in one or the other time period. These differences should be insignificant in practice, in the microsecond range, but IMHO it seems more logical and readable this way too. Also fix and clarify some of the surrounding comments. (This caught my eye while reviewing PR #7577)

sharnoff · 2024-05-07T00:59:34Z

Planning to merge in 24h or so, if no objections (cc @tristan957).

A couple lines moved further down in main(), and one case of using Option<&str> instead of Option<&String>.

This commit is intentionally designed to have as small a diff as possible. To that end, the basic idea is that each distinct "chunk" of the previous main() has been wrapped in its own function, with the return values from each function being passed directly into the next. The structure of main() is now visible from its contents, which have a handful of smaller functions. There's a lot of other work that can / should(?) be done beyond this, but I figure that's more opinionated, and this should be a solid start. Co-authored-by: Heikki Linnakangas <heikki@neon.tech>

Part of applying the changes from #7600. This piece *technically* can change the semantics because now the context guard is held before process_cli, but... the difference is likely quite small. Co-authored-by: Heikki Linnakangas <heikki@neon.tech>

A couple lines moved further down in main(), and one case of using Option<&str> instead of Option<&String>.

This commit is intentionally designed to have as small a diff as possible. To that end, the basic idea is that each distinct "chunk" of the previous main() has been wrapped in its own function, with the return values from each function being passed directly into the next. The structure of main() is now visible from its contents, which have a handful of smaller functions. There's a lot of other work that can / should(?) be done beyond this, but I figure that's more opinionated, and this should be a solid start. Co-authored-by: Heikki Linnakangas <heikki@neon.tech>

Introduced by refactorings from #7577. See an example check-macos-build failure here: https://github.com/neondatabase/neon/actions/runs/8992211409/job/24701531264

- On a non-pooled start, do not reset the 'start_time' after launching the HTTP service. In a non-pooled start, it's fair to include that in the total startup time. - When setting wait_for_spec_ms and resetting start_time, call Utc::now() only once. It's a waste of cycles to call it twice, but also, it ensures the time between setting wait_for_spec_ms and resetting start_time is included in one or the other time period. These differences should be insignificant in practice, in the microsecond range, but IMHO it seems more logical and readable this way too. Also fix and clarify some of the surrounding comments. (This caught my eye while reviewing PR #7577)

A couple lines moved further down in main(), and one case of using Option<&str> instead of Option<&String>.

This commit is intentionally designed to have as small a diff as possible. To that end, the basic idea is that each distinct "chunk" of the previous main() has been wrapped in its own function, with the return values from each function being passed directly into the next. The structure of main() is now visible from its contents, which have a handful of smaller functions. There's a lot of other work that can / should(?) be done beyond this, but I figure that's more opinionated, and this should be a solid start. Co-authored-by: Heikki Linnakangas <heikki@neon.tech>

Part of applying the changes from #7600. This piece *technically* can change the semantics because now the context guard is held before process_cli, but... the difference is likely quite small. Co-authored-by: Heikki Linnakangas <heikki@neon.tech>

Introduced by refactorings from #7577. See an example check-macos-build failure here: https://github.com/neondatabase/neon/actions/runs/8992211409/job/24701531264

sharnoff requested a review from tristan957 May 1, 2024 19:15

sharnoff requested review from a team as code owners May 1, 2024 19:15

sharnoff requested a review from mtyazici May 1, 2024 19:15

tristan957 reviewed May 1, 2024

View reviewed changes

compute_tools/src/bin/compute_ctl.rs Show resolved Hide resolved

compute_tools/src/bin/compute_ctl.rs Outdated Show resolved Hide resolved

compute_tools/src/bin/compute_ctl.rs Outdated Show resolved Hide resolved

compute_tools/src/bin/compute_ctl.rs Outdated Show resolved Hide resolved

This was referenced May 3, 2024

Compute ctl main refactor startup context #7600

Closed

compute_ctl: Be more precise in how startup time is calculated #7601

Merged

sharnoff force-pushed the sharnoff/compute_ctl-main()-phase-refactor branch from ffe65ef to 10a0754 Compare May 4, 2024 01:26

hlinnaka approved these changes May 4, 2024

View reviewed changes

sharnoff force-pushed the sharnoff/compute_ctl-main()-phase-refactor branch from 10a0754 to 855679e Compare May 4, 2024 16:22

sharnoff changed the title ~~compute_ctl: Break up main() into discrete phases~~ compute_ctl: Break up main() May 4, 2024

tristan957 approved these changes May 7, 2024

View reviewed changes

sharnoff and others added 3 commits May 7, 2024 12:32

compute_ctl: Non-functional prep changes to reduce diff (#7577)

6f23ba6

A couple lines moved further down in main(), and one case of using Option<&str> instead of Option<&String>.

sharnoff force-pushed the sharnoff/compute_ctl-main()-phase-refactor branch from 855679e to 7f45a29 Compare May 7, 2024 19:32

sharnoff enabled auto-merge (rebase) May 7, 2024 20:50

sharnoff merged commit 26b1483 into main May 7, 2024
53 checks passed

sharnoff deleted the sharnoff/compute_ctl-main()-phase-refactor branch May 7, 2024 20:58

sharnoff added a commit that referenced this pull request May 7, 2024

compute_ctl: Non-functional prep changes to reduce diff (#7577)

b158a5e

A couple lines moved further down in main(), and one case of using Option<&str> instead of Option<&String>.

sharnoff added a commit that referenced this pull request May 7, 2024

compute_ctl: Fix unused variable on non-Linux

766f0da

Introduced by refactorings from #7577. See an example check-macos-build failure here: https://github.com/neondatabase/neon/actions/runs/8992211409/job/24701531264

sharnoff mentioned this pull request May 7, 2024

compute_ctl: Fix unused variable on non-Linux #7646

Merged

5 tasks

conradludgate pushed a commit that referenced this pull request May 8, 2024

compute_ctl: Non-functional prep changes to reduce diff (#7577)

7a1fbcc

A couple lines moved further down in main(), and one case of using Option<&str> instead of Option<&String>.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compute_ctl: Break up main() #7577

compute_ctl: Break up main() #7577

sharnoff commented May 1, 2024 •

edited

github-actions bot commented May 1, 2024 •

edited

Postgres 16

Postgres 15

Postgres 14

sharnoff commented May 7, 2024

compute_ctl: Break up main() #7577

compute_ctl: Break up main() #7577

Conversation

sharnoff commented May 1, 2024 • edited

Problem

Summary of changes

Checklist before requesting a review

Checklist before merging

github-actions bot commented May 1, 2024 • edited

3072 tests run: 2939 passed, 0 failed, 133 skipped (full report)

Postgres 16

Postgres 15

Postgres 14

Code coverage* (full report)

sharnoff commented May 7, 2024

sharnoff commented May 1, 2024 •

edited

github-actions bot commented May 1, 2024 •

edited