Create attribute based NumberPools when the schema is updated #6618
Conversation
github-actions
bot
added
the
group/backend
CodSpeed Performance ReportMerging #6618 will not alter performanceComparing Summary
|
ogenstad
force-pushed
the
pog-precreate-number-pools-IFC-1551
branch
from
843137f to
eb242e8
Compare
| start_range=attribute.parameters.start_range, | ||
| end_range=attribute.parameters.end_range, | ||
| ) | ||
| existing_pool_ids.append(attribute.parameters.number_pool_id) |
There was a problem hiding this comment.
Can we factorize above blocks in a single one? Even if it means calling SchemaManager.get
There was a problem hiding this comment.
I think for now the above might actually be a bit cleaner just due to the name of the kind as we'd need to find the name of the generic that defined the attribute in the case of a node_schema. I'll take another look at this later.
| @@ -54,3 +55,62 @@ async def validate_schema_number_pools( | |||
| elif not defined_on_branches: | |||
| log.info(f"Deleting number pool (id={schema_number_pool.id}) as it is no longer defined in the schema") | |||
| await schema_number_pool.delete(db=service.database) | |||
|
|
|||
| existing_pool_ids = [pool.id for pool in schema_number_pools] | |||
There was a problem hiding this comment.
I don't know how big a change it is but I think it would be better to let the id being set when we create the number pool as for a regular node instead of setting it during validate_attribute_parameters. Is there still a value to have it set earlier?
There was a problem hiding this comment.
That said maybe I miss something because I am not sure where the number pool is currently created (regardless of this PR). As it seems we now want to create schema number pools during schema validation, should we remove the code responsible for creating it in the first place?
There was a problem hiding this comment.
It's done like this with the ID because we need the ID to be in the schema. I think timing wise it would be better to keep the other creation as well as we can't be sure of the order of operations here. I plan to open a new PR later with a lock around the two of them though.
This PR changes how attribute based NumberPools are created. Previously they were only created the first time they were needed which led to the issue of the schema within the frontend linking to something that didn't exist which caused some confusion.
With this change we have an alternate way where the pool is created during the validation process of those pools.
There might still be a few seconds delay so if a user is quick and visits the schema page directly after loading the schema there might be an instance where the pool hasn't yet been created, I think the risk is quite low though.