Also, if we do it this way we 1. Need to undo the work Alan started. We should not have two ways to deal with the strings configurations and conversions.
2. Figure out how to do this without breaking the compatibility every time we add a type. I’m not sold on the std:: variant here either, besides the compatibility issues, it will become large I think. It is the c++ way though, but probably not for this use case. — Leif On Thu, Jun 12, 2025 at 08:00 Leif Hedstrom <apachezw...@gmail.com> wrote: > > > > On Jun 12, 2025, at 01:15, Masaori Koshiba <masa...@apache.org> wrote: > > > > I'd propose adding new TS APIs that make overriding configuration > > variables more flexible. > > > > ``` > > TS_RECORDDATATYPE_VARIANT > > using TSConfigValue = std::variant<HttpStatusCodeList *>; > > > > TSReturnCode TSHttpTxnConfigParse(TSConfigValue &dst, > > TSOverridableConfigKey key, const char *value, size_t length); > > TSReturnCode TSHttpTxnConfigSet(TSHttpTxn txnp, TSOverridableConfigKey > > key, const TSConfigValue &src); > > ``` > > > > ## Current Limitations > > > > Today, we can override configs only in int, float, and string. > > > > ``` > > TSReturnCode TSHttpTxnConfigIntSet(TSHttpTxn txnp, > > TSOverridableConfigKey conf, TSMgmtInt value); > > TSReturnCode TSHttpTxnConfigFloatSet(TSHttpTxn txnp, > > TSOverridableConfigKey conf, TSMgmtFloat value); > > TSReturnCode TSHttpTxnConfigStringSet(TSHttpTxn txnp, > > TSOverridableConfigKey conf, const char *value, int length); > > ``` > > > > However, some configs like `negative_caching_list` are list. While > > it's technically possible to override these configs with > > `TSHttpTxnConfigStringSet` and `MgmtConverter`, every transaction > > needs to run the converter. > > That’s not how it is supposed to work, it should only run a converter when > loading the text configuration (if there is one). > > If we have configs that don’t adhere to that, we ought to fix them before > anything else. Do you have a list of the configurations that aren’t doing > this right? They each should have a concession function that gets called > when we load or “set” the string. > > The “list” types are a mess for sure though, and the code was never > finished completely. > > > > > ## New APIs > > > > With the new APIs, we can > > > > - Parse only once on (re)loading configs > > - Store the parsed structure in a `TSConfigValue` > > - Override the value on remap > > > > `TSConfigValue` is a `std::variant` for type safety and future > > extension. For now, it has only `HttpStatusCodeList *` for > > negative_caching_list and negative_revalidating_list, but whenever we > > want to override configs that has other types, we can add it in this > > variant. > > That means we break compatibility every time we add a new type ? > > — Leif > > > > Please take a look at PR#12284 for implementations. > > > > https://github.com/apache/trafficserver/pull/12284 > > > > Thanks, > > Masaori >
