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.

Sign up for GitHub

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

Jump to bottom

Support custom types in broker #5761

Merged
merged 4 commits into from
Dec 19, 2023
Merged

Support custom types in broker #5761

merged 4 commits into from
Dec 19, 2023

Conversation

petuhovskiy
Copy link
Member

@petuhovskiy petuhovskiy commented Nov 1, 2023 •
edited
Loading

Old methods are unchanged for backwards compatibility. Added SafekeeperDiscoveryRequest and SafekeeperDiscoveryResponse types to serve as example, and also as a prerequisite for #5471

@petuhovskiy petuhovskiy marked this pull request as ready for review November 1, 2023 20:57
@petuhovskiy petuhovskiy requested a review from arssher November 1, 2023 20:57
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 1, 2023 •
edited
Loading

2184 tests run: 2101 passed, 0 failed, 83 skipped (full report)

Flaky tests (4)

Postgres 16

  • test_location_conf_churn[1]: debug

Postgres 15

Postgres 14

  • test_crafted_wal_end[last_wal_record_xlog_switch_ends_on_page_boundary]: debug
  • test_ondemand_download_timetravel: debug

Code coverage (full report)

  • functions: 55.0% (9714 of 17671 functions)
  • lines: 82.2% (55807 of 67870 lines)
The comment gets automatically updated with the latest test results
7c4662a at 2023-12-19T16:06:40.773Z :recycle:

@petuhovskiy petuhovskiy force-pushed the storage-broker-discovery branch from b9ae49c to 80986e3 Compare November 3, 2023 14:53
@petuhovskiy
Copy link
Member Author

I'm currently refreshing my protobuf knowledge by reading docs https://protobuf.dev/programming-guides/proto3/
I think I'll update this PR a bit today-tomorrow, but general idea will stay the same.

@petuhovskiy petuhovskiy force-pushed the storage-broker-discovery branch 3 times, most recently from 53cd0a8 to b3d46a2 Compare November 10, 2023 15:00
@petuhovskiy petuhovskiy requested a review from arssher November 10, 2023 15:00
@petuhovskiy
Copy link
Member Author

@arssher I've finished patches that I wanted, ready for review now

arssher
arssher approved these changes Dec 14, 2023
View reviewed changes
Copy link
Contributor

@arssher arssher left a comment

Choose a reason for hiding this comment

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

One unobvious gotcha here is that we have single sub channel for ttid for all message types, which means in theory one type can completely suppress others in dense stream. Moreover, I wanted to change DEFAULT_CHAN_SIZE to 1 or just make channel watch instead of broadcast as accumulating old messages pointless and broker eats a lot of memory in busy regions; with that change it would be even more important. Probably ok for now, but consider fixing it.

storage_broker/proto/broker.proto Show resolved Hide resolved
storage_broker/benches/rps.rs Show resolved Hide resolved
storage_broker/src/bin/storage_broker.rs Show resolved Hide resolved
storage_broker/src/metrics.rs Show resolved Hide resolved
@petuhovskiy petuhovskiy force-pushed the storage-broker-discovery branch from b3d46a2 to 7c4662a Compare December 19, 2023 13:52
@petuhovskiy
Copy link
Member Author

Rebased over main, ready to merge.

@petuhovskiy petuhovskiy merged commit 613906a into main Dec 19, 2023
41 checks passed
@petuhovskiy petuhovskiy deleted the storage-broker-discovery branch December 19, 2023 17:06
abhigets added a commit that referenced this pull request Dec 20, 2023
@abhigets
added if condition inside the reused workflow and in the shell script
304d0f0 
Update jsonwebtoken to 9 and sct to 0.7.1 (#6189)

This increases the list of crates that base on `ring` 0.17.

fixing build

fixing build

testing build

testing build

testing build

testing build

testing build

reverting the build changes

eliminate GCC warning for unchecked result of fread (#6167)

GCCproduce warning that bread result is not checked. It doesn't affect
program logic, but better live without warnings.

Check read result.

- [ ] 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.

- [ ] Do not forget to reformat commit message to not include the above
checklist

Handle role deletion when project has no databases. (#6170)

There is still default 'postgres' database, that may contain objects
owned by the role or some ACLs. We need to reassign objects in this
database too.

If customer deleted all databases and then tries to delete role, that
has some non-standard ACLs,
`apply_config` operation will stuck because of failing role deletion.

fix metric `pageserver_initial_logical_size_start_calculation` (#6191)

It wasn't being incremented.

Fixup of

    commit 1c88824
    Author: Christian Schwarz <[email protected]>
    Date:   Fri Dec 1 12:52:59 2023 +0100

        initial logical size calculation: add a bunch of metrics (#5995)

Support custom types in broker (#5761)

Old methods are unchanged for backwards compatibility. Added
`SafekeeperDiscoveryRequest` and `SafekeeperDiscoveryResponse` types to
serve as example, and also as a prerequisite for
#5471

removed the if check from jobs

ammended review comments

fixing build
abhigets added a commit that referenced this pull request Dec 20, 2023
@abhigets
added if condition inside the reused workflow and in the shell script
45bf494 
Update jsonwebtoken to 9 and sct to 0.7.1 (#6189)

This increases the list of crates that base on `ring` 0.17.

fixing build

fixing build

testing build

testing build

testing build

testing build

testing build

reverting the build changes

eliminate GCC warning for unchecked result of fread (#6167)

GCCproduce warning that bread result is not checked. It doesn't affect
program logic, but better live without warnings.

Check read result.

- [ ] 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.

- [ ] Do not forget to reformat commit message to not include the above
checklist

Handle role deletion when project has no databases. (#6170)

There is still default 'postgres' database, that may contain objects
owned by the role or some ACLs. We need to reassign objects in this
database too.

If customer deleted all databases and then tries to delete role, that
has some non-standard ACLs,
`apply_config` operation will stuck because of failing role deletion.

fix metric `pageserver_initial_logical_size_start_calculation` (#6191)

It wasn't being incremented.

Fixup of

    commit 1c88824
    Author: Christian Schwarz <[email protected]>
    Date:   Fri Dec 1 12:52:59 2023 +0100

        initial logical size calculation: add a bunch of metrics (#5995)

Support custom types in broker (#5761)

Old methods are unchanged for backwards compatibility. Added
`SafekeeperDiscoveryRequest` and `SafekeeperDiscoveryResponse` types to
serve as example, and also as a prerequisite for
#5471

removed the if check from jobs

ammended review comments

fixing build
bayandin pushed a commit that referenced this pull request Dec 20, 2023
@abhigets @bayandin
added if condition inside the reused workflow and in the shell script
9df54f5 
Update jsonwebtoken to 9 and sct to 0.7.1 (#6189)

This increases the list of crates that base on `ring` 0.17.

fixing build

fixing build

testing build

testing build

testing build

testing build

testing build

reverting the build changes

eliminate GCC warning for unchecked result of fread (#6167)

GCCproduce warning that bread result is not checked. It doesn't affect
program logic, but better live without warnings.

Check read result.

- [ ] 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.

- [ ] Do not forget to reformat commit message to not include the above
checklist

Handle role deletion when project has no databases. (#6170)

There is still default 'postgres' database, that may contain objects
owned by the role or some ACLs. We need to reassign objects in this
database too.

If customer deleted all databases and then tries to delete role, that
has some non-standard ACLs,
`apply_config` operation will stuck because of failing role deletion.

fix metric `pageserver_initial_logical_size_start_calculation` (#6191)

It wasn't being incremented.

Fixup of

    commit 1c88824
    Author: Christian Schwarz <[email protected]>
    Date:   Fri Dec 1 12:52:59 2023 +0100

        initial logical size calculation: add a bunch of metrics (#5995)

Support custom types in broker (#5761)

Old methods are unchanged for backwards compatibility. Added
`SafekeeperDiscoveryRequest` and `SafekeeperDiscoveryResponse` types to
serve as example, and also as a prerequisite for
#5471

removed the if check from jobs

ammended review comments

fixing build
abhigets added a commit that referenced this pull request Dec 20, 2023
@abhigets
added if condition inside the reused workflow and in the shell script
164195c 
Update jsonwebtoken to 9 and sct to 0.7.1 (#6189)

This increases the list of crates that base on `ring` 0.17.

reverting the build changes

eliminate GCC warning for unchecked result of fread (#6167)

GCCproduce warning that bread result is not checked. It doesn't affect
program logic, but better live without warnings.

Check read result.

- [ ] 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.

- [ ] Do not forget to reformat commit message to not include the above
checklist

Handle role deletion when project has no databases. (#6170)

There is still default 'postgres' database, that may contain objects
owned by the role or some ACLs. We need to reassign objects in this
database too.

If customer deleted all databases and then tries to delete role, that
has some non-standard ACLs,
`apply_config` operation will stuck because of failing role deletion.

fix metric `pageserver_initial_logical_size_start_calculation` (#6191)

It wasn't being incremented.

Fixup of

    commit 1c88824
    Author: Christian Schwarz <[email protected]>
    Date:   Fri Dec 1 12:52:59 2023 +0100

        initial logical size calculation: add a bunch of metrics (#5995)

Support custom types in broker (#5761)

Old methods are unchanged for backwards compatibility. Added
`SafekeeperDiscoveryRequest` and `SafekeeperDiscoveryResponse` types to
serve as example, and also as a prerequisite for
#5471

removed the if check from jobs

ammended review comments

fixing build

abstracted tag generation of build images in reuse workflow

fixing buikd
abhigets added a commit that referenced this pull request Dec 20, 2023
@abhigets
added if condition inside the reused workflow and in the shell script
335d875 
Update jsonwebtoken to 9 and sct to 0.7.1 (#6189)

This increases the list of crates that base on `ring` 0.17.

reverting the build changes

eliminate GCC warning for unchecked result of fread (#6167)

GCCproduce warning that bread result is not checked. It doesn't affect
program logic, but better live without warnings.

Check read result.

- [ ] 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.

- [ ] Do not forget to reformat commit message to not include the above
checklist

Handle role deletion when project has no databases. (#6170)

There is still default 'postgres' database, that may contain objects
owned by the role or some ACLs. We need to reassign objects in this
database too.

If customer deleted all databases and then tries to delete role, that
has some non-standard ACLs,
`apply_config` operation will stuck because of failing role deletion.

fix metric `pageserver_initial_logical_size_start_calculation` (#6191)

It wasn't being incremented.

Fixup of

    commit 1c88824
    Author: Christian Schwarz <[email protected]>
    Date:   Fri Dec 1 12:52:59 2023 +0100

        initial logical size calculation: add a bunch of metrics (#5995)

Support custom types in broker (#5761)

Old methods are unchanged for backwards compatibility. Added
`SafekeeperDiscoveryRequest` and `SafekeeperDiscoveryResponse` types to
serve as example, and also as a prerequisite for
#5471

removed the if check from jobs

ammended review comments

fixing build

abstracted tag generation of build images in reuse workflow

fixing buikd
abhigets added a commit that referenced this pull request Dec 20, 2023
@abhigets
added if condition inside the reused workflow and in the shell script
7fcac2f 
Update jsonwebtoken to 9 and sct to 0.7.1 (#6189)

This increases the list of crates that base on `ring` 0.17.

reverting the build changes

eliminate GCC warning for unchecked result of fread (#6167)

GCCproduce warning that bread result is not checked. It doesn't affect
program logic, but better live without warnings.

Check read result.

- [ ] 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.

- [ ] Do not forget to reformat commit message to not include the above
checklist

Handle role deletion when project has no databases. (#6170)

There is still default 'postgres' database, that may contain objects
owned by the role or some ACLs. We need to reassign objects in this
database too.

If customer deleted all databases and then tries to delete role, that
has some non-standard ACLs,
`apply_config` operation will stuck because of failing role deletion.

fix metric `pageserver_initial_logical_size_start_calculation` (#6191)

It wasn't being incremented.

Fixup of

    commit 1c88824
    Author: Christian Schwarz <[email protected]>
    Date:   Fri Dec 1 12:52:59 2023 +0100

        initial logical size calculation: add a bunch of metrics (#5995)

Support custom types in broker (#5761)

Old methods are unchanged for backwards compatibility. Added
`SafekeeperDiscoveryRequest` and `SafekeeperDiscoveryResponse` types to
serve as example, and also as a prerequisite for
#5471

removed the if check from jobs

ammended review comments

fixing build

abstracted tag generation of build images in reuse workflow

fixing buikd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arssher arssher arssher approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@petuhovskiy @arssher