MF-780 - Use Normalizer as a lib #915

dborovcanin · 2019-10-28T16:36:11Z

Signed-off-by: Dušan Borovčanin dusan.borovcanin@mainflux.com

What does this do?

This pull request removes Normalizer service in favor of using normalizer as a lib on the message consumer side.

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

Resolves #780.

List any changes that modify/break current functionality

Message consumers (writers) are modified to listen to all the message topics and normalize and persist messages.

Have you included tests for your changes?

Yes.

Did you document any new/modified functionality?

Yes.

codecov-io · 2019-10-28T16:43:30Z

Codecov Report

Merging #915 into master will increase coverage by 0.42%.
The diff coverage is 84.7%.

@@            Coverage Diff             @@
##           master     #915      +/-   ##
==========================================
+ Coverage   83.16%   83.59%   +0.42%     
==========================================
  Files          75       75              
  Lines        5484     5341     -143     
==========================================
- Hits         4561     4465      -96     
+ Misses        652      610      -42     
+ Partials      271      266       -5

Impacted Files	Coverage Δ
writers/mongodb/messages.go	`100% <100%> (ø)`	⬆️
writers/postgres/messages.go	`79.68% <35.71%> (-1.98%)`	⬇️
writers/influxdb/messages.go	`90.9% <84.61%> (+4.99%)`	⬆️
writers/cassandra/messages.go	`94.11% <92.59%> (-5.89%)`	⬇️
users/users.go	`68.42% <0%> (-4.66%)`	⬇️
users/api/http/requests.go	`61.53% <0%> (-2.75%)`	⬇️
users/api/http/transport.go	`82.08% <0%> (-2.71%)`	⬇️
users/api/http/responses.go	`66.66% <0%> (ø)`	⬆️
users/api/http/endpoint.go	`86.2% <0%> (+0.87%)`	⬆️
users/service.go	`28.57% <0%> (+1.54%)`	⬆️

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 380af87...1c76adb. Read the comment docs.

chombium · 2019-10-29T08:27:12Z

@dusanb94 Don't forget to add documentation with description of the library and it's api. This would be helpful for the others who will write custom components/writers in some other programming languages ;)

drasko

I scanned through the PR. Looks like we are really on good track here - I like the simplification!

anovakovic01 · 2019-10-30T09:16:01Z

topics.go

@@ -7,3 +7,4 @@ package mainflux

 // OutputSenML represents subject SenML messages will be published to.
 const OutputSenML = "out.senml"
+const OutputChannels = "channel.>"


I don't think that you can publish using the wild card as part of the topic.

anovakovic01 · 2019-10-30T09:17:37Z

writers/cassandra/messages.go

@@ -20,37 +20,44 @@ func New(session *gocql.Session) writers.MessageRepository {
 	return &cassandraRepository{session}
 }

-func (cr *cassandraRepository) Save(msg mainflux.Message) error {
+func (cr *cassandraRepository) Save(messages []mainflux.Message) error {


Think about updating the interface to have a variadic function.

anovakovic01 · 2019-10-30T09:24:39Z

internal/normalizer/normalizer.go

@@ -0,0 +1,64 @@
+// Copyright (c) Mainflux


I think that the normalizer shouldn't be in the internal package. AFAIK, everything that's in the internal cannot be used from the outside and as far as I understood, we want this lib to be usable from the outside too. At least think about creating an interface for the normalizer. Maybe this interface should have:

Normalize(msg mainflux.RawMessage) ([]interface{}, error)

You'll later have to cast this interface{} but that's part of the language (if it had generics maybe we could have done something about it). This way you'll be able to create an interface for any other message type.

To normalize messages on the consumer side, Normalizer is moved to the internal pkgs. Writers being message consumers are modified to do message normalization instead of subscribing to normalized messages subject. Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

As we agreed on absmach#919, we'll use normalizer as an interface and provide the default SenML implementation. Because of that, Normalizer is removed from `internal` and we'll use the project structure proposed in the aforementioned issue. Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Move Normalizer service to `addons`. Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

manuio · 2019-10-30T19:36:44Z

readers/api/logging.go

 	defer func(begin time.Time) {
-		lm.logger.Info(fmt.Sprintf(`Method read_all for offset %d and limit %d took %s to complete without errors.`, offset, limit, time.Since(begin)))
+		message := fmt.Sprintf("Method read_all for offset %d and limit %d took %s to complete", offset, limit, time.Since(begin))


You could log the Channel ID here

drasko · 2019-10-31T10:24:19Z

docker/addons/normalizer/docker-compose.yml

@@ -0,0 +1,28 @@
+


Add Mainflux header insted of this blank line at the top.

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Makefile

topics.go

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

drasko

LGTM

Need to move on

* Use Normalizer as a lib To normalize messages on the consumer side, Normalizer is moved to the internal pkgs. Writers being message consumers are modified to do message normalization instead of subscribing to normalized messages subject. Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Fix logging middleware for readers and writers Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Remove normalizer interface Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Use Normalizer in writers As we agreed on #919, we'll use normalizer as an interface and provide the default SenML implementation. Because of that, Normalizer is removed from `internal` and we'll use the project structure proposed in the aforementioned issue. Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Fix tests Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Remove unused batch settings from influxDB reader Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Update docs Move Normalizer service to `addons`. Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Rename channels input topic Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Update Noramlizer docs Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Remove commented code Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Update readers logging Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Update addons docker-compose files Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Update topcis explanations Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

dborovcanin added the work-in-progress label Oct 28, 2019

dborovcanin self-assigned this Oct 28, 2019

dborovcanin force-pushed the MF-780 branch 3 times, most recently from 4c889e9 to a53720b Compare October 29, 2019 12:48

dborovcanin mentioned this pull request Oct 29, 2019

Consider Mainflux message updates #919

Closed

drasko reviewed Oct 30, 2019

View reviewed changes

anovakovic01 previously requested changes Oct 30, 2019

View reviewed changes

dborovcanin added 4 commits October 30, 2019 14:24

Fix logging middleware for readers and writers

55cf450

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Remove normalizer interface

6a5c980

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

dborovcanin force-pushed the MF-780 branch from a003564 to 471f8cc Compare October 30, 2019 13:25

dborovcanin added 5 commits October 30, 2019 14:47

Fix tests

d702146

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Remove unused batch settings from influxDB reader

f0f034b

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Update docs

5ac76c2

Move Normalizer service to `addons`. Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Rename channels input topic

6d84c32

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Update Noramlizer docs

644bd13

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

dborovcanin removed the work-in-progress label Oct 30, 2019

dborovcanin marked this pull request as ready for review October 30, 2019 15:55

dborovcanin requested a review from a team as a code owner October 30, 2019 15:55

Remove commented code

c112f5f

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

manuio reviewed Oct 30, 2019

View reviewed changes

drasko requested changes Oct 31, 2019

View reviewed changes

dborovcanin added 2 commits October 31, 2019 11:57

Update readers logging

13c07c7

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Update addons docker-compose files

c7c10c5

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

nmarcetic reviewed Oct 31, 2019

View reviewed changes

Makefile Show resolved Hide resolved

nmarcetic reviewed Oct 31, 2019

View reviewed changes

topics.go Show resolved Hide resolved

nmarcetic previously approved these changes Oct 31, 2019

View reviewed changes

Update topcis explanations

1c76adb

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

dborovcanin dismissed nmarcetic’s stale review via 1c76adb October 31, 2019 12:48

nmarcetic approved these changes Oct 31, 2019

View reviewed changes

drasko approved these changes Oct 31, 2019

View reviewed changes

drasko merged commit 8be2516 into absmach:master Oct 31, 2019

dborovcanin deleted the MF-780 branch December 18, 2019 10:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MF-780 - Use Normalizer as a lib #915

MF-780 - Use Normalizer as a lib #915

dborovcanin commented Oct 28, 2019

codecov-io commented Oct 28, 2019 •

edited

Loading

chombium commented Oct 29, 2019

drasko left a comment

anovakovic01 Oct 30, 2019

anovakovic01 Oct 30, 2019

anovakovic01 Oct 30, 2019

manuio Oct 30, 2019

drasko Oct 31, 2019

drasko left a comment

MF-780 - Use Normalizer as a lib #915

MF-780 - Use Normalizer as a lib #915

Conversation

dborovcanin commented Oct 28, 2019

What does this do?

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

List any changes that modify/break current functionality

Have you included tests for your changes?

Did you document any new/modified functionality?

codecov-io commented Oct 28, 2019 • edited Loading

Codecov Report

chombium commented Oct 29, 2019

drasko left a comment

Choose a reason for hiding this comment

anovakovic01 Oct 30, 2019

Choose a reason for hiding this comment

anovakovic01 Oct 30, 2019

Choose a reason for hiding this comment

anovakovic01 Oct 30, 2019

Choose a reason for hiding this comment

manuio Oct 30, 2019

Choose a reason for hiding this comment

drasko Oct 31, 2019

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 28, 2019 •

edited

Loading