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

completed changing max images to max storage #68

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 25 additions & 13 deletions services/imagery/src/image-store.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,17 @@ export default class ImageStore extends EventEmitter {
* The rate at which images are added is also stored. Note that
* this does not take in consideration the timestamp of the images.
*
* A limit can be placed on the maximum number of images stored by
* setting the maxImages parameter. When the limit is reached,
* A limit can be placed on the maximum size of images stored by
* setting the maxStorage parameter. When the limit is reached,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be absolutely clear on what the unit of maxStorage is supposed to be. I recommend megabytes.

* the store will begin deleting old images.
*/

/** Create a new image store. */
constructor(clearExisting = false, maxImages = undefined) {
constructor(clearExisting = false, maxStorage = undefined) {
super();

this._clearExisting = clearExisting;
this._maxImages = maxImages;
this._maxStorage = maxStorage;

// The time of the last images in the store.
this._times = [];
Expand Down Expand Up @@ -68,10 +68,12 @@ export default class ImageStore extends EventEmitter {
destroy: (db) => db.close()
}, { max: 1, min: 0 });

// Added a size column to store the size of the images
await this._withDb(async (db) => {
await db.run('CREATE TABLE IF NOT EXISTS ' +
'images(id INTEGER PRIMARY KEY AUTOINCREMENT, ' +
'deleted BOOLEAN DEFAULT FALSE)');
'deleted BOOLEAN DEFAULT FALSE,' +
'size REAL)');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

size might want to be INTEGER since it's going to be in bytes.

});
}

Expand Down Expand Up @@ -133,6 +135,13 @@ export default class ImageStore extends EventEmitter {
)))['id'];
}

/** Get the total size of all the images in the database */
async getTotalSize() {
return (await this._withDb(async(db) => await db.get(
'SELECT SUM(size) FROM images WHERE NOT deleted'
)))['SUM(size)'];
}

/**
* Return whether or not an image ID exists (regardless of
* whether or not it was deleted).
Expand Down Expand Up @@ -180,10 +189,13 @@ export default class ImageStore extends EventEmitter {
await this._withDb(async (db) => {
if (id === undefined) {
id = (await db.run('INSERT INTO images DEFAULT VALUES')).lastID;
// Can we simplify this to just one insert statement?
await db.run('UPDATE images SET size = ? WHERE id = ?',
[metadata.size, id]);
Comment on lines 191 to +194
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's possible to simplify into this:

INSERT INTO images(size) VALUES (?);

The rest of the columns will be populated with their default values.

} else {
await db.run('INSERT INTO images(id) VALUES (?)', id);
await db.run('INSERT INTO images(id, size) VALUES (?, ?)',
[id, metadata.size]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The metadata is currently not set up to pass in the size. You'll need to edit the base backend to calculate the size of each photo taken, and update the Protobuf message to include a size field.

}

// Set the id number in the metadata.
metadata.id = id;

Expand Down Expand Up @@ -233,17 +245,17 @@ export default class ImageStore extends EventEmitter {
/** Remove old images such that the image store is no longer
above the limit. */
async purgeImages() {
if (!this._maxImages) return;
if (!this._maxStorage) return;

await this._withDb(async (db) => {
// We cannot use this.getCount because it uses a new connection
// We cannot use this.getSize because it uses a new connection
// from the connection pool, so it will not work while we are
// performing a transaction.
let getCount = async () => (await db.get(
'SELECT COUNT(id) FROM images WHERE NOT DELETED'
))['COUNT(id)'];
let getSize = async () => (await db.get(
'SELECT SUM(size) FROM images WHERE NOT deleted'
))['SUM(size)'];

while (await getCount() > this._maxImages) {
while (await getSize() > this._maxStorage) {
let id = (await db.get(
'SELECT id FROM images WHERE NOT DELETED ORDER BY id ASC'
))['id'];
Expand Down
18 changes: 16 additions & 2 deletions services/imagery/test/image-store.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,20 @@ test('images can be added to the image store', async () => {
expect((await imageStore.getMetadata(id2)).time).toEqual(5);
});

test('total size can be computed correctly', async () => {
const imageStore = new ImageStore(true);
await imageStore.setup();

await imageStore.addImage(
shapes[0], imagery.Image.create({ time: 4, size: 100 })
);
await imageStore.addImage(
shapes[1], imagery.Image.create({ time: 5, size: 10 })
);

expect((await imageStore.getTotalSize())).toEqual(110);
});

test('clear existing removes existing images', async () => {
const imageStore1 = new ImageStore(true);
await imageStore1.setup();
Expand Down Expand Up @@ -93,14 +107,14 @@ test('image store stores image add rates', async () => {
});

test('image store deletes old images', async () => {
const imageStore = new ImageStore(true, 2);
const imageStore = new ImageStore(true, 100);
await imageStore.setup();

const ids = [];
for (let i = 0; i < 3; i++) {
ids[i] = await imageStore.addImage(
shapes[i],
imagery.Image.create({ time: i })
imagery.Image.create({ time: i, size: 40})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
imagery.Image.create({ time: i, size: 40})
imagery.Image.create({ time: i, size: 40 })

);
}
const lastIds = [ids[ids.length - 2], ids[ids.length - 1]];
Expand Down