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 - Adding subtopics filtering in writer services #1072

Merged
merged 20 commits into from
Mar 30, 2020

Conversation

jonathandreyer
Copy link
Contributor

What does this do?

Adding subtopic's filtering for each writer service. It is strongly based on channel's filtering.

Which issue(s) does this PR fix/relate to?

Also resolves #925.

List any changes that modify/break current functionality

Have you included tests for your changes?

Did you document any new/modified functionality?

Update readme of each writer service with new

Notes

I have also refactored a little part of the main code of every writer services to centralize duplicate code into writer package.

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>
Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>
Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>
Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>
Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>
@jonathandreyer jonathandreyer requested a review from a team as a code owner March 11, 2020 22:05
@codecov-io
Copy link

codecov-io commented Mar 11, 2020

Codecov Report

Merging #1072 into master will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1072      +/-   ##
==========================================
+ Coverage   77.93%   77.99%   +0.06%     
==========================================
  Files          95       95              
  Lines        6743     6685      -58     
==========================================
- Hits         5255     5214      -41     
+ Misses       1174     1159      -15     
+ Partials      314      312       -2     
Impacted Files Coverage Δ
http/api/transport.go 74.39% <0.00%> (+0.55%) ⬆️
ws/adapter.go 93.75% <0.00%> (+12.21%) ⬆️

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 70955d1...eab3574. Read the comment docs.

cmd/cassandra-writer/main.go Outdated Show resolved Hide resolved
cmd/cassandra-writer/main.go Outdated Show resolved Hide resolved
docker/addons/postgres-writer/subtopics.toml Outdated Show resolved Hide resolved
docker/addons/mongodb-writer/subtopics.toml Outdated Show resolved Hide resolved
writers/writer.go Outdated Show resolved Hide resolved
writers/writer.go Outdated Show resolved Hide resolved
writers/writer.go Outdated Show resolved Hide resolved
writers/writer.go Outdated Show resolved Hide resolved
writers/writer.go Outdated Show resolved Hide resolved
Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>
@jonathandreyer jonathandreyer force-pushed the feature/subtopics-filter-writer branch from a5cbaec to 2ffb670 Compare March 12, 2020 20:19
…erge both loading methods & returning error)

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>
cmd/influxdb-writer/main.go Outdated Show resolved Hide resolved
cmd/postgres-writer/main.go Outdated Show resolved Hide resolved
cmd/mongodb-writer/main.go Outdated Show resolved Hide resolved
cmd/cassandra-writer/main.go Outdated Show resolved Hide resolved
@manuio
Copy link
Contributor

manuio commented Mar 12, 2020

@jonathandreyer update your branch please

writers/writer.go Outdated Show resolved Hide resolved
writers/writer.go Outdated Show resolved Hide resolved
writers/writer.go Outdated Show resolved Hide resolved
writers/writer.go Outdated Show resolved Hide resolved
writers/writer.go Outdated Show resolved Hide resolved
Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>
cmd/cassandra-writer/main.go Outdated Show resolved Hide resolved
@drasko
Copy link
Contributor

drasko commented Mar 12, 2020

I did not yet go into details, but at the first look this looks over-complicated to me. We must brainstorm to find a simpler solution. This introduces impact, and is not something that I'd like to maintain (at least at the first look).

@jonathandreyer can you please describe us your use-case and requirement here in greater detail. From what I understood, you needed to redirect messages to 2 different DBs. Why could not you do it on the channel level?

@jonathandreyer
Copy link
Contributor Author

I did not yet go into details, but at the first look this looks over-complicated to me. We must brainstorm to find a simpler solution. This introduces impact, and is not something that I'd like to maintain (at least at the first look).

@jonathandreyer can you please describe us your use-case and requirement here in greater detail. From what I understood, you needed to redirect messages to 2 different DBs. Why could not you do it on the channel level?

I fully understand the first point.

Sure, I will try to explain the reason. The explain that, I will take as an example the subtopics in the documentation (https://mainflux.readthedocs.io/en/latest/messaging/#subtopics). In my usage, I have two channels by device (which are provided by bootstrap service) and subtopics for each kind of sended values. I would like store numerical values (related to specific topics, like temperature) in influxDB and string values in another database (mongoDB) for more general information about device, like wake-up, shutdown, etc. With this function (filtering by subtopics), I can select which information I would like to store in which database.

@drasko
Copy link
Contributor

drasko commented Mar 12, 2020

OK, this is something what I thought.

Now second question: what about having just one config file - for example subjects.toml which would list full NATS subject to which Writer should subscribe (instead of having separate channels.toml and subtopics.toml which IMHO complicate the things, especially that it is not clear to me which is a colleration with a certain channel and its subtopics).

cmd/influxdb-writer/main.go Outdated Show resolved Hide resolved
@jonathandreyer
Copy link
Contributor Author

Now second question: what about having just one config file - for example subjects.toml which would list full NATS subject to which Writer should subscribe (instead of having separate channels.toml and subtopics.toml which IMHO complicate the things, especially that it is not clear to me which is a colleration with a certain channel and its subtopics).

@drasko For me, both alternatives are ok. As discussed here (https://github.com/mainflux/mainflux/pull/1072#discussion_r391604923), @manuio think is better to keep both files for retro-compatible reason and also easier to understand. With the resolution of #925, I think the version with one file is OK.

…method

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>
Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>
@jonathandreyer
Copy link
Contributor Author

Hi @manuio & @drasko, I have merged both filters files into one file as explain before but now it has no effect. My knowledge in NATS is pretty poor to subscribe in only selected subjects (here: https://github.com/jonathandreyer/mainflux/blob/4c6576193344b82f067a25f22abf9d4ebbc680b5/writers/writer.go#L45). Have you an idea how proceed?

writers/writer.go Outdated Show resolved Hide resolved
writers/writer.go Outdated Show resolved Hide resolved
Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>
Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>
Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>
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.

@jonathandreyer this starts to look good. On the first look just a minor suggestion regarding the docs, but I will analyze with more attention tomorrow.

We will be needing approvals from several maintainers, as this touches lot of code.

docker/addons/cassandra-writer/subjects.toml Outdated Show resolved Hide resolved
writers/writer.go Outdated Show resolved Hide resolved
writers/writer.go Outdated Show resolved Hide resolved
writers/writer.go Outdated Show resolved Hide resolved
Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>
writers/writer.go Outdated Show resolved Hide resolved
Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>
manuio
manuio previously approved these changes Mar 29, 2020
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

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>
@nmarcetic
Copy link
Collaborator

LGTM!

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

@drasko drasko merged commit 46aadcf into absmach:master Mar 30, 2020
@drasko
Copy link
Contributor

drasko commented Mar 30, 2020

Merged! Thanks @jonathandreyer and @manuio

@jonathandreyer jonathandreyer deleted the feature/subtopics-filter-writer branch April 1, 2020 08:56
manuio pushed a commit that referenced this pull request Oct 12, 2020
* Add feature of filtering by subtopics in writer

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>

* Fix mistake

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>

* Refactoring writer sevices

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>

* Rename variables related to filter (channels & subtopics)

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>

* Set default value of filtering when configuration file doesn't exist

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>

* Add a blank line at the end of the file

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>

* Refactor loading filters configuration (moving into writer package, merge both loading methods & returning error)

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>

* Remove useless log

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>

* Change type of variables (channels & subtopics) and simplify loading method

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>

* Add logging error when loading filters

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>

* Simplify return configuration in loading method

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>

* Merge both filter files into one file

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>

* Move loading subjects into writer package

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>

* Add subscribe to selected subjects

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>

* Edit README of writer services

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>

* Keep only subscribe loop

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>

* Use full NATS subjects

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>

* Edit comment in subjects files

Signed-off-by: Jonathan Dreyer <jonathan.dreyer@cleanenergie.ch>

Co-authored-by: Drasko DRASKOVIC <drasko.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.

Writers - default value for channel list when toml file is not found
7 participants