You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "zwoop (via GitHub)" <gi...@apache.org> on 2024/11/14 23:53:11 UTC
[PR] Adds a fragment-size= option to volume.config [trafficserver]
zwoop opened a new pull request, #11865:
URL: https://github.com/apache/trafficserver/pull/11865
(no comment)
--
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] Adds a fragment-size= option to volume.config [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #11865:
URL: https://github.com/apache/trafficserver/pull/11865#discussion_r1843038143
##########
src/iocore/cache/Stripe.h:
##########
@@ -115,7 +117,7 @@ class Stripe
*
* @see START_POS
*/
- Stripe(CacheDisk *disk, off_t blocks, off_t dir_skip, int avg_obj_size = -1);
+ Stripe(CacheDisk *disk, off_t blocks, off_t dir_skip, int avg_obj_size = -1, int fragment_size = -1);
Review Comment:
This could make take the vol config directly, especially if we are likely to add more configuration parameters in the future.
##########
src/iocore/cache/StripeSM.h:
##########
@@ -168,7 +168,7 @@ class StripeSM : public Continuation, public Stripe
*
* @see START_POS
*/
- StripeSM(CacheDisk *disk, off_t blocks, off_t dir_skip, int avg_obj_size = -1);
+ StripeSM(CacheDisk *disk, off_t blocks, off_t dir_skip, int avg_obj_size = -1, int fragment_size = -1);
Review Comment:
Looks like the docstring is out of date.
--
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] Adds a fragment-size= option to volume.config [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #11865:
URL: https://github.com/apache/trafficserver/pull/11865#issuecomment-2477668411
The new configuration is per-volume. The `cache_config_` is the default for all volumes. We need both.
--
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] Adds a fragment-size= option to volume.config [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop merged PR #11865:
URL: https://github.com/apache/trafficserver/pull/11865
--
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] Adds a fragment-size= option to volume.config [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #11865:
URL: https://github.com/apache/trafficserver/pull/11865#issuecomment-2477658592
Would it make sense to remove the `cache_config_` global for this, or mark it const? The purposes seem to be redundant to me.
Maybe it would be good to introduce a type to hold cache options for initialization at some point; doesn't need to be in this PR though.
--
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] Adds a fragment-size= option to volume.config [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #11865:
URL: https://github.com/apache/trafficserver/pull/11865#discussion_r1843043114
##########
src/iocore/cache/Stripe.h:
##########
@@ -115,7 +117,7 @@ class Stripe
*
* @see START_POS
*/
- Stripe(CacheDisk *disk, off_t blocks, off_t dir_skip, int avg_obj_size = -1);
+ Stripe(CacheDisk *disk, off_t blocks, off_t dir_skip, int avg_obj_size = -1, int fragment_size = -1);
Review Comment:
On second thought, that may not be great, because each config is used for an entire cache span (volume), so the dependencies would get messier. I think it'd be nice to have a config type for stripes at some point, which could maybe be part of the volume config, but let's leave it as is for now.
--
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] Adds a fragment-size= option to volume.config [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #11865:
URL: https://github.com/apache/trafficserver/pull/11865#issuecomment-2477672330
Actually, I take that back, the records.config value ("the default") being reloadable, we can't set that in the stripe value, because then the stripes are fixed for the runtime of the process. So I think we have to leave this as-is.
--
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] Adds a fragment-size= option to volume.config [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #11865:
URL: https://github.com/apache/trafficserver/pull/11865#discussion_r1844004649
##########
src/iocore/cache/StripeSM.h:
##########
@@ -168,7 +168,7 @@ class StripeSM : public Continuation, public Stripe
*
* @see START_POS
*/
- StripeSM(CacheDisk *disk, off_t blocks, off_t dir_skip, int avg_obj_size = -1);
+ StripeSM(CacheDisk *disk, off_t blocks, off_t dir_skip, int avg_obj_size = -1, int fragment_size = -1);
Review Comment:
Updated.
--
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] Adds a fragment-size= option to volume.config [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #11865:
URL: https://github.com/apache/trafficserver/pull/11865#issuecomment-2477669333
> Would it make sense to remove the `cache_config_` global for this, or mark it const? The purposes seem to be redundant to me.
Well, I don't think we can (easily) eliminate cache_config_target_fragment_size, it's a reloadable configuration, and it has to have a "destination" to write to in the callback.
What we could do is to simplify target_fragment_size() by always setting the StripeSMs size to either cache_config_target_fragment_size or the volume.config specified value. Since the cache is a bit of a mystery, I was worried about making too many changes ... But I could do this one (it avoids a "comparison" at runtime).
--
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