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

pageserver: improve readability of shard.rs #7330

Merged
merged 4 commits into from Apr 15, 2024
Merged

Conversation

jcsp
Copy link
Contributor

@jcsp jcsp commented Apr 5, 2024

No functional changes, this is a comments/naming PR.

While merging sharding changes, some cleanup of the shard.rs types was deferred.

In this PR:

  • Rename is_zero to is_shard_zero to make clear that this method doesn't literally mean that the entire object is zeros, just that it refers to the 0th shard in a tenant.
  • Pull definitions of types to the top of shard.rs and add a big comment giving an overview of which type is for what.

Closes: #6072

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

@jcsp jcsp added c/storage/pageserver Component: storage: pageserver a/tech_debt Area: related to tech debt labels Apr 5, 2024
@jcsp jcsp requested a review from a team as a code owner April 5, 2024 17:52
@jcsp jcsp requested a review from koivunej April 5, 2024 17:52
Copy link

github-actions bot commented Apr 5, 2024

No tests were run or test report is not available

Code coverage* (full report)

  • functions: 28.0% (6428 of 22962 functions)
  • lines: 46.6% (45012 of 96565 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
124c054 at 2024-04-12T12:39:28.459Z :recycle:

@jcsp jcsp changed the title Jcsp/sharding comments pageserver: improve readability of shard.rs Apr 6, 2024
libs/pageserver_api/src/shard.rs Outdated Show resolved Hide resolved
libs/pageserver_api/src/shard.rs Outdated Show resolved Hide resolved
libs/pageserver_api/src/shard.rs Outdated Show resolved Hide resolved
libs/pageserver_api/src/shard.rs Outdated Show resolved Hide resolved
@arpad-m
Copy link
Member

arpad-m commented Apr 10, 2024

On a related note, I wonder why TenantShardId is in pageserver_api: TenantId is in the utils crate instead, same for TimelineId. In #7322 I had to add a dependency on pageserver_api because of TenantShardId (it was already a transitive dependency, but still).

jcsp and others added 2 commits April 12, 2024 14:55
Co-authored-by: Arpad Müller <arpad-m@users.noreply.github.com>
@jcsp
Copy link
Contributor Author

jcsp commented Apr 12, 2024

On a related note, I wonder why TenantShardId is in pageserver_api: TenantId is in the utils crate instead, same for TimelineId.

I wanted to avoid exposing all the shard.rs content to modules that don't really talk to pageservers, as it's kind of a concept that's "owned" by the pageserver (at least, until the safekeeper becomes shard aware). In future it might be nice to define a crate for "foundational types" (or some better name) -- these ID types aren't really "utilities as such".

@jcsp jcsp merged commit 83cdbbb into main Apr 15, 2024
52 of 53 checks passed
@jcsp jcsp deleted the jcsp/sharding-comments branch April 15, 2024 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a/tech_debt Area: related to tech debt c/storage/pageserver Component: storage: pageserver
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pageserver: clean up sharding helper names
2 participants