You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "JosiahWI (via GitHub)" <gi...@apache.org> on 2024/07/30 15:22:03 UTC
[PR] Extract `PreservationTable` from `StripeSM` [trafficserver]
JosiahWI opened a new pull request, #11628:
URL: https://github.com/apache/trafficserver/pull/11628
This extracts `PreservationTable` as a superclass. The new class represents the responsibility of tracking documents that need to be preserved to avoid being overwritten, such as pinned documents. Docstrings are added to better document how this process works. The `StripeSM::evac_range` method is left with that class because it is not part of the evacuation data structure despite the name. It should be refactored and renamed in a future PR. The documentation has been updated.
Waiting to rebase on #11623.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract `PreservationTable` from `StripeSM` [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #11628:
URL: https://github.com/apache/trafficserver/pull/11628#discussion_r1702225077
##########
src/iocore/cache/P_CacheVol.h:
##########
@@ -73,34 +66,7 @@ struct DiskStripe;
struct CacheVol;
class CacheEvacuateDocVC;
-// Key and Earliest key for each fragment that needs to be evacuated
-struct EvacuationKey {
- SLink<EvacuationKey> link;
- CryptoHash key;
- CryptoHash earliest_key;
-};
-
-struct EvacuationBlock {
- union {
- unsigned int init;
- struct {
- unsigned int done : 1; // has been evacuated
- unsigned int pinned : 1; // check pinning timeout
- unsigned int evacuate_head : 1; // check pinning timeout
- unsigned int unused : 29;
- } f;
- };
-
- int readers;
- Dir dir;
- Dir new_dir;
- // we need to have a list of evacuationkeys because of collision.
- EvacuationKey evac_frags;
- CacheEvacuateDocVC *earliest_evacuator;
- LINK(EvacuationBlock, link);
-};
-
-class StripeSM : public Continuation, public Stripe
+class StripeSM : public Continuation, public Stripe, public PreservationTable
Review Comment:
I don't think it's presently an issue for it to be `isA`, but I'm also thinking it should be `hasA`. My reason was that I want to add a constructor, but bringing in some logic from outside `StateSM` is another great reason. The question is whether to do it immediately in this PR. I didn't do it yet because the added delegation is more complicated than doing the superclass and minimizing the changes minimizes risk, but I think it'd be fine to do it now if you think that's better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract `PreservationTable` from `StripeSM` [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI merged PR #11628:
URL: https://github.com/apache/trafficserver/pull/11628
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract `PreservationTable` from `StripeSM` [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on code in PR #11628:
URL: https://github.com/apache/trafficserver/pull/11628#discussion_r1702331009
##########
src/iocore/cache/P_CacheVol.h:
##########
@@ -73,34 +66,7 @@ struct DiskStripe;
struct CacheVol;
class CacheEvacuateDocVC;
-// Key and Earliest key for each fragment that needs to be evacuated
-struct EvacuationKey {
- SLink<EvacuationKey> link;
- CryptoHash key;
- CryptoHash earliest_key;
-};
-
-struct EvacuationBlock {
- union {
- unsigned int init;
- struct {
- unsigned int done : 1; // has been evacuated
- unsigned int pinned : 1; // check pinning timeout
- unsigned int evacuate_head : 1; // check pinning timeout
- unsigned int unused : 29;
- } f;
- };
-
- int readers;
- Dir dir;
- Dir new_dir;
- // we need to have a list of evacuationkeys because of collision.
- EvacuationKey evac_frags;
- CacheEvacuateDocVC *earliest_evacuator;
- LINK(EvacuationBlock, link);
-};
-
-class StripeSM : public Continuation, public Stripe
+class StripeSM : public Continuation, public Stripe, public PreservationTable
Review Comment:
I don't feel strongly about it, but I do think it would help clean up some boundaries. Leaving the more impactful refactoring to another PR sounds smart to me as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract `PreservationTable` from `StripeSM` [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on code in PR #11628:
URL: https://github.com/apache/trafficserver/pull/11628#discussion_r1702218787
##########
src/iocore/cache/P_CacheVol.h:
##########
@@ -73,34 +66,7 @@ struct DiskStripe;
struct CacheVol;
class CacheEvacuateDocVC;
-// Key and Earliest key for each fragment that needs to be evacuated
-struct EvacuationKey {
- SLink<EvacuationKey> link;
- CryptoHash key;
- CryptoHash earliest_key;
-};
-
-struct EvacuationBlock {
- union {
- unsigned int init;
- struct {
- unsigned int done : 1; // has been evacuated
- unsigned int pinned : 1; // check pinning timeout
- unsigned int evacuate_head : 1; // check pinning timeout
- unsigned int unused : 29;
- } f;
- };
-
- int readers;
- Dir dir;
- Dir new_dir;
- // we need to have a list of evacuationkeys because of collision.
- EvacuationKey evac_frags;
- CacheEvacuateDocVC *earliest_evacuator;
- LINK(EvacuationBlock, link);
-};
-
-class StripeSM : public Continuation, public Stripe
+class StripeSM : public Continuation, public Stripe, public PreservationTable
Review Comment:
Does StripeSM need to inherit PreservationTable or can it be aggregate? It seems like the few public members could be either internalized or proxied. I prefer more hasA to isA, especially since StripeSM is already spread across multiple classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org