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

compute <-> sk protocol v3 #10647

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

compute <-> sk protocol v3 #10647

wants to merge 20 commits into from

Conversation

arssher
Copy link
Contributor

@arssher arssher commented Feb 4, 2025

Problem

As part of #8614 we need to pass membership configurations between compute and safekeepers.

Summary of changes

Add version 3 of the protocol carrying membership configurations. Greeting message in both sides gets full conf, and other messages generation number only. Use protocol bump to include other accumulated changes:

  • stop packing whole structs on the wire as is;
  • make the tag u8 instead of u64;
  • send all ints in network order;
  • drop proposer_uuid, we can pass it in START_WAL_PUSH and it wasn't much useful anyway.
    Per message changes, apart from mconf:
  • ProposerGreeting: tenant / timeline id is sent now as hex cstring. Remove proto version, it is passed outside in START_WAL_PUSH. Remove postgres timeline, it is unused. Reorder fields a bit.
  • AcceptorGreeting: reorder fields
  • VoteResponse: timeline_start_lsn is removed. It can be taken from first member of term history, and later we won't need it at all when all timelines will be explicitly created. Vote itself is u8 instead of u64.
  • ProposerElected: timeline_start_lsn is removed for the same reasons.
  • AppendRequest: epoch_start_lsn removed, it is known from term history in ProposerElected.

Both compute and sk are able to talk v2 and v3 to make rollbacks (in case we need them) easier; neon.safekeeper_proto_version GUC sets the client version. v2 code can be dropped later.

So far empty conf is passed everywhere, future PRs will handle them.

To test, add param to some tests choosing proto version; we want to test both 2 and 3 until we fully migrate.

ref #10326

@arssher arssher requested a review from arpad-m February 4, 2025 09:52
@arssher arssher requested review from a team as code owners February 4, 2025 09:52
Copy link

github-actions bot commented Feb 4, 2025

7447 tests run: 7093 passed, 0 failed, 354 skipped (full report)


Flaky tests (1)

Postgres 16

Code coverage* (full report)

  • functions: 33.2% (8580 of 25863 functions)
  • lines: 49.1% (72376 of 147537 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
748bb5d at 2025-02-07T11:55:16.429Z :recycle:

MAX_SEND_SIZE
);
// TODO remove proto_version == 3 after converting all msgs
} else if proto_version == SK_PROTO_VERSION_2 || proto_version == SK_PROTO_VERSION_3 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I missing something but doesn't proto_version == SK_PROTO_VERSION_3 handled in the condition above?
Also I believe comment should be // TODO remove proto_version == 2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, indeed forgot to remove it. I was just using proto_version == SK_PROTO_VERSION_3 to test things before all messages were made into v3. Fixed.

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