PR for normalizing host to all lowercase in the TS API effective URL

[Walt Karas <wk...@verizonmedia.com.INVALID>]

https://github.com/apache/trafficserver/pull/5217

Re: PR for normalizing host to all lowercase in the TS API effective URL

[Leif Hedstrom <zw...@apache.org>]

> On Apr 1, 2019, at 2:57 PM, Walt Karas <wk...@verizonmedia.com.INVALID> wrote:
> 
> https://github.com/apache/trafficserver/pull/5217


Well, other than I seriously question this use of template (how smart are compilers, will it optimize away the to_same_char() calls completely, I don’t know), there might be a real concern here:

	This changes the cache key as well, right?

If so, not only should the PR mention that, we also have to make sure the release notes for v9.0.0 are clear on this. Albeit unlikely, a particular app could get 0% cache hit at an upgrade, if for some reason they used non-normalized scheme or host names in their URLs (bad on them, but we don’t like surprises).

Question 1; Why isn’t the scheme parsed down to a WKS? E.g. URL_WKSIDX_HTTPS; / URL_WKSIDX_HTTP?

Question 2: Since this changes the url_print() method, doesn’t this change behavior for a lot more than just TSHttpTxnEffectiveUrlStringGet() ? url_print() is used by a lot of functions, including url_string_get(), which is used in even more code. If so, the PR title / subject ought to be more descriptive, and not focus on this just one API.


I think I’m ok with the general concept of normalizing these fields (better for cache hit ratios). Not stoked with the template.


— leif