[uss_qualifier/astm] Make cleanup phase in down_uss use estimated extents as well#1405
[uss_qualifier/astm] Make cleanup phase in down_uss use estimated extents as well#1405the-glu wants to merge 1 commit intointeruss:mainfrom
Conversation
| details=f"DSS responded code {del_query.status_code}; {e}", | ||
| query_timestamps=[del_query.request.timestamp], | ||
| ) | ||
| for area in [ |
There was a problem hiding this comment.
By always potentially clearing according to both areas, we're probably being a little less clear (scenario_execution_max_extents will never be relevant when calling this function from the setup step) and a little more broad than necessary (we don't need to clear estimated_max_extents in the end-of-scenario cleanup if nothing actually happened during the scenario). If we were still going to do this, I think we should still make just one query as queries are far, far more expensive than a tiny bit of local math:
areas = Volume4DCollection([])
if self.scenario_execution_max_extents:
areas.append(self.scenario_execution_max_extents)
if self.estimated_max_extents:
areas.append(self.estimated_max_extents)
if not areas:
return
area = areas.bounding_volumeBut, it seems like we could instead simply have _clear_op_intents accept a Volume4D and call it with estimated_max_extents in setup and scenario_execution_max_extents in end-of-scenario cleanup.
| self.scenario_execution_max_extents, | ||
| ]: | ||
| if area is None: | ||
| continue |
There was a problem hiding this comment.
This is a pre-existing problem, but we're initiating the check too early. Ideally, we should complete the action to collect data before we initiate the check to evaluate that data. But, it would be inconvenient to need to repeat the with initiation statement in two different places, so I think practically it's ok to initiate the check before performing the action (find_op_intent) since starting to perform the action necessarily means we're going to perform the check one way or another. But, we should not initiate the check if we're not actually going to perform the action (and therefore definitely not perform the check).
If area is None, currently the check will indicate as passing, but that is incorrect as we haven't checked anything. Instead, we should make sure we're actually going to check something before initiating the check since initiating a check and not recording a failure is defined as meaning that the check has positively passed.
This PR should fix #1400 by also using estimated extends during cleanup phase.
Initial cleanup was not doing anything during initial phase since it was using
scenario_execution_max_extentsonly, where as check was done onestimated_max_extents.I didn't verify exact scenario in original issue, but looking at report, there is now a query to perform clean up:
(before, no query was done:
so potentially existing OP intents would not being deleted
)
And having a previous test leaving an OPI cleanup it as expected:
The other usage of
estimate_scenario_execution_max_extentsdon't perform any cleanup (but does perform an 'area clear' check), so I didn't change anything there.