NOISSUE - Refactor messaging #1141

dborovcanin · 2020-04-23T15:57:40Z

Signed-off-by: Dušan Borovčanin [email protected]

What does this do?

This pull request reorganizes message publishers, subscribers, and the Mainflux Message structure.

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

There is no such issue.

List any changes that modify/break current functionality

Writers Start method and protocol adapters factory methods signatures are changed.

Have you included tests for your changes?

Yes, tests are updated according to code updates.

Did you document any new/modified functionality?

No external API changed, so there was no need to update docs.

manuio · 2020-04-23T16:27:14Z

http/api/transport.go

 		Protocol: protocol,
 		Channel:  chanID,
 		Subtopic: subtopic,
 		Payload:  payload,
-		Created:  created,
+		Occured:  time.Now().UnixNano(),


Why this revert?

codecov-io · 2020-04-26T12:42:57Z

Codecov Report

Merging #1141 into master will decrease coverage by 0.01%.
The diff coverage is 52.94%.

@@            Coverage Diff             @@
##           master    #1141      +/-   ##
==========================================
- Coverage   75.33%   75.32%   -0.02%     
==========================================
  Files         101      101              
  Lines        6853     6857       +4     
==========================================
+ Hits         5163     5165       +2     
- Misses       1333     1334       +1     
- Partials      357      358       +1

Impacted Files	Coverage Δ
twins/service.go	`42.38% <45.45%> (+0.14%)`	⬆️
transformers/senml/transformer.go	`81.81% <60.00%> (ø)`
http/api/transport.go	`72.61% <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 36d00d8...0ca8c60. Read the comment docs.

drasko · 2020-04-26T22:54:46Z

message.proto

@@ -12,5 +11,5 @@ message Message {
 	string                    publisher   = 3;
 	string                    protocol    = 4;
 	bytes                     payload     = 5;
-	google.protobuf.Timestamp created     = 6;
+	int64                     occured     = 6;


Needs a comment - that it represents nano

drasko · 2020-04-26T22:57:04Z

coap/api/transport.go

 		Channel:   chanID,
 		Subtopic:  subtopic,
 		Publisher: publisher,
 		Protocol:  protocol,
 		Payload:   msg.Payload,
-		Created:   created,
+		Occured:   time.Now().UnixNano(),


Not sure about this Occured... Created is very usual in these occasions, but it is true that payload could have been created before (on device). However, Creted here relates to the Mainflux Message{} entity.

What is the opinion of other @mainflux/maintainers?

drasko · 2020-04-27T14:56:41Z

cmd/cassandra-writer/main.go

@@ -69,19 +70,21 @@ func main() {
 		log.Fatalf(err.Error())
 	}

-	b, err := broker.New(cfg.natsURL)
+	nc, err := nats.Connect(cfg.natsURL)


The main question is - do we want this really? That for every new broker we put this connecting logic in every adapter? Why not solving once and for all in a shared package?

It won't change a lot of even if we do that. If we move connection logic to, for example, pubsub/nats method NewPub(..), we'll need to:

return an error alongside with the implementation

enrich returned interface with Close method, so that we can call defer(ifc.Close())

check returned error if err != nil ...

Would it be hard to have a generic full wrapper? I would prefer that then having a partial wrapping for every broker.

No, we can do that. It won't help too much, but it will mask the underlying broker specifics (nats.Connect).

drasko

Getting there... Starts to look really good!

drasko · 2020-04-27T18:49:31Z

http/api/transport.go

 		Protocol: protocol,
 		Channel:  chanID,
 		Subtopic: subtopic,
 		Payload:  payload,
-		Created:  created,
+		Occurred: occurred,


I prefer Created. Please change.

drasko · 2020-04-27T18:50:05Z

message.proto

+	bytes                     payload     = 5;
+	// Timestamp the Message occured in the system.
+	// By the default, occured represents Unix nanoseconds timestamp.
+	int64                     occurred     = 6;


We will not accept timestamp change in this PR. And it should be created.

drasko · 2020-04-27T18:50:17Z

messaging/message.proto

@@ -12,5 +12,5 @@ message Message {
 	string                    publisher   = 3;
 	string                    protocol    = 4;
 	bytes                     payload     = 5;
-	google.protobuf.Timestamp created     = 6;
+	google.protobuf.Timestamp occurred     = 6;


manuio · 2020-04-27T19:18:29Z

cmd/coap/main.go

 	thingsapi "github.com/mainflux/mainflux/things/api/auth/grpc"
+	broker "github.com/nats-io/nats.go"


I would remove the alias here

manuio · 2020-04-27T19:23:16Z

cmd/twins/main.go

-			return
-		}
-
+	err := ps.Subscribe("channels.>", func(msg messaging.Message) error {


Can we keep here SubjectAllChannels?

manuio · 2020-04-27T19:24:15Z

cmd/cassandra-writer/main.go

 	"github.com/mainflux/mainflux/logger"
+	pubsub "github.com/mainflux/mainflux/messaging/nats"


I would use another alias. messaging or messengerNats

manuio · 2020-04-27T19:24:26Z

cmd/coap/main.go

 	"github.com/mainflux/mainflux/coap"
 	"github.com/mainflux/mainflux/coap/api"
 	logger "github.com/mainflux/mainflux/logger"
+	pubsub "github.com/mainflux/mainflux/messaging/nats"


manuio · 2020-04-27T19:24:33Z

cmd/http/main.go

 	adapter "github.com/mainflux/mainflux/http"
 	"github.com/mainflux/mainflux/http/api"
 	"github.com/mainflux/mainflux/logger"
+	pubsub "github.com/mainflux/mainflux/messaging/nats"


manuio · 2020-04-27T19:24:42Z

cmd/influxdb-writer/main.go

 	"github.com/mainflux/mainflux/logger"
+	pubsub "github.com/mainflux/mainflux/messaging/nats"


manuio · 2020-04-27T19:25:01Z

cmd/mongodb-writer/main.go

 	"github.com/mainflux/mainflux/logger"
+	pubsub "github.com/mainflux/mainflux/messaging/nats"


manuio · 2020-04-27T19:25:11Z

cmd/mqtt/main.go

 	"github.com/mainflux/mainflux/logger"
+	"github.com/mainflux/mainflux/messaging"
+	pubsub "github.com/mainflux/mainflux/messaging/nats"


I renamed to messaging, but here I need both messaging and messaging/nats. Same for Twins service.

Why not then calling it nats.NewPublisher - it will be clearer from the code that this is a specific NATS-based implementation of our interface.

TBH, actually the best would be probably something like messagingNats.NewPublisher, to distinguish from ordinary NATS package and not to be confused with functions from NATS driver.

I can accept pusub alias as well, just a proposition.

I removed named import by addressing this one: https://github.com/mainflux/mainflux/pull/1141#discussion_r415905298.

manuio · 2020-04-27T19:25:25Z

cmd/opcua/main.go

 	"github.com/mainflux/mainflux/logger"
+	pubsub "github.com/mainflux/mainflux/messaging/nats"


manuio · 2020-04-27T19:25:32Z

cmd/postgres-writer/main.go

 	"github.com/mainflux/mainflux/logger"
+	pubsub "github.com/mainflux/mainflux/messaging/nats"


manuio · 2020-04-27T19:25:41Z

cmd/twins/main.go

 	"github.com/mainflux/mainflux/logger"
+	"github.com/mainflux/mainflux/messaging"
+	pubsub "github.com/mainflux/mainflux/messaging/nats"


Answered here: https://github.com/mainflux/mainflux/pull/1141#discussion_r416187236.

drasko

Neatpicking ;)

drasko · 2020-04-27T20:16:36Z

coap/api/transport.go

@@ -225,21 +225,21 @@ func receive(svc coap.Service, msg *gocoap.Message) *gocoap.Message {
 		return res
 	}

-	created, err := ptypes.TimestampProto(time.Now())
+	occured, err := ptypes.TimestampProto(time.Now())


I would prefer calling this var timestamp or just t. Mixing "occured" and "created" is confusing. Here you are just taking a simple timestamp recording, so t would fit nicely.

I renamed back to created. We call it like that in all the other services.

manuio · 2020-04-27T20:31:36Z

Neatpicking

Nitpicking :)
And yes, but pub := pubsub.NewPublisher it's hard to digest

drasko · 2020-04-27T20:41:34Z

This branch needs update as well

Signed-off-by: Dušan Borovčanin <[email protected]>

Package `broker` is conceptually renamed to package `nats`. Signed-off-by: Dušan Borovčanin <[email protected]>

Signed-off-by: Dušan Borovčanin <[email protected]>

drasko · 2020-04-27T22:30:07Z

cmd/mqtt/main.go

 	"github.com/mainflux/mainflux/logger"
+	"github.com/mainflux/mainflux/messaging"
+	pubsub "github.com/mainflux/mainflux/messaging/nats"


Why not then calling it nats.NewPublisher - it will be clearer from the code that this is a specific NATS-based implementation of our interface.

Signed-off-by: Dušan Borovčanin <[email protected]>

manuio · 2020-04-28T00:14:03Z

messaging/nats/pubsub.go

+			return
+		}
+		if err := h(msg); err != nil {
+			ps.logger.Warn(fmt.Sprintf("Failed handle Mainflux message: %s", err))


Failed to handle

drasko

Looks great now. Approved!

Signed-off-by: Dušan Borovčanin <[email protected]>

manuio

LGTM

drasko

LGTM

drasko · 2020-04-28T09:02:51Z

Merged! Great PR, thanks a lot @dusanb94!

* Refactor messaging Signed-off-by: Dušan Borovčanin <[email protected]> * Rename SubscribeHandler to MessageHandler Signed-off-by: Dušan Borovčanin <[email protected]> * Remove `Auth` event logs Signed-off-by: Dušan Borovčanin <[email protected]> * Update message pubsub APi Signed-off-by: Dušan Borovčanin <[email protected]> * Fix topics handling Signed-off-by: Dušan Borovčanin <[email protected]> * Update CoAP adapter Signed-off-by: Dušan Borovčanin <[email protected]> * Update Twins service Signed-off-by: Dušan Borovčanin <[email protected]> * Update LoRa adapter Signed-off-by: Dušan Borovčanin <[email protected]> * Update OPC UA adapter Signed-off-by: Dušan Borovčanin <[email protected]> * Remove broker package Package `broker` is conceptually renamed to package `nats`. Signed-off-by: Dušan Borovčanin <[email protected]> * Update makefile Signed-off-by: Dušan Borovčanin <[email protected]> * Add comment explanation Signed-off-by: Dušan Borovčanin <[email protected]> * Fix MQTT adapter Signed-off-by: Dušan Borovčanin <[email protected]> * Fix typo Signed-off-by: Dušan Borovčanin <[email protected]> * Move NATS pub/sub implementation to pubsub pkg Signed-off-by: Dušan Borovčanin <[email protected]> * Remove an empty line in main methods Signed-off-by: Dušan Borovčanin <[email protected]> * Move messaging-related code to messaging package Signed-off-by: Dušan Borovčanin <[email protected]> * Fix Twins mocks Signed-off-by: Dušan Borovčanin <[email protected]> * Change Occurred back to Created Signed-off-by: Dušan Borovčanin <[email protected]> * Fix tranformer test Signed-off-by: Dušan Borovčanin <[email protected]> * Fix message proto commands Signed-off-by: Dušan Borovčanin <[email protected]> * Replace string literal with constant Signed-off-by: Dušan Borovčanin <[email protected]> * Remove alias from main method Signed-off-by: Dušan Borovčanin <[email protected]> * Change messaging pubsub alias Signed-off-by: Dušan Borovčanin <[email protected]> * Rename occured to created Signed-off-by: Dušan Borovčanin <[email protected]> * Handle NATS connection in the NATS PubSub Signed-off-by: Dušan Borovčanin <[email protected]> * Rename n to pub/pubSub Signed-off-by: Dušan Borovčanin <[email protected]> * Fix typos Signed-off-by: Dušan Borovčanin <[email protected]>

dborovcanin added the work-in-progress label Apr 23, 2020

dborovcanin requested a review from a team as a code owner April 23, 2020 15:57

manuio suggested changes Apr 23, 2020

View reviewed changes

dborovcanin removed the work-in-progress label Apr 26, 2020

drasko requested changes Apr 26, 2020

View reviewed changes

nmarcetic previously approved these changes Apr 27, 2020

View reviewed changes

dborovcanin dismissed nmarcetic’s stale review via c5238ca April 27, 2020 14:48

drasko requested changes Apr 27, 2020

View reviewed changes

dborovcanin force-pushed the messaging branch from 6e3c5ab to 222b284 Compare April 27, 2020 18:40

drasko requested changes Apr 27, 2020

View reviewed changes

manuio suggested changes Apr 27, 2020

View reviewed changes

drasko requested changes Apr 27, 2020

View reviewed changes

dborovcanin added 14 commits April 28, 2020 00:21

Refactor messaging

301c4f7

Signed-off-by: Dušan Borovčanin <[email protected]>

Rename SubscribeHandler to MessageHandler

20e619d

Signed-off-by: Dušan Borovčanin <[email protected]>

Remove Auth event logs

511f620

Signed-off-by: Dušan Borovčanin <[email protected]>

Update message pubsub APi

f64bce7

Signed-off-by: Dušan Borovčanin <[email protected]>

Fix topics handling

ecd5f73

Signed-off-by: Dušan Borovčanin <[email protected]>

Update CoAP adapter

62af5ac

Signed-off-by: Dušan Borovčanin <[email protected]>

Update Twins service

4d806f4

Signed-off-by: Dušan Borovčanin <[email protected]>

Update LoRa adapter

b599cc0

Signed-off-by: Dušan Borovčanin <[email protected]>

Update OPC UA adapter

e4cd0c0

Signed-off-by: Dušan Borovčanin <[email protected]>

Remove broker package

f469a83

Package `broker` is conceptually renamed to package `nats`. Signed-off-by: Dušan Borovčanin <[email protected]>

Update makefile

bd5bec3

Signed-off-by: Dušan Borovčanin <[email protected]>

Add comment explanation

244a27b

Signed-off-by: Dušan Borovčanin <[email protected]>

Fix MQTT adapter

a55fff5

Signed-off-by: Dušan Borovčanin <[email protected]>

Fix typo

ccd15f3

Signed-off-by: Dušan Borovčanin <[email protected]>

dborovcanin added 11 commits April 28, 2020 00:21

Move NATS pub/sub implementation to pubsub pkg

a88c9cb

Signed-off-by: Dušan Borovčanin <[email protected]>

Remove an empty line in main methods

45b1407

Signed-off-by: Dušan Borovčanin <[email protected]>

Move messaging-related code to messaging package

49a0b66

Signed-off-by: Dušan Borovčanin <[email protected]>

Fix Twins mocks

1c50f8c

Signed-off-by: Dušan Borovčanin <[email protected]>

Change Occurred back to Created

bd296d1

Signed-off-by: Dušan Borovčanin <[email protected]>

Fix tranformer test

b19d0f3

Signed-off-by: Dušan Borovčanin <[email protected]>

Fix message proto commands

f54047f

Signed-off-by: Dušan Borovčanin <[email protected]>

Replace string literal with constant

7f1e76a

Signed-off-by: Dušan Borovčanin <[email protected]>

Remove alias from main method

09165b7

Signed-off-by: Dušan Borovčanin <[email protected]>

Change messaging pubsub alias

408c267

Signed-off-by: Dušan Borovčanin <[email protected]>

Rename occured to created

d22e451

Signed-off-by: Dušan Borovčanin <[email protected]>

dborovcanin force-pushed the messaging branch from 22193d1 to d22e451 Compare April 27, 2020 22:22

drasko requested changes Apr 27, 2020

View reviewed changes

dborovcanin added 2 commits April 28, 2020 00:53

Handle NATS connection in the NATS PubSub

ae76b43

Signed-off-by: Dušan Borovčanin <[email protected]>

Rename n to pub/pubSub

0ca8c60

Signed-off-by: Dušan Borovčanin <[email protected]>

manuio suggested changes Apr 28, 2020

View reviewed changes

drasko previously approved these changes Apr 28, 2020

View reviewed changes

dborovcanin dismissed drasko’s stale review via c73fb47 April 28, 2020 08:52

Fix typos

c73fb47

Signed-off-by: Dušan Borovčanin <[email protected]>

manuio approved these changes Apr 28, 2020

View reviewed changes

drasko approved these changes Apr 28, 2020

View reviewed changes

drasko merged commit c3019ff into absmach:master Apr 28, 2020

dborovcanin deleted the messaging branch April 28, 2020 09:22

		thingsapi "github.com/mainflux/mainflux/things/api/auth/grpc"
		broker "github.com/nats-io/nats.go"

		"github.com/mainflux/mainflux/logger"
		pubsub "github.com/mainflux/mainflux/messaging/nats"

NOISSUE - Refactor messaging #1141

NOISSUE - Refactor messaging #1141

Conversation

dborovcanin commented Apr 23, 2020

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?

Choose a reason for hiding this comment

codecov-io commented Apr 26, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuio commented Apr 27, 2020 • edited Loading

drasko commented Apr 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

manuio left a comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

drasko commented Apr 28, 2020

codecov-io commented Apr 26, 2020 •

edited

Loading

manuio commented Apr 27, 2020 •

edited

Loading