records - Honour the minimum value for --maxRecords

[brbzull0]

Although the default and minimum value is 2048 (as advertised) ATS will accept any value less than the advertised default, which will end up in ATS crashing. This change sets the advertised default value if the passed value is invalid or less than the default.

Not sure if this may impact someone using different default values, or max values for records?

[cmcfarlen]

It's a little odd that you can change the number of records allocated at all currently. I think you'll get issues if you set it > REC_MAX_RECORDS. See this here:
https://github.com/apache/trafficserver/blob/master/src/records/RecUtils.cc#L54-L57

Maybe we should either fix that or remove (reremove?) the ability to change how many are allocated?

[brbzull0]

It's a little odd that you can change the number of records allocated at all currently. I think you'll get issues if you set it > REC_MAX_RECORDS. See this here: https://github.com/apache/trafficserver/blob/master/src/records/RecUtils.cc#L54-L57

Maybe we should either fix that or remove (reremove?) the ability to change how many are allocated?

Thanks Chris, I was looking for feedback here too.

Agreed it is odd, the whole point is to avoid crashing. I did some tests before putting this PR testing out after passing this max, I know some people just change the REC_MAX_RECORDS on their own build. The above check happens when it actually reads the records.yaml and not by us setting this manually I believe.

I did some tests also with a large records.yaml(and also changing the REC_MAX_RECORDS to avoid the warning) and it was fine, which makes me think we can actually(as you suggested) remove the hard max from the code and have it as default instead, with the possibility ofc to change it. I believe this max is a legacy restriction from the old traffic_manager stuff(for metrics and records).

I'll make some changes here(and add some AuTest) and we can keep discussing.

Thanks.