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

[GSoC 2019] RabbitMQ architecture for notification system #382

Merged
merged 1 commit into from Jul 5, 2019

Conversation

@aquibbaig
Copy link
Collaborator

aquibbaig commented Jun 26, 2019

Aimed at fixing #373 #374 currently. This will be a WIP for now.

@aquibbaig aquibbaig added the gsoc label Jun 26, 2019
@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Jun 26, 2019

Thanks @aquibbaig for reducing the scope of this PR. It looks mostly good, but let's start in even smaller steps by only doing the RabbitMQ architectural stuff for now.

  • Please remove the commits " create new Notification entity " and " Add Web Notifications " from the PR (use git rebase -i and remove the two commits, then force-push to this branch). Make sure you have your full work history in another branch.
  • This needs a rebase first on top of the current master. So please do a
    git fetch origin (or whatever the remote name of the upstream repository is) and then do a git rebase origin/master (again, replace the remote name)
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jun 26, 2019

@imphil

  • I think the branch is fine. The git log returns 3 commits on top of master even. Also, rebasing is not adding any new changes. We have 623 commits on master and 626 on this branch. So, it shouldn't be a problem
  • Are you sure I should remove the first commit? I believe the first commit is needed for the architecture interface that we are building?
@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Jun 26, 2019

It's fine indeed, no idea what problem github has here. The comment about removing all bug the rabbitmq commit still holds, however.

@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch 4 times, most recently from a9ffb3b to 6fb1420 Jun 26, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jul 2, 2019

@imphil just force pushed the branch reflecting required changes

@aquibbaig aquibbaig requested review from agathver and imphil Jul 2, 2019
@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch from 6fb1420 to b5697fc Jul 2, 2019
Copy link
Contributor

imphil left a comment

Thanks @aquibbaig for the update to this PR. I've left a couple comments inline.

On the commit structure:

  • Please modify the commits to have a single commit where you add all the rabbitmq configuration/classes, and the Notification(Hydrator) class. So that's essentially combining the two commits you now have into one.
  • Separate the demo use case of adding a message to the queue from the remaining infrastructure. so the changes to ProjectController.php should be a separate commit.

The composer.lock file changes should not be part of this PR at all.

@@ -19,6 +19,9 @@ old_sound_rabbit_mq:
update_project_info:
connection: default
exchange_options: {name: 'update-project-info', type: fanout}
send_notification:
connection: default
exchange_options: {name: 'send-notification', type: fanout}

This comment has been minimized.

Copy link
@imphil

imphil Jul 3, 2019

Contributor

how about notification instead of send-notification?

web_notification:
connection: default
exchange_options: {name: 'send-notification', type: fanout}
queue_options: {name: 'send-notification'}

This comment has been minimized.

Copy link
@imphil

imphil Jul 3, 2019

Contributor

We need a setup where we have one queue per consumer, as in this picture:
image

In the tutorial at https://www.rabbitmq.com/tutorials/tutorial-three-python.html they're using an empty queue name to generate two unique queue names. You're using the same queue name for both consumers. Please double-check that that leads to the right queue structure.

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Jul 3, 2019

Author Collaborator

The message currently goes through the following areas

  • An Exchange first
  • Queue next
  • Both the consumers where shouldHandle() checks if it will be handled by the one it is in

But I guess we want a structure where it goes through

  • An Exchange first
  • The Queue of the specific Consumer (two queues now)
  • To the specific Consumer where a shouldHandle() will confirm that it is handled indeed by this Consumer

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Jul 3, 2019

Author Collaborator

I'll be working on this today itself

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Jul 3, 2019

Author Collaborator

@imphil @agathver
So, I have come up with two promising architecture designs. I believe we need very small attention to detail here. I found there is another way here(it may be unnecessary).

A Direct Exchange architecture

direct

Advantages

  • Less message load in the buffer

A fanout Exchange architecture

Fan

Advantages

  • No routing key required as it broadcasts messages to both queues, simpler
  • No logic required beforehand to generate a routing key

This comment has been minimized.

Copy link
@imphil

imphil Jul 3, 2019

Contributor

We want to go for the "fanout" architecture as inserting a message should be fast (it's in the synchronous page rendering path in many cases).

connection: default
exchange_options: {name: 'send-notification', type: fanout}
queue_options: {name: 'send-notification'}
callback: email_notification_consumer

This comment has been minimized.

Copy link
@imphil

imphil Jul 3, 2019

Contributor

Isn't that callback the default and can be omitted?

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Jul 4, 2019

Author Collaborator

Just checked that it expects a callback to be there else an error is thrown

@@ -94,3 +97,6 @@ services:
App\RepoCrawler\GitRepoCrawler:
tags:
- 'app.repo_crawler'

# Register Mgilet NotificationManager as a service to be used in WebNotificationConsumer
Mgilet\NotificationBundle\Manager\NotificationManager: '@mgilet.notification'

This comment has been minimized.

Copy link
@imphil

imphil Jul 3, 2019

Contributor

We discussed this before: This commit is titled " setup Rabbit MQ architecture with respective consumers ". the mgilet bundle is not needed here, and should not appear anywhere in this commit.

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Jul 3, 2019

Author Collaborator

This is a mistake I overlooked accidentally

* AbstractNotificationConsumer constructor.
* @param UserManagerInterface $userManager
*/
public function __construct(UserManagerInterface $userManager)

This comment has been minimized.

Copy link
@imphil

imphil Jul 3, 2019

Contributor

Why is the UserManagerInterface part of this interface?

*
* Class NotificationHydrator
*/
class NotificationHydrator

This comment has been minimized.

Copy link
@imphil

imphil Jul 3, 2019

Contributor

NotificationHydrator is a strange name. This is just a plain old data class (POD), so how about Notification?

/**
* @var string $link
*/
protected $link;

This comment has been minimized.

Copy link
@imphil

imphil Jul 3, 2019

Contributor

What's that for?

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Jul 4, 2019

Author Collaborator

Actually, Mgilet provides us with a link field by default which is nullable. So, we can either add/remove this field, later on, based on our usage

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

So let's do it the other way around: remove the link field in our Notification(Hydrator) class, as I don't see any use for it. If mgilet uses it that's fine, we can always set it to null.

/**
* @var int $userIdentifier
*/
protected $userIdentifier;

This comment has been minimized.

Copy link
@imphil

imphil Jul 3, 2019

Contributor

Is this the sender or the recipient? And it should be of type User; you can serialize that into a user id when sending.

* @var int $userIdentifier
*/
protected $userIdentifier;

This comment has been minimized.

Copy link
@imphil

imphil Jul 3, 2019

Contributor

This class is missing a bit of meta data: send timestamp, author

*
* @author Aquib Baig <aquibbaig97@gmail.com>
*/
class NotificationProducerService

This comment has been minimized.

Copy link
@imphil

imphil Jul 3, 2019

Contributor

This class seems to be modeled after QueueDispatcherService, which isn't actually a particularly good design and needs to be fixed now that we have more than one producer service.

Please remove this class, you can inject the auto-generated service (i.e. the one you'd pass here as $notificationProducer) directly into the class method/class that needs to produce messages. (ProjectController.php in this case). Please don't use the ProducerInterface, but the actual class -- all producers conform to this interface, and autowire cannot really figure out which service class is the right one to bind now that there are two options.

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Jul 4, 2019

Author Collaborator

So, while publishing a message, do we need to get the producer just as
this->get('@old_sound_rabbit_mq.notification_producer')->publish($msg) ?

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

You can inject the NotificationProducerService instead of using $this->get()

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Jul 4, 2019

Author Collaborator

That's better

@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch 7 times, most recently from cb7c179 to 63a3fca Jul 4, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jul 4, 2019

@imphil Just made the changes as discussed in yesterday's meeting. Added a few comments too, sorry for the mess the last time. I hope it's fine now. If not, please let me know

Copy link
Contributor

imphil left a comment

Thanks @aquibbaig. Looks pretty good now, some small fixes and we're there.

public function execute(AMQPMessage $msg)
{
try {
$notification = unserialize($msg->body);

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

You can make $notification a member variable ($this->notification), and remove the argument from shouldHandle and handle()

try {
$notification = unserialize($msg->body);
return $this->handle($notification);

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

You need to call shouldHandle() as well.

*
* @return bool
*/
abstract protected function shouldHandle(Notification $notification): bool;

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

You can always return true in the abstract implementation, it's a sane default.

use Psr\Log\LoggerInterface;
/**
* Class EmailNotificationConsumer

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

Can you make this summary line more descriptive, e.g. "Send out notifications over email"

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

That comment needs resolving in a updated push.

*/
protected function handle(Notification $notification)
{
if ($this->shouldHandle($notification)) {

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

This should go into the execute() function (see the comment there)

*
* @var User $user
*/
protected $user;

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

naming: user -> recipient

*
* @var string author
*/
protected $author;

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

Hm, thinking about it, in almost all cases our messages come from the "system", so there's actually no sender/author. You can remove this field.

/**
* Notification constructor.
* @throws \Exception

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

does it?

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Jul 4, 2019

Author Collaborator

https://php.net/manual/en/datetime.construct.php
It emits an exception in case of error, if it contains an invalid format, but we sure don't need to throw it. As we do not set datetimes explicitely

@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch from 63a3fca to 30d8eff Jul 4, 2019
* @param LoggerInterface $logger
* @param Notification $notification
*/
public function __construct(LoggerInterface $logger, Notification $notification)

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

Please remove Notification from the constructor, it's not injected or passed there somehow: it's set in the execute() function.

class EmailNotificationConsumer extends AbstractNotificationConsumer
{
/**
* EmailNotificationConsumer constructor.

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

Add a empty line after this one.

use Psr\Log\LoggerInterface;
/**
* Class EmailNotificationConsumer

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

That comment needs resolving in a updated push.

protected $message;
/**
* User associated with the Notification

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

Please change this comment to match the variable (it's the recipient of the notification)

protected $subject;
/**
* Actual definition of the Notification

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

Actual definition -> Notification message

}
/**
* Returns User associated with a Notification

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

"Get the recipient of this notification"

}
/**
* Used to bind users to any Notification

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

"Set the recipient of this message"

}
/**
* Gets the timestamp of a Notification

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

"Get the creation date of this notification"

}
/**
* Sets the timestamp of a Notification

This comment has been minimized.

Copy link
@imphil

imphil Jul 4, 2019

Contributor

do something equivalent to the get... description for this field.

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Jul 4, 2019

Author Collaborator

@imphil Yes. Just did that

Currently creates two consumers for email and web notifications
@aquibbaig aquibbaig force-pushed the aquibbaig:notifications branch from 30d8eff to 746808c Jul 4, 2019
@imphil
imphil approved these changes Jul 5, 2019
@imphil imphil marked this pull request as ready for review Jul 5, 2019
@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Jul 5, 2019

Thanks @aquibbaig, this now looks good! Merged & ready for new things!

@imphil imphil merged commit c063c54 into librecores:master Jul 5, 2019
@aquibbaig aquibbaig changed the title [GSoC 2019] Implementation of a Notification System for Librecores [GSoC 2019] RabbitMQ architecture for notification system Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.