storagenode/notifications: db created #3707
Conversation
VitaliiShpital
requested review from
rikysya,
stefanbenten,
crawter,
Qweder93 and
a team
ghost
removed their request for review
| "storj.io/storj/pkg/storj" | ||
| ) | ||
|
|
||
| // DB is an implementation of notifications.Notifications. |
There was a problem hiding this comment.
it's interface, not implementation
this DB interface exposes the way how to manage records in your db implementation
|
|
||
| // Notification holds notification entity info which is being retrieved from database. | ||
| type Notification struct { | ||
| ID uuid.UUID |
There was a problem hiding this comment.
should we inherit Notification from the NewNotification?
There was a problem hiding this comment.
let's leave it as it is for better readability
| "testing" | ||
| "time" | ||
|
|
||
| "storj.io/storj/private/testrand" |
| Page: 1, | ||
| } | ||
|
|
||
| // test read. |
There was a problem hiding this comment.
this comments are not meaningful
|
|
||
| page := notifications.NotificationPage{} | ||
|
|
||
| // test paged list. |
There was a problem hiding this comment.
this comment is not meaningful
| assert.Equal(t, page.Notifications[2].ReadAt, (*time.Time)(nil)) | ||
| }) | ||
|
|
||
| // test read all. |
There was a problem hiding this comment.
this comment is not meaningful
| } | ||
|
|
||
| // Insert puts new notification to database. | ||
| func (repo *notificationDB) Insert(ctx context.Context, notification notifications.NewNotification) (_ notifications.Notification, err error) { |
There was a problem hiding this comment.
repo is a bad name for this receiver. Lets name it db?
storagenode/storagenodedb/uuid.go
Outdated
| ) | ||
|
|
||
| // bytesToUUID is used to convert []byte to UUID | ||
| func bytesToUUID(data []byte) (uuid.UUID, error) { |
There was a problem hiding this comment.
this method is used in satellitedb and in storagenodedb. What if we place it in pkg or private?
There was a problem hiding this comment.
let me do it in another PR since it's not in scope of this one
There was a problem hiding this comment.
I feel like this should be done within the PR, else its forgotten about.
Especially if reusing existing code, moving it into another package is fine.
There was a problem hiding this comment.
if i do it within this PR it's size would become really big, cause i'll have to update a lot of repositories in satellite and storagenode. I'll create a ticket in Jira not to forget about it
| NotificationTypeDisqualification NotificationType = 3 | ||
| ) | ||
|
|
||
| // NewNotification holds notification entity info which is being inserted to database. |
There was a problem hiding this comment.
which is being received from satellite
| "storj.io/storj/pkg/storj" | ||
| ) | ||
|
|
||
| // DB works with notifications database. |
There was a problem hiding this comment.
DB tells us how application want to work with db
| // Copyright (C) 2019 Storj Labs, Inc. | ||
| // See LICENSE for copying information. | ||
|
|
||
| package notifications |
There was a problem hiding this comment.
Lets avoid a second package name with similar name in regards to PR #3659
There was a problem hiding this comment.
why do we care? it is the same as satellite/orders and storagenode/orders or console pkg
|
|
||
| const ( | ||
| // NotificationTypeCustom is a common notification type which doesn't describe node's core functionality. | ||
| NotificationTypeCustom NotificationType = 0 |
There was a problem hiding this comment.
I feel like this Type will be misused in the future.
We should make something along the lines of Information..
There was a problem hiding this comment.
now we don't know all custom notification types. Let's wait for all the info about it. I'll add TODO for this. Are you ok with that?
There was a problem hiding this comment.
custom notification can have any topic, for example it can be notification about new blogpost, New Year sale, payouts made etc.
| // Copyright (C) 2019 Storj Labs, Inc. | ||
| // See LICENSE for copying information. | ||
|
|
||
| package notifications_test |
storagenode/storagenodedb/uuid.go
Outdated
| ) | ||
|
|
||
| // bytesToUUID is used to convert []byte to UUID | ||
| func bytesToUUID(data []byte) (uuid.UUID, error) { |
There was a problem hiding this comment.
I feel like this should be done within the PR, else its forgotten about.
Especially if reusing existing code, moving it into another package is fine.
There was a problem hiding this comment.
The package name still isnt matching the one of #3659 , which it definitely should.
Thank you so much for making the UUID conversion changes in this PR!
VitaliiShpital
added
the
Reviewer Can Merge
…creating' into storagenodedb_notifications_db
…creating' into storagenodedb_notifications_db
rikysya
changed the title
| } | ||
|
|
||
| // NotificationType is a numeric value of specific notification type. | ||
| type NotificationType int |
There was a problem hiding this comment.
Stutter. notifications.NotificationType, notifcations.Type would be sufficent. See
| const ( | ||
| // NotificationTypeCustom is a common notification type which doesn't describe node's core functionality. | ||
| // TODO: change type name when all notification types will be known | ||
| NotificationTypeCustom NotificationType = 0 |
There was a problem hiding this comment.
Stutter and long notifications.NotificationTypeCustom (same for others)
| } | ||
|
|
||
| // NotificationCursor holds notification cursor entity which is used to create listed page from database. | ||
| type NotificationCursor struct { |
There was a problem hiding this comment.
stutter notifications.NotificationCursor
| } | ||
|
|
||
| // NotificationPage holds notification page entity which is used to show listed page of notifications on UI. | ||
| type NotificationPage struct { |
There was a problem hiding this comment.
stutter notifications.NotificationPage
| } | ||
|
|
||
| // NotificationType is a numeric value of specific notification type. | ||
| type NotificationType int |
There was a problem hiding this comment.
Is there a reason this isn't a string constant instead of an number?
| // NewNotification holds notification entity info which is being received from satellite or local client. | ||
| type NewNotification struct { | ||
| SenderID storj.NodeID | ||
| Type NotificationType |
There was a problem hiding this comment.
Can notifications have only one type? Maybe it should be tags?
There was a problem hiding this comment.
what do you mean? could you please provide an example?
There was a problem hiding this comment.
For example, could the notification be a warning and audit, or something similar? Currently it seems we end up with tons of constants that all try to encode different ideas into a single enumeration. Usually, the level and scope are separated. For some logging/notification systems there's support for custom tags.
There was a problem hiding this comment.
hm, yeah. So you think we should add fields for level and for tags? And probably rename type to scope?
There was a problem hiding this comment.
It's more of a question, . I.e. I don't have a strong argument either way, but I usually just have seen them separated, mainly because it's more flexible (and avoid proliferation of constants). Strings also end up being forwards compatible.
* storj/master: (27 commits) storagenode/garbagecollection: enable in production satellite: adds and enables cockroachdb compatibility for tests scripts: Add script that filters postgres plaintext backup to cockroachdb compat. Adding benchmarking script that reports response times. pkg/rpc: Change drpcheader to save a packet private/dbutil: register "cockroach" as sql.DB driver satellite/satellitedb: unexport satellitedb.DB ci: remove Jenkinsfile.drpc web/satellite: billing banner changed satellitedb: fix migration cockroach test storagenode/notifications: db created (#3707) web/satellite: project limits do not update pointer for failed audits uplink/storage: remove unused Meta methods storagenode/orderdb: fix db lock storagenode/trust: wire up list into pool cmd/segment-reaper: Add test cases from/to processSegment lib/uplinkc: use SliceHeader instead of [1<<30] satellite/metainfo: Improve docs & use common funcs Use Postgres 9.6 for Jenkins builds (#3729) ... Change-Id: I437a8960a3b0b203a2e82bec5e4badaeb8f11563
What:
db for notification system for SN implemented
Why:
SN Notification System MVP
Please describe the tests:
Please describe the performance impact:
Code Review Checklist (to be filled out by reviewer)