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 Avoid triggering notifications if only the metadata changes (continuation in prelanding branch) #4310

Conversation

fgalan
Copy link
Member

@fgalan fgalan commented Apr 13, 2023

Issues #3727

This PR incorporates the work in PR #4300. I will continue the work from that point

CC: @chetan-NEC

@fgalan fgalan changed the title Avoid triggering notifications if only the metadata changes prelanding Fix Avoid triggering notifications if only the metadata changes (continuation in prelanding branch) Apr 13, 2023
op = true;
}
// By definition, notificationVector cannot be empty in blacklist case
// (checked at parsing time)
Copy link
Member Author

Choose a reason for hiding this comment

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

A .test will be added to cover this case, in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

PR #4317

NTC (in this PR)

@@ -0,0 +1,229 @@
# Copyright 2023 Telefonica Investigacion y Desarrollo, S.A.U
Copy link
Member Author

Choose a reason for hiding this comment

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

Add also a .test for the basic notifyOnMetadataChange CRUD. For instance:

  • Create entity without notifyOnMetadataChange (defaults to true)
  • Modify entity with notifyOnMetadataChange: false
  • Modify entity with notifyOnMetadataChange: true
  • Modify entity with notifyOnMetadataChange: false
  • Modify entity witout notifyOnMetadataChange (defaults to true)

Copy link
Member Author

Choose a reason for hiding this comment

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

s/entity/subscription/ above :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in aee4daa

@fgalan fgalan force-pushed the avoid-triggering-notifications-if-only-the-metadata-changes-prelanding branch from 9373975 to 2fd6f7b Compare April 26, 2023 12:13
@fgalan fgalan force-pushed the avoid-triggering-notifications-if-only-the-metadata-changes-prelanding branch from 2fd6f7b to aee4daa Compare April 28, 2023 11:00
@fgalan fgalan requested a review from mapedraza April 28, 2023 11:47
@fgalan
Copy link
Member Author

fgalan commented Apr 28, 2023

@chetan-NEC I'd suggest to have a look to the changes 12edfad...f749ec9 in this PR.

This is the way of solving the implementation in the line I commented here. The key change is to make mergeAttrInfo() return a ChangeType that specifies the exact change type has taken place, instead of using bool. All the changes of the code are a straightforward consequence of it (except by some simplifications in subToNotifyList() and MongoGlobal functions that I did as I looked to that code).

Note that with this changes no modification has to be done in the ngsiNotify module. The decission to trigger subscription or not is done before in the CB logic, upon entity update processing. As side effect of this, unit tests doesn't need any change so they are passing again.

I think it would be a good idea if you could review this changes in deep. It would increase you knowledge on Context Broker code so you new PRs will be easier for you :)

Copy link
Collaborator

@mapedraza mapedraza left a comment

Choose a reason for hiding this comment

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

LGTM

@mapedraza mapedraza merged commit be2b3e9 into master Apr 28, 2023
@mapedraza mapedraza deleted the avoid-triggering-notifications-if-only-the-metadata-changes-prelanding branch April 28, 2023 14:41
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.

3 participants