Notification filtering #278

georgifarashev · 2019-05-17T16:25:04Z

Notification filtering

Motivation

Some notifications might not be necessary for some platforms.
For example if a broker must not be registered in a specific platform then a filter can be introduced on /v1/service_brokers which filters the broker. But the creation of the broker will create a notification for all platforms. A way of filtering this notification must be introduced.

Approach

Introduce notification filters - functions which filter the recipients of a notification.

Pull Request status

Implementation
Tests

coveralls · 2019-05-17T16:54:57Z

Coverage decreased (-0.05%) to 88.735% when pulling 2380091 on notification-filtering into 098d2e6 on master.

dpanayotov

This is very confusing implementation that doesn't make sense without a lot of additional comments. I'd try a different approach to filtering because this implementation will be unmaintainable in a month. Maybe inverse the logic.

dpanayotov · 2019-05-21T07:02:17Z

api/notifications/notifications.go

@@ -58,9 +48,18 @@ func (c *Controller) handleWS(req *web.Request) (*web.Response, error) {
 		return util.NewJSONResponse(http.StatusGone, nil)
 	}

-	notificationQueue, lastKnownRevision, notificationsList, err := c.registerConsumer(ctx, revisionKnownToProxy, user)
+	user, ok := web.UserFromContext(req.Context())
+	if !ok {


why move the user check down here?

It is not used above. I have moved it to the place where it is needed for a first time.

dpanayotov · 2019-05-21T07:10:34Z

pkg/util/errors.go

@@ -100,6 +100,9 @@ var (

 	// ErrConcurrentResourceModification error returned when concurrent resource updates are happening
 	ErrConcurrentResourceModification = errors.New("another resource update happened concurrently. Please reattempt the update")
+
+	// ErrInvalidNotificationRevision provided notification revision is not valid, must return http status GONE
+	ErrInvalidNotificationRevision = errors.New("notification revision is not valid")


Minor - I'd rather remove all these util.Err* because they are not util errors. One option is to move all errors and their handling in an errors package or move every error in its respective package (e.g. storage.ErrConcurrentModification, notifications.ErrInvalidRevision)

I have created an issue for that - #281 as it could be done in a different PR.

dpanayotov · 2019-05-21T07:21:02Z

storage/postgres/notification_storage.go

@@ -30,6 +36,10 @@ import (
 type notificationStorage interface {


I know that's not part of this PR, but this smells of bad design... a private interface for the Postgres storage only to make the tests work

I agree. This interface will be removed when the storage supports sorting by a specific property and limit of returned results (in sql - order by and limit).

dpanayotov · 2019-05-21T07:36:29Z

storage/interfaces.go

 }
+
+// NotificationFilterFunc filters recipients for a given notifications
+type NotificationFilterFunc func(recipients []*types.Platform, notification *types.Notification) (filteredRecipients []*types.Platform)


maybe ReceiversFilterFunc

dpanayotov · 2019-05-21T07:48:14Z

storage/postgres/notificator.go

+}
+
+func (n *Notificator) replaceQueueWithMissingNotificationsQueue(queue storage.NotificationQueue, lastKnownRevision, lastKnownRevisionToSM int64, platform *types.Platform) (storage.NotificationQueue, error) {
+	if lastKnownRevision > lastKnownRevisionToSM {


move this check outside as it's unrelated to this function

dpanayotov · 2019-05-21T07:54:03Z

pkg/types/notification.go

@@ -35,6 +35,9 @@ const (

 	// DELETED represents a notification type for deleting a resource
 	DELETED OperationType = "DELETED"
+
+	// INVALIDREVISION revision with invalid value
+	INVALIDREVISION int64 = -1


strange for this to be in the types package. I'd rather have the previous state

It is used in the storage/postgres and api/notifications. It will be strange for any of them to import the other. It cannot be in storage as well as postgres must not import storage. This seemed the most appropriate as InvalidRevision is some value of a property of notification similar to the operation types (CREATED, DELETED ...).

Can it be InvalidNotificationRevision then, because types.InvalidRevision doesn't mean much

dpanayotov · 2019-05-21T07:56:27Z

storage/postgres/notificator.go

+	}
+
+	if n.queueSize < len(filteredMissedNotification) {
+		log.C(n.ctx).Debugf("too many missed notifications %d", len(filteredMissedNotification))


why? it should just queue them

Yes, but if it can't queue them (queues have limited size) the proxy will do a full resync.

dpanayotov · 2019-05-21T08:52:38Z

storage/postgres/notificator.go

+		return nil, err
+	}
+	filteredMissedNotification := make([]*types.Notification, 0, len(missedNotifications))
+	var recipients []*types.Platform


remove this row

dpanayotov · 2019-05-21T08:53:37Z

storage/postgres/notificator.go

+	filteredMissedNotification := make([]*types.Notification, 0, len(missedNotifications))
+	var recipients []*types.Platform
+	for _, notification := range missedNotifications {
+		recipients = n.filterRecipients([]*types.Platform{platform}, notification)


this is very weird... it says "filter recepients" that uses "notifications filter" but filters platforms and after that this filtering doesn't use the returned result. The problem is that you're using the same function to do 2 different things.

Hopefully with the rename of NotificationFilterFunc to ReceiversFilterFunc it might be more clear. The idea is to filter given recipients for a specific notification by returning the filtered recipients. This way the one who is writing the filter can process the notification once for all recipients. filterRecipients function iterates over all registered filter functions and applies them to the initial recipients and given notification.

dpanayotov · 2019-05-21T08:55:19Z

storage/postgres/notificator.go

+func (n *Notificator) filterRecipients(recipients []*types.Platform, notification *types.Notification) []*types.Platform {
+	for _, filter := range n.notificationFilters {
+		recipients = filter(recipients, notification)
+		if len(recipients) == 0 {


I can't understand why this works this way - filter returns an empty slice if any filter decides that no platform should receive it and the same list if all filters decide that at least one platform should receive it?

This is an optimisation - if a filter in the chain of filters returns 0 recipients then it is not necessary to call the other filters as there are no platforms to filter. See also comment above.

dzahariev · 2019-05-21T08:49:49Z

api/notifications/notifications.go

@@ -24,22 +22,14 @@ const (
 	LastKnownRevisionHeader     = "last_notification_revision"
 	LastKnownRevisionQueryParam = "last_notification_revision"

-	unknownRevision int64 = -1
-	noRevision      int64 = 0
+	noRevision int64 = 0


Look like this constant is not needed.
If the revision is Unknown the flow is the same.

In the producer: https://github.com/Peripli/service-broker-proxy/blob/master/pkg/sbproxy/notifications/producer.go#L43
The value for InvalidRevision is returned when it is unknown.

SM sends 0 as last notification when there are no notifications in the database. This 0 is received by the proxy and if its connection drops it should resend it as last known revision. In this case SM sends 410 (see line 47 and 79). Idea to improve this is not to send the 0 at all (both by the SM and proxy). But this requires changes in the proxy as well. It should be done in a different PR.

dzahariev · 2019-05-21T08:56:28Z

storage/postgres/notification_storage.go

@@ -69,3 +79,38 @@ func (ns *notificationStorageImpl) GetNotification(ctx context.Context, id strin
 	}
 	return notificationObj.(*types.Notification), nil
 }
+
+func (ns *notificationStorageImpl) ListNotifications(ctx context.Context, platformID string, from, to int64) ([]*types.Notification, error) {
+	// TODO: replace with less than or equal operator when available


Avoid TODOs

They were from a previous PR. I will remove them.

dzahariev · 2019-05-21T08:57:33Z

storage/postgres/notification_storage.go

+		return nil, err
+	}
+	notificationsList := objectList.(*types.Notifications)
+	// TODO: Should be done in the database with order by


The same - create a ticket/issues for the desired/missing functionalities and clean the TODOs.

dpanayotov · 2019-05-22T07:05:57Z

storage/interfaces.go

 	// When consumer wants to stop listening for notifications it must unregister the notification queue.
-	RegisterConsumer(userContext *web.UserContext) (NotificationQueue, int64, error)
+	RegisterConsumer(platform *types.Platform, lastKnownRevision int64) (NotificationQueue, int64, error)


minor - platform should be consumer

Will be addressed in another PR

georgifarashev added 4 commits May 15, 2019 17:50

Add notification filtering

9da5c00

Filter notifications in controller

f0fd112

fix tests

f6dee69

add tests

42e1095

georgifarashev added the 🚧 WIP 🚧 label May 17, 2019

fix lint

b4c7594

georgifarashev force-pushed the notification-filtering branch from 9dc8c8e to b4c7594 Compare May 17, 2019 16:36

Merge remote-tracking branch 'origin/master' into notification-filtering

8f355cd

add tests

eaee3ce

georgifarashev added 👋request review and removed 🚧 WIP 🚧 labels May 20, 2019

change invalid revision constant name

b347e56

dpanayotov previously requested changes May 21, 2019

View reviewed changes

dzahariev reviewed May 21, 2019

View reviewed changes

dpanayotov reviewed May 22, 2019

View reviewed changes

georgifarashev and others added 2 commits May 22, 2019 12:17

fix comments

0199a5d

Merge remote-tracking branch 'origin/master' into notification-filtering

2380091

dzahariev approved these changes May 27, 2019

View reviewed changes

dzahariev merged commit b405502 into master May 27, 2019

dzahariev deleted the notification-filtering branch May 27, 2019 08:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notification filtering #278

Notification filtering #278

georgifarashev commented May 17, 2019

coveralls commented May 17, 2019 •

edited

Loading

dpanayotov left a comment •

edited

Loading

dpanayotov May 21, 2019

georgifarashev May 22, 2019

dpanayotov May 21, 2019

georgifarashev May 22, 2019

dpanayotov May 21, 2019

georgifarashev May 22, 2019

dpanayotov May 21, 2019

dpanayotov May 21, 2019

dpanayotov May 21, 2019

georgifarashev May 22, 2019

dpanayotov May 23, 2019

dpanayotov May 21, 2019

georgifarashev May 22, 2019

dpanayotov May 21, 2019

dpanayotov May 21, 2019 •

edited

Loading

georgifarashev May 22, 2019

dpanayotov May 21, 2019 •

edited

Loading

georgifarashev May 22, 2019

dzahariev May 21, 2019

georgifarashev May 22, 2019

georgifarashev May 22, 2019

dzahariev May 21, 2019

georgifarashev May 22, 2019

dzahariev May 21, 2019

dpanayotov May 22, 2019

		@@ -30,6 +36,10 @@ import (
		type notificationStorage interface {

Notification filtering #278

Notification filtering #278

Conversation

georgifarashev commented May 17, 2019

Notification filtering

Motivation

Approach

Pull Request status

coveralls commented May 17, 2019 • edited Loading

dpanayotov left a comment • 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

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

dpanayotov May 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpanayotov May 21, 2019 • 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

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

coveralls commented May 17, 2019 •

edited

Loading

dpanayotov left a comment •

edited

Loading

dpanayotov May 21, 2019 •

edited

Loading

dpanayotov May 21, 2019 •

edited

Loading