-
Notifications
You must be signed in to change notification settings - Fork 672
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 - RabbitMQ build and deployment #1570
Conversation
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #1570 +/- ##
=======================================
Coverage 69.20% 69.21%
=======================================
Files 138 138
Lines 11210 11209 -1
=======================================
Hits 7758 7758
+ Misses 2765 2764 -1
Partials 687 687
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New messaging/broker
and another wrapper look unnecessary.
docker/docker-compose.yml
Outdated
@@ -305,11 +321,12 @@ services: | |||
depends_on: | |||
- things | |||
- nats | |||
- rabbitmq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding both Nats and RabbitMQ as dependencies is suboptimal. The one should choose between them, rather than always having them running together. Consider using multiple compose files (one for broker and the other for the Mainflux services), updating Makefile, or checking if there are options to do it with a single compose file.
Signed-off-by: 0x6f736f646f <[email protected]>
It aggregates the currently two brokers nats and rabbitmq based on the broker url provided |
Signed-off-by: 0x6f736f646f <[email protected]>
type ( | ||
Publisher nats.Publisher | ||
PubSub nats.PubSub | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's simplify remove these wrappers and update the base interface with Close
:
// Publisher specifies message publishing API.
type Publisher interface {
// Publishes message to the stream.
Publish(topic string, msg Message) error
// Close gracefully closes message publisher's connection.
Close() error
}
// MessageHandler represents Message handler for Subscriber.
type MessageHandler interface {
// Handle handles messages passed by underlying implementation.
Handle(msg Message) error
// Cancel is used for cleanup during unsubscribing and it's optional.
Cancel() error
}
// Subscriber specifies message subscription API.
type Subscriber interface {
// Subscribe subscribes to the message stream and consumes messages.
Subscribe(id, topic string, handler MessageHandler) error
// Unsubscribe unsubscribes from the message stream and
// stops consuming messages.
Unsubscribe(id, topic string) error
// Close gracefully closes message subscriber's connection.
Close() error
}
and use messaging.{Publisher,Subscriber,PubSub}
everwhere instead of wrapped versions (brokers.PubSub
, nats.PubSub
, mqtt.PubSub
...).
Signed-off-by: 0x6f736f646f <[email protected]>
docker/brokers/nats.yml
Outdated
volumes: | ||
- ./../nats/:/etc/nats | ||
ports: | ||
- ${MF_NATS_PORT}:${MF_NATS_PORT} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an empty line.
docker/brokers/rabbitmq.yml
Outdated
RABBITMQ_DEFAULT_VHOST: ${MF_RABBITMQ_VHOST} | ||
ports: | ||
- ${MF_RABBITMQ_PORT}:${MF_RABBITMQ_PORT} | ||
- ${MF_RABBITMQ_HTTP_PORT}:${MF_RABBITMQ_HTTP_PORT} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an empty line.
pkg/messaging/pubsub.go
Outdated
@@ -26,6 +29,9 @@ type Subscriber interface { | |||
// Unsubscribe unsubscribes from the message stream and | |||
// stops consuming messages. | |||
Unsubscribe(id, topic string) error | |||
|
|||
// Close gracefully closes message subscriber's connection. | |||
Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Close should return an error. In case the underlying implementation is not returning it, return nil.
Signed-off-by: 0x6f736f646f <[email protected]>
docker/brokers/rabbitmq.yml
Outdated
@@ -0,0 +1,11 @@ | |||
services: | |||
broker: | |||
image: rabbitmq:3.7-management |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use more recent image such as rabbitmq:3.9.20-management-alpine
.
docker/.env
Outdated
MF_NATS_URL=nats://nats:4222 | ||
# Message Broker | ||
MF_BROKER_TYPE=nats | ||
MF_BROKER_URL=nats://broker:4222 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still not addressed.
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: GitHub <[email protected]>
scripts/ci.sh
Outdated
@@ -81,6 +83,7 @@ setup() { | |||
setup_lint | |||
} | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove added empty line.
Makefile
Outdated
docker-compose -f docker/docker-compose.yml up | ||
ifeq ("$(MF_BROKER_TYPE)", "rabbitmq") | ||
sed -i "s,file: brokers/.*.yml,file: brokers/rabbitmq.yml," docker/docker-compose.yml | ||
export MF_BROKER_URL=amqp://$(MF_RABBITMQ_USER):$(MF_RABBITMQ_PASS)@broker:$(MF_RABBITMQ_PORT)$(MF_RABBITMQ_VHOST) && docker-compose -f docker/docker-compose.yml up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not work unless the user has locally set MF_RABBITMQ_*
env vars set because it will be replaced with empty values. I would suggest doing something like this instead:
sed -i "s,file: brokers/.*.yml,file: brokers/rabbitmq.yml," docker/docker-compose.yml
sed -i "s,MF_BROKER_URL: .*,MF_BROKER_URL: $$\{MF_RABBITMQ_URL\}," docker/docker-compose.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we needed to build the broker URL for instance when using rabbitmq
Signed-off-by: GitHub <[email protected]>
Signed-off-by: GitHub <[email protected]>
Makefile
Outdated
else | ||
echo "Invalid broker type"; exit 1 | ||
endif | ||
docker-compose -f docker/docker-compose.yml up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is still not resolved.
Makefile
Outdated
docker-compose -f docker/docker-compose.yml up | ||
ifeq ("$(MF_BROKER_TYPE)", "rabbitmq") | ||
sed -i "s,file: brokers/.*.yml,file: brokers/rabbitmq.yml," docker/docker-compose.yml | ||
sed -i 's#{MF_BROKER_URL}#MF_RABBITMQ_URL#' docker/docker-compose.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach is not flexible enough and will work only when you first replace ${MF_BROKER_URL}
with ${MF_RABBITMQ_URL}
. If you want to run it with any other broker later, it won't work. Since you changed docker-compose.yml
to use ${MF_NATS_URL}
, it will not work at all. I prefer using MF_BROKER_URL: ${MF_NATS_URL}
over MF_BROKER_URL: ${MF_BROKER_URL}
in docker-compose.yml
, but Makefile sed
command needs an update.
Signed-off-by: 0x6f736f646f <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're almost done.
docker/.env
Outdated
MF_RABBITMQ_PASS=mainflux | ||
MF_RABBITMQ_COOKIE=mainflux | ||
MF_RABBITMQ_VHOST=/ | ||
MF_RABBITMQ_URL=amqp://mainflux:mainflux@broker:5672/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can reuse previous values to construct the URL: MF_RABBITMQ_URL=amqp://$MF_RABBITMQ_USER:$MF_RABBITMQ_PASS@broker:$MF_RABBITMQ_PORT$MF_MF_RABBITMQ_VHOST
scripts/ci.sh
Outdated
echo "Compile check for rabbitmq..." | ||
MF_BROKER_TYPE=rabbitmq make -j $NPROC http |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put this before make -j $NPROC
: https://github.com/mainflux/mainflux/pull/1570#discussion_r901527315
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Signed-off-by: 0x6f736f646f <[email protected]>
Once this is merged we will update the docs and devops repos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What does this do?
It aggregates the different message broker deployments starting with rabbitmq
Which issue(s) does this PR fix/relate to?
Resolves #1610
List any changes that modify/break current functionality
We are modularising the backend broker from nats to either nats or rabbitmq or Kafka
Have you included tests for your changes?
No
Did you document any new/modified functionality?
Yes
Notes
N/A