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.

Sign up for GitHub

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

fix: partial index on events announced #4856

Merged
merged 6 commits into from
Sep 28, 2023
Merged

fix: partial index on events announced #4856

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3eacbd9
fix: partial index on events announced
gastonfournier Sep 27, 2023
cc82ba6
Fix test name
gastonfournier Sep 28, 2023
a38f735
Fix lint
gastonfournier Sep 28, 2023
8bccf4c
Merge remote-tracking branch 'origin/main' into index-events-by-annou…
gastonfournier Sep 28, 2023
a02e530
Pin node to 18.17
gastonfournier Sep 28, 2023
29ff257
Pin node-version to 18.17
gastonfournier Sep 28, 2023
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
52 changes: 19 additions & 33 deletions src/lib/db/event-store.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ test('Trying to get events if db fails should yield empty list', async () => {
const store = new EventStore(db, getLogger);
const events = await store.getEvents();
expect(events.length).toBe(0);
await db.destroy();
});

test('Trying to get events by name if db fails should yield empty list', async () => {
Expand All @@ -29,39 +30,24 @@ test('Trying to get events by name if db fails should yield empty list', async (
const events = await store.searchEvents({ type: 'application-created' });
expect(events).toBeTruthy();
expect(events.length).toBe(0);
await db.destroy();
});

test.each([
{
createdAt: formatRFC3339(subHours(new Date(), 1)),
expectedCount: 1,
},
{
createdAt: formatRFC3339(subHours(new Date(), 23)),
expectedCount: 1,
},
{
createdAt: formatRFC3339(subHours(new Date(), 25)),
expectedCount: 0,
},
])(
'Find unnanounced events is capped to last 24hs',
async ({ createdAt, expectedCount }) => {
const db = await dbInit('events_test', getLogger);
const type = 'application-created' as const;
const insertQuery = db.rawDatabase('events');
await insertQuery
.insert({
type,
created_at: createdAt,
created_by: 'a test',
data: { name: 'test', createdAt },
})
.returning(['id']);
test('Find unnanounced events is capped to 500', async () => {
gastonfournier marked this conversation as resolved.
Show resolved Hide resolved
const db = await dbInit('events_test', getLogger);
const type = 'application-created' as const;

const store = new EventStore(db.rawDatabase, getLogger);
const events = await store.setUnannouncedToAnnounced();
expect(events).toBeTruthy();
expect(events.length).toBe(expectedCount);
},
);
const allEvents = Array.from({ length: 505 }).map((_, i) => ({
type,
created_at: formatRFC3339(subHours(new Date(), i)),
created_by: `test ${i}`,
data: { name: 'test', iteration: i },
}));
await db.rawDatabase('events').insert(allEvents).returning(['id']);

const store = new EventStore(db.rawDatabase, getLogger);
let events = await store.setUnannouncedToAnnounced();
expect(events).toBeTruthy();
expect(events.length).toBe(505);
await db.destroy();
});
7 changes: 1 addition & 6 deletions src/lib/db/event-store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import { sharedEventEmitter } from '../util/anyEventEmitter';
import { Db } from './db';
import { Knex } from 'knex';
import EventEmitter from 'events';
import { subDays } from 'date-fns';

const EVENT_COLUMNS = [
'id',
Expand Down Expand Up @@ -412,11 +411,7 @@ class EventStore implements IEventStore {
const rows = await this.db(TABLE)
.update({ announced: true })
.where('announced', false)
.whereNotNull('announced')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where not null no longer needed due to the migration

.where('created_at', '>', subDays(Date.now(), 1))
.returning(EVENT_COLUMNS)
.limit(500);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I've tested that limit does not work with update. There are alternatives but may require using native queries:


.returning(EVENT_COLUMNS);
return rows.map(this.rowToEvent);
}

Expand Down
17 changes: 17 additions & 0 deletions src/migrations/20230927172930-events-announced-index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
'use strict';

exports.up = function (db, callback) {
db.runSql(
`
UPDATE events set announced = false where announced IS NULL;
ALTER TABLE events ALTER COLUMN announced SET NOT NULL;
ALTER TABLE events ALTER COLUMN announced SET DEFAULT false;
CREATE INDEX events_unannounced_idx ON events(announced) WHERE announced = false;
`,
callback(),
);
};

exports.down = function (db, callback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a down function would be beneficial.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered that but down migrations are not executed automatically, so if we want to make changes we're probably going to move forward rather than backward

Copy link
Member

Choose a reason for hiding this comment

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

@sjaanus I know we're in a weird place with the down migrations and what we should do with them. I've never used them though. Are you using them? I don't remember any recommendations to users to consider down migrations either, our response has always been get latest version where we fixed the issue

Copy link
Contributor

Choose a reason for hiding this comment

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

@sighphyre I have not used, but I know customers have backed down to previous versions and in some cases if we do not provide down migration, it may make their instance unusable.

For example, if we currently do not allow nulls and previously somehow they were entered, we would probably just have errors in logs, but in other cases, if we change something related to projects/strategies/segments, it might break UI.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah, if there are actually folks using them then it seems important

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know customers have backed down to previous versions and in some cases if we do not provide down migration, it may make their instance unusable.

Indeed, but even in these cases, there's no command for the customer to execute the down migration other than by hand. They'd have to either go one by one, or identify the one they need to execute.

In such a scenario, the down migration does not update the migrations table (because we're not using the tool for this), and therefore the framework doing the up migration will still believe that the migration is applied and will never try to apply it again... this leaves the customer in a very weird situation.

I know because we faced this same problem with our demo instance and we learned that down migrations should not be applied manually because they don't include the change in the migrations table (and they shouldn't! The right solution is to use the tool's command for doing it)

callback();
};
Loading