Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix looking on all resources when retrieving extensions for preloading collection #463

Merged
merged 4 commits into from
Oct 9, 2024

Conversation

antoinebhs
Copy link
Collaborator

@antoinebhs antoinebhs commented Oct 7, 2024

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)

What kind of change does this PR introduce?

In #411, we introduced a regression where we are looking over all resources to return a map in getAllExtensionsAttributesByResourceType() and getAllExtensionsAttributesByResourceTypeAndExtensionName().
Instead we now return null to avoid looking over all resources. We should refactor this method to return void in another PR.

What is the current behavior?

Looking over all resources to return a map in getAllExtensionsAttributesByResourceType() and getAllExtensionsAttributesByResourceTypeAndExtensionName() takes around 10ms, when writing a network it's called for each equipment which can lead to very long import/export of networks.

What is the new behavior (if this is a feature change)?
We now return null to avoid looking over all resources. We should refactor this method to return void in another PR.

@antoinebhs
Copy link
Collaborator Author

antoinebhs commented Oct 7, 2024

Now, extensionAttributesByIdentifiableId and extensionAttributesByExtensionName are not synchronized with the resources map (before, we retrieved directly the extensions from the up-to-date resources map).
I'm not sure that maintaining the synchronization between the resources map and the new extension maps is worthy as it's prone to error and can probably add some overhead (on each update/remove resource/remove extension, you need to update these maps if there is an extension change in an identifiable).
These maps are not used later as the client uses the resources map (which is always up to date). So they are a bit fake/placeholders because these methods only load extensions to cache from the client perspective but do not return anything used in the client (the return is only used for the delegate pattern...).

I'm not sure what's best here:

  • Implement a method that returns something which is not always up to date (if there are resources removed / updated) => this PR implementation
  • Implement the logic to ensure synchronization between extensionAttributesByIdentifiableId, extensionAttributesByExtensionName and resources map.
  • Return an empty map

Copy link

sonarqubecloud bot commented Oct 9, 2024

@antoinebhs
Copy link
Collaborator Author

After discussing with Jon, we decided to return null for the two methods to avoid the overhead on import/export. I will do another PR so that these methods return void and are only used to load extension to cache and not return extensions.

Copy link
Contributor

@etiennehomer etiennehomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests ok. Performances for import/export are back to normal

@antoinebhs antoinebhs merged commit ff357dd into main Oct 9, 2024
4 checks passed
@antoinebhs antoinebhs deleted the fix-extension-preloading-collection branch October 9, 2024 12:12
antoinebhs added a commit that referenced this pull request Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants