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

NOISSUE - Add subtopic wildcard for twin attribute's definition #1214

Merged
merged 6 commits into from
Jul 9, 2020

Conversation

darkodraskovic
Copy link
Contributor

@darkodraskovic darkodraskovic commented Jul 6, 2020

What does this do?

Twin stores semantic definition of a data system. Such definition consists of attributes which are uniquely defined by the combination of channel and subtopic. Currently, we can only define attribute to match the combination of an empty or non-empty subtopic and channel. However, we don't have means to define attribute to match all the combinations of a certain channel and subtopics, i.e. we don't have means to store to states all message of a channel irrespective of the subtopic used to send messages. Adition of a wildcard subtopic enables attribute to pick up all messages sent via it's channel.

List any changes that modify/break current functionality

N/A

Have you included tests for your changes?

Yes

Did you document any new/modified functionality?

Yes

@darkodraskovic darkodraskovic requested a review from a team as a code owner July 6, 2020 14:59
@darkodraskovic darkodraskovic marked this pull request as draft July 6, 2020 14:59
@codecov-commenter
Copy link

codecov-commenter commented Jul 6, 2020

Codecov Report

Merging #1214 into master will increase coverage by 0.41%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1214      +/-   ##
==========================================
+ Coverage   77.02%   77.44%   +0.41%     
==========================================
  Files         104      104              
  Lines        6868     6875       +7     
==========================================
+ Hits         5290     5324      +34     
+ Misses       1196     1164      -32     
- Partials      382      387       +5     
Impacted Files Coverage Δ
twins/redis/twins.go 55.55% <50.00%> (-0.45%) ⬇️
twins/mongodb/twins.go 81.19% <100.00%> (+25.93%) ⬆️
twins/service.go 71.96% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09d09c6...e8852a1. Read the comment docs.

twins/README.md Outdated Show resolved Hide resolved
twins/redis/twins.go Outdated Show resolved Hide resolved
@darkodraskovic darkodraskovic marked this pull request as ready for review July 8, 2020 09:55
Signed-off-by: Darko Draskovic <darko.draskovic@gmail.com>
Signed-off-by: Darko Draskovic <darko.draskovic@gmail.com>
Signed-off-by: Darko Draskovic <darko.draskovic@gmail.com>
Signed-off-by: Darko Draskovic <darko.draskovic@gmail.com>
Signed-off-by: Darko Draskovic <darko.draskovic@gmail.com>
twins/README.md Outdated Show resolved Hide resolved
twins/README.md Outdated Show resolved Hide resolved
twins/mocks/states.go Outdated Show resolved Hide resolved
@@ -74,6 +74,11 @@ func (tc *twinCache) IDs(_ context.Context, channel, subtopic string) ([]string,
if err != nil {
return nil, errors.Wrap(ErrRedisTwinIDs, err)
}
idsWildcard, err := tc.client.SMembers(attrKey(channel, twins.SubtopicWildcard)).Result()
Copy link
Contributor

Choose a reason for hiding this comment

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

SMembers could have better naming

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please elaborate on this. SMembers is redis function.

Signed-off-by: Darko Draskovic <darko.draskovic@gmail.com>
Copy link
Contributor

@manuio manuio left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -38,6 +41,15 @@ func CreateDefinition(channels []string, subtopics []string) twins.Definition {
return def
}

// CreateTwin creates twin
func CreateTwin(channels []string, subtopics []string) twins.Twin {
id++
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not know if having a global var would be a good idea here. I do not go in deeper analysis, but there is some "code smell". I would prefer some internal structure, created in the main() via New() fnc, then passed back here... Or something like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This id is not a global var, it's a package level var, not exported, used for the sole purpose of generating unique id's for the mock entitites that we use to test service. It's rather so called closure variable. The function users can't know anything about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@darkodraskovic sorry I did not see this was for the mock. I will approve then. I let you take a look at this Manu's remark optionally, but this is good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@manuio i purposefully didn't use the mock uuid generator, cause i don't want to handle uuid error creation. I really need only mock id's, can be really anything, it's just for the purpose of differentiating the twins, so I just give them unique serial number like this - no possibility of error, so no need for error handling.

Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -38,6 +41,15 @@ func CreateDefinition(channels []string, subtopics []string) twins.Definition {
return def
}

// CreateTwin creates twin
func CreateTwin(channels []string, subtopics []string) twins.Twin {
id++
Copy link
Contributor

Choose a reason for hiding this comment

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

@darkodraskovic sorry I did not see this was for the mock. I will approve then. I let you take a look at this Manu's remark optionally, but this is good to me.

@manuio manuio merged commit 381ebb1 into absmach:master Jul 9, 2020
@darkodraskovic darkodraskovic deleted the twins-joker branch July 9, 2020 13:03
manuio pushed a commit that referenced this pull request Oct 12, 2020
* Add wildcard to attribute subtopic

Signed-off-by: Darko Draskovic <darko.draskovic@gmail.com>

* Add MF_TWINS_SUBTOPIC_WILDCARD env var

Signed-off-by: Darko Draskovic <darko.draskovic@gmail.com>

* Remove configurable wildcard env var and mqtt notif leftovers

Signed-off-by: Darko Draskovic <darko.draskovic@gmail.com>

* Add mongodb RetrieveByAttribute tests

Signed-off-by: Darko Draskovic <darko.draskovic@gmail.com>

* Add redis wildcard subtopic IDs retrieval

Signed-off-by: Darko Draskovic <darko.draskovic@gmail.com>

* Add tests for wildcard state save

Signed-off-by: Darko Draskovic <darko.draskovic@gmail.com>
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.

5 participants