976223 - Loop server needs abstraction layer to store and retrieve service-related data

Status: VERIFIED FIXED

(Hello (Loop) :: Server, defect)

Description

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

The Loop server needs to persist simplepush urls as well as call information in MongoDB. 

We should use or create an small abstraction on top of the driver so we can easily switch to something else if we change our mind eventually.

Comment 1

[Adam Roach [:abr]]

Looking at the current work in progress (https://github.com/mozilla/loop-server/pull/9/files), I'm concerned that every adaptation layer is going to be required to implement application specific logic. Adding a new collection of data -- or even changing the way an existing collection works -- will necessarily require each and every adaptation to be touched to accommodate the changes. This is a maintenance nightmare, since it will require the ops group to touch their adaptation layer every time we update the data we're storing.

What I expected from the adaptation layer is something that abstracts a key/value store, and then an object that uses that abstract layer to implement things like storing push URLs and call records. In fact, this earlier patch seemed to be going in the right direction: https://github.com/mozilla/loop-server/pull/5/files#diff-33a176c9da8c90f3ddfca7abacb017c3R3

Also, while the in-memory adaptation might be nice for proving out the abstract API, I'm not 100% convinced we need to put cycles into writing one right now, unless we think people might have difficulties running a persistent database for their local testing. So let's hold off on implementing that unless and until its utility becomes evident.

Comment 2

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: PR #10

Comment 3

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

So I went with a very simple wrapper on top of the native node driver for mongo, exposing an API being easily implemetable in other backend. A MemoryStore adapter is provided as an example.

Comment 4

[Adam Roach [:abr]]

Comment on attachment 8382187 [details] [review]
PR #10

r-; see comments on github.

To make sure I'm looking at the right code, please tag me for review again when you think the patch is ready for me to look at. Thanks!

Comment 5

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Review comments have been addressed in https://github.com/mozilla/loop-server/commit/5495bdc57988968eb8a566f0f27374f100ef41aa

Please also ensure to read some of my answers to some of your comments there :)

Comment 6

[Adam Roach [:abr]]

Comment on attachment 8382187 [details] [review]
PR #10

r+ for the original pull request as modified by 5495bdc and 29351a4.

Comment 7

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Patch landed into master. I'm leaving anybody with sheriff privileges or equivalent to mark this bug as fixed :)

Comment 8

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

After discussing with :abr on IRC about that, I'll just mark the bug as resolved:

> Go ahead and close, making sure to include a link to the github patch set that actually landed in the accompanying comment.

Landed patch is viewable here: https://github.com/mozilla/loop-server/commit/1b9d4df7fec6b05cab36935cbf948ad2c9695084

Comment 9

[James Bonacci [:jbonacci]]

Closing out these older bugs.