NOISSUE - Create broker package for NATS #1080

manuio · 2020-03-20T07:57:35Z

Signed-off-by: Manuel Imperiale manuel.imperiale@gmail.com

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

brokers/brokers.go

brokers/nats/publish.go

brokers/nats/subscribe.go

brokers/brokers.go

brokers/nats/publish.go

brokers/nats/subscribe.go

brokers/nats/publish.go

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

coap/adapter.go

ws/adapter.go

coap/adapter.go

codecov-io · 2020-03-21T21:47:04Z

Codecov Report

Merging #1080 into master will decrease coverage by 0.11%.
The diff coverage is 65.67%.

@@            Coverage Diff             @@
##           master    #1080      +/-   ##
==========================================
- Coverage   77.21%   77.10%   -0.12%     
==========================================
  Files          95       95              
  Lines        6921     6760     -161     
==========================================
- Hits         5344     5212     -132     
+ Misses       1238     1211      -27     
+ Partials      339      337       -2

Impacted Files	Coverage Δ
readers/influxdb/messages.go	`76.92% <ø> (ø)`
ws/adapter.go	`67.34% <44.44%> (-11.45%)`	⬇️
twins/service.go	`42.23% <76.47%> (-5.22%)`	⬇️
http/api/endpoint.go	`100.00% <100.00%> (ø)`
http/api/transport.go	`74.39% <100.00%> (-0.40%)`	⬇️
transformers/senml/transformer.go	`93.10% <100.00%> (ø)`
ws/api/transport.go	`80.46% <100.00%> (-6.09%)`	⬇️
... and 1 more

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 51ec256...a8efe61. Read the comment docs.

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

cmd/coap/main.go

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

drasko

LGTM

drasko

LGTM

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

broker/broker.go

coap/adapter.go

mteodor · 2020-03-31T09:45:34Z

coap/observer.go

@@ -36,7 +36,7 @@ type Observer struct {
 // NewObserver instantiates a new Observer.
 func NewObserver() *Observer {
 	return &Observer{
-		Messages: make(chan mainflux.Message),
+		Messages: make(chan broker.Message),


maybe we expired and msgID should be in its own structure with its own mutex, and Observer can embed those structures.
Also wouldnt be better that Observer is interface?

mteodor · 2020-03-31T09:49:30Z

coap/adapter.go

 	}

+	go func() {
+		<-o.Cancel


I may be wrong but seems that wait group fits here, us if you can, it is faster.

mteodor · 2020-03-31T09:52:37Z

coap/adapter.go

+// Service specifies coap service API.
+type Service interface {
+	// Publish Messssage
+	Publish(context.Context, string, broker.Message) error


this fits nats.Broker, right? why not use embedded interface

Because it doesn't fit broker.Nats interface

cmd/http/main.go

nmarcetic · 2020-03-31T09:46:39Z

cmd/http/main.go

@@ -87,10 +80,16 @@ func main() {
 	thingsTracer, thingsCloser := initJaeger("things", cfg.jaegerURL, logger)
 	defer thingsCloser.Close()

+	pubsub, err := broker.New(cfg.natsURL)


Maybe it would be better to say nats.New(cfg.natsURL) you are making new nats conn from nats package (e.g isolated impl of nats broker) or tomorrow kafka.New(cfg.kafka) from broker/kafka package. Just an idea
Also I would prefer messenger or msg or whatever that sounds better then pubsub.Publish() for example

I agree for pubsub, I was never convinced. I simply replaced it by b, err := broker.New(cfg.natsURL).
I also propose that we think about to use an interface for all brokers or different ones per broker.
In my last commit I replaced broker/nats/pubsub.go by broker/nats.go. There is no nats package anymore. Eventually I could replace broker.New by broker.NewNats

go.mod

nmarcetic · 2020-03-31T09:57:53Z

transformers/service.go


 // Transformer specifies API form Message transformer.
 type Transformer interface {
 	// Transform Mainflux message to any other format.
-	Transform(mainflux.Message) (interface{}, error)
+	Transform(broker.Message) (interface{}, error)


Much better than mainflux.Message IMHO

nmarcetic · 2020-03-31T10:06:18Z

broker/broker.go

+// Nats specifies a NATS message API.
+type Nats interface {
+	// Publish publishes message to the msessage broker.
+	Publish(context.Context, string, Message) error


Can you pls put named params, it's easier to understand what are params. Now I see string string and dunno what it means.

I think that's what we should do for all the interfaces, starting from here.

I'm not against but in most of our services we are not doing this. Let me open an issue to fix it everywhere

nmarcetic · 2020-03-31T10:09:58Z

broker/broker.go

+const SubjectAllChannels = "channel.>"
+
+// Nats specifies a NATS message API.
+type Nats interface {


You think its better to have a separate interface for each broker that we use like nats kafka etc...? Not one Interface lets say Messenger that needs to be satisfied by broker imply for example nats.

My first idea was to have a single interface. But it's not possible unless you create a new service for this because NATS interface will never match with Kafka one (it's doable for publish but not for subscribe). For now if we want to use this as package it's better to have different interfaces .

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

dborovcanin · 2020-03-31T12:09:03Z

broker/broker.go

+// Nats specifies a NATS message API.
+type Nats interface {
+	// Publish publishes message to the msessage broker.
+	Publish(context.Context, string, Message) error


I think that's what we should do for all the interfaces, starting from here.

dborovcanin · 2020-03-31T12:09:26Z

broker/broker.go

+const SubjectAllChannels = "channel.>"
+
+// Nats specifies a NATS message API.
+type Nats interface {


dborovcanin · 2020-03-31T12:14:06Z

broker/message.pb.go

@@ -1,14 +1,15 @@
 // Code generated by protoc-gen-gogo. DO NOT EDIT.
 // source: message.proto

-package mainflux
+package broker


Please don't change this manually, but rather fix Makefile and use it to generate this code. I don't mind keeping broker.go and messages.{proto, pb.go} in the project root.

broker/nats/pubsub.go

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

twins/service.go

twins/api/http/endpoint_test.go

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

broker/nats.go

nmarcetic · 2020-04-01T16:07:29Z

broker/nats.go

+	ps := fmt.Sprintf("%s.%s", prefix, subject)
+	sub, err := b.conn.Subscribe(ps, f)
+	if err != nil {
+		return nil, err


You are not wrapping errors? Not sure if it's supposed to be wrapped everywhere or just in some places?

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

drasko

LGTM

* NOISSUEE - Create broker package for NATS Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Create funcs to return NATS connection Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * mv os.exit to main Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix Reviews Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix tests and typos Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix CI Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix reviews Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Unify Publisher and Subscriber interfaces Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Rename Nats interface Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * typo Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Mv message.pb.go, messsage.proto and topics.go to broker directory Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix go.mod Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Use mainflux broker for writers and twins services Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix go.mod Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix twins tests Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix make proto Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix message.proto Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix golangcibot Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * regenerate message.pb.go Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix comment Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix comment Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix make proto Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Add NATS errors Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

manuio requested a review from a team as a code owner March 20, 2020 07:57

drasko changed the title ~~NOISSUEE - Create broker package for NATS~~ NOISSUE - Create broker package for NATS Mar 20, 2020

NOISSUEE - Create broker package for NATS

9aad3b2

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

nmarcetic reviewed Mar 20, 2020

View reviewed changes

dborovcanin requested changes Mar 20, 2020

View reviewed changes

brokers/brokers.go Outdated Show resolved Hide resolved

brokers/brokers.go Outdated Show resolved Hide resolved

brokers/nats/publish.go Outdated Show resolved Hide resolved

brokers/nats/subscribe.go Outdated Show resolved Hide resolved

brokers/nats/publish.go Outdated Show resolved Hide resolved

nmarcetic reviewed Mar 20, 2020

View reviewed changes

brokers/nats/publish.go Outdated Show resolved Hide resolved

manuio added 4 commits March 20, 2020 13:30

Create funcs to return NATS connection

6dc7334

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

mv os.exit to main

387635b

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix Reviews

443e89f

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix tests and typos

04a05c5

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

golangcibot reviewed Mar 21, 2020

View reviewed changes

coap/adapter.go Outdated Show resolved Hide resolved

ws/adapter.go Outdated Show resolved Hide resolved

coap/adapter.go Outdated Show resolved Hide resolved

Fix CI

7a07e6e

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

mteodor reviewed Mar 24, 2020

View reviewed changes

cmd/coap/main.go Outdated Show resolved Hide resolved

manuio mentioned this pull request Mar 25, 2020

Adapters should insert timestamp in the case it is missing #1069

Closed

manuio added 5 commits March 25, 2020 09:15

Fix reviews

ed6ca0c

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix conflicts

653d3c6

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Unify Publisher and Subscriber interfaces

c37239d

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Rename Nats interface

b9cddf2

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

typo

f0924be

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

drasko previously approved these changes Mar 28, 2020

View reviewed changes

manuio dismissed drasko’s stale review via b59ca6f March 30, 2020 17:09

drasko previously approved these changes Mar 30, 2020

View reviewed changes

manuio dismissed drasko’s stale review via f0924be March 30, 2020 17:29

manuio added 2 commits March 30, 2020 20:15

Mv message.pb.go, messsage.proto and topics.go to broker directory

14c2954

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Merge and fix conflicts

c2cf246

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

mteodor reviewed Mar 31, 2020

View reviewed changes

broker/broker.go Outdated Show resolved Hide resolved

mteodor reviewed Mar 31, 2020

View reviewed changes

coap/adapter.go Show resolved Hide resolved

mteodor reviewed Mar 31, 2020

View reviewed changes

coap/adapter.go

}

go func() {

<-o.Cancel

Copy link

Contributor

mteodor Mar 31, 2020

Choose a reason for hiding this comment

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

I may be wrong but seems that wait group fits here, us if you can, it is faster.

mteodor reviewed Mar 31, 2020

View reviewed changes

nmarcetic reviewed Mar 31, 2020

View reviewed changes

Fix go.mod

cf1f2d1

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

dborovcanin requested changes Mar 31, 2020

View reviewed changes

manuio added 3 commits March 31, 2020 15:53

Use mainflux broker for writers and twins services

660eeb6

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix go.mod

2133dcc

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix twins tests

d15c3f8

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

golangcibot reviewed Mar 31, 2020

View reviewed changes

twins/service.go Outdated Show resolved Hide resolved

twins/api/http/endpoint_test.go Outdated Show resolved Hide resolved

manuio and others added 9 commits March 31, 2020 17:34

Fix make proto

1802904

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix message.proto

20d551c

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix golangcibot

ad07a0d

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

regenerate message.pb.go

17289ca

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix comment

a746a95

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Fix comment

00521d4

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Merge branch 'master' into broker

8aff074

Fix make proto

539574e

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

Merge branch 'master' into broker

61d5d9d

blokovi previously approved these changes Apr 1, 2020

View reviewed changes

nmarcetic reviewed Apr 1, 2020

View reviewed changes

Add NATS errors

a8efe61

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

manuio dismissed blokovi’s stale review via a8efe61 April 1, 2020 16:24

nmarcetic approved these changes Apr 1, 2020

View reviewed changes

drasko approved these changes Apr 1, 2020

View reviewed changes

dborovcanin approved these changes Apr 1, 2020

View reviewed changes

drasko merged commit fff492b into absmach:master Apr 1, 2020

manuio deleted the broker branch February 23, 2022 12:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NOISSUE - Create broker package for NATS #1080

NOISSUE - Create broker package for NATS #1080

manuio commented Mar 20, 2020

codecov-io commented Mar 21, 2020 •

edited

Loading

drasko left a comment

drasko left a comment

mteodor Mar 31, 2020

mteodor Mar 31, 2020

mteodor Mar 31, 2020

manuio Mar 31, 2020

nmarcetic Mar 31, 2020

manuio Mar 31, 2020

nmarcetic Mar 31, 2020

nmarcetic Mar 31, 2020

dborovcanin Mar 31, 2020

manuio Mar 31, 2020

nmarcetic Mar 31, 2020

dborovcanin Mar 31, 2020

manuio Mar 31, 2020 •

edited

Loading

dborovcanin Mar 31, 2020

dborovcanin Mar 31, 2020

dborovcanin Mar 31, 2020

nmarcetic Apr 1, 2020

drasko left a comment

NOISSUE - Create broker package for NATS #1080

NOISSUE - Create broker package for NATS #1080

Conversation

manuio commented Mar 20, 2020

codecov-io commented Mar 21, 2020 • edited Loading

Codecov Report

drasko left a 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

manuio Mar 31, 2020 • edited Loading

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

codecov-io commented Mar 21, 2020 •

edited

Loading

manuio Mar 31, 2020 •

edited

Loading