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

Decouple disk from stripe #11737

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

JosiahWI
Copy link
Contributor

@JosiahWI JosiahWI commented Aug 23, 2024

This moves the following variables out of Stripe:

  • fd
  • path
  • disk
  • hash_text

@JosiahWI JosiahWI added this to the 10.1.0 milestone Aug 23, 2024
@JosiahWI JosiahWI self-assigned this Aug 23, 2024
@JosiahWI JosiahWI force-pushed the refactor/decouple-disk-from-stripe branch from b76be6e to f6b029d Compare August 23, 2024 12:29
@masaori335
Copy link
Contributor

A tricky idea is adding a pointer to the StripeSM to the Stripe.
https://github.com/JosiahWI/trafficserver/pull/2/files

@@ -65,6 +65,8 @@ class StripeSM : public Continuation, public Stripe
{
public:
CryptoHash hash_id;
char *path = nullptr;
int fd{-1};
Copy link
Contributor

Choose a reason for hiding this comment

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

CacheDisk has path and fd. I start wondering we should just refer them instead of copy it because StripeSM has CacheDisk *.


cp->stripes[vol_no]->fd = d->fd;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure enough. We should definitely deduplicate that! Good find.

Copy link

This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community.

@github-actions github-actions bot added the Stale label Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants