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

Optimize GetNodeMetadata #1392

Merged
merged 9 commits into from
May 5, 2021
Merged

Conversation

mrsuciu
Copy link
Contributor

@mrsuciu mrsuciu commented May 4, 2021

  • Reduce number of callback hits on reading attributes
  • Reduce number of reads to underlying data sources for unused attributes
  • Use temporary cache to avoid multiple attribute read calls
  • Optimize GetNodeMetadata calls for Read service calls
  • Minor API change: INodeManager.GetNodeMetadata signature update

…olePermission attributes when specified; All service calls use only these attributes on validating AccessRestrictions and RolePermissions. Read and Browse services also cache the values for avoiding unecessary calls.
…OnlyValidationAttributes into permitionsOnly
@AlinMoldovean AlinMoldovean requested a review from mregen May 4, 2021 11:35
@mregen mregen requested a review from AlinMoldovean May 4, 2021 13:11
Copy link
Contributor

@mregen mregen left a comment

Choose a reason for hiding this comment

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

beside this question on the signature, it looks ok

object targetHandle,
BrowseResultMask resultMask,
Dictionary<NodeId, List<object>> uniqueNodesServiceAttributes = null,
bool permissionsOnly = false)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

is there really no issue changing the signature, e.g. with compiled code? In the past I see there was a INodeManager2 added with additional functions. same could be done here with a GetNodeMetaData2 with more parameters in a INodeManager3?

Copy link
Contributor

Choose a reason for hiding this comment

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

for compat it might also be etter to add the 5 parm function separately and not use the defaults. Maybe add it in INodeManager2 only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

INodeManager2 now contains the 5 param function GetPermissionsMetadata which handles the optimizations and GetNodeMetadata is left as is.

…ptimized retrieval if necessary and reverted changes to INodeManager.GetNodeMetadata implementation; Fixed missing summary and typeo's
@AlinMoldovean AlinMoldovean merged commit 8b9bbe2 into OPCFoundation:master May 5, 2021
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.

None yet

3 participants