[GitHub] [trafficcontrol] rawlinp commented on a diff in pull request #7291: Extended LP feature to aggregate params for all profiles.

["rawlinp (via GitHub)" <gi...@apache.org>]

rawlinp commented on code in PR #7291:
URL: https://github.com/apache/trafficcontrol/pull/7291#discussion_r1089280959


##########
traffic_ops/traffic_ops_golang/monitoring/monitoring.go:
##########
@@ -188,7 +188,7 @@ SELECT
 	status.name as status,
 	cachegroup.name as cachegroup,
 	me.tcp_port as port,
-	profile.name as profile,
+	(SELECT STRING_AGG(sp.profile_name, '+' ORDER by sp.priority ASC) FROM server_profile AS sp where sp.server=me.id group by sp.server) as profile,

Review Comment:
   I think the main reason to generate the "pseudo-aggregate" profile is to avoid breaking the monitoring config format unnecessarily. If we do that, we have to worry about things like keeping 4.0 backwards-compatible while letting 5.0 break, which means more version-handling code in TO and TM. If we just keep it fully backwards-compatible by using this "pseudo-aggregate" profile approach, then we don't need any of that version-handling code at all. Plus, TM wouldn't need to change at all with this approach, because TO has already handled that "profile aggregation" for us.
   
   If we're worried that joining profile names with `+` to create the pseudo-aggregate profile name would cause problems, then we could choose a much less-likely set of characters to join with -- e.g. `+_+_+` or something else I would think is highly unlikely of being used in a real profile name.



-- 
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: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org