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

Index Cleanup - Messages #3107

Merged
merged 14 commits into from
Apr 13, 2023

Conversation

djabarovgeorge
Copy link
Contributor

What change does this PR introduce?

Why? (Context)

We want to remove as many inefficient indexs and queries as possible.

Why was this change needed?

Stop using single indexes on the mongoose key implementation
Add compound indexes for common queries and patterns using MongoDB index best practices
Remove unused indexes from MongoDB atlas after implementing the new compound indexes

Other information (Screenshots)

@linear
Copy link

linear bot commented Mar 29, 2023

Comment on lines -211 to -220
async getFeed(
environmentId: string,
query: { channels?: ChannelTypeEnum[]; templates?: string[]; emails?: string[]; _subscriberId?: string } = {},
skip = 0,
limit = 10
) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this one because it is not in use and can confuse other users of the repo.

@@ -22,10 +23,9 @@ export class MessageRepository extends BaseRepository<MessageDBModel, MessageEnt
environmentId: string,
subscriberId: string,
channel: ChannelTypeEnum,
query: { feedId?: string[]; seen?: boolean; read?: boolean } = {},
options: { limit: number; skip?: number } = { limit: 10 }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed 'options' because it was not used internally in the method in order to create the query.


type MessageQuery = FilterQuery<MessageDBModel>;

export class MessageRepository extends BaseRepository<MessageDBModel, MessageEntity> {
export class MessageRepository extends BaseRepository<MessageDBModel, MessageEntity, EnforceEnvId> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added and update the env enforce to EnforceEnvId

const message = await this.messageRepository.findOne({
transactionId,
_environmentId: environment._id,
_subscriberId: subscriber._id,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added subscriberId so the query will fit the index in line 148 in message scheme file

@@ -52,10 +52,14 @@ export class MessageRepository extends BaseRepository<MessageDBModel, MessageEnt

if (query.seen != null) {
requestQuery.seen = query.seen;
} else {
requestQuery.seen = { $in: [true, false] };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added so we always use the same index no matter if there is seen or read or not (with the createAt key)

Comment on lines 130 to 133
/*
* This index was initially created to optimize:
* in app widget - feed query
*/
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'm really not sure about the comment, I just wanted to make sure the initial intent wasn't ambiguous and was documented so that if needed we could remove or update the indexes if needed.

_subscriberId: 1,
_environmentId: 1,
channel: 1,
_feedId: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should be removed, as it's optional in some places

Comment on lines 235 to 238
messageSchema.index({
_parentId: 1,
_environmentId: 1,
});
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 this one is not relevant

Comment on lines 276 to 283
transactionId: 1,
_subscriberId: 1,
_environmentId: 1,
_messageTemplateId: 1,
_templateId: 1,
providerId: 1,
channel: 1,
_feedId: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
transactionId: 1,
_subscriberId: 1,
_environmentId: 1,
_messageTemplateId: 1,
_templateId: 1,
providerId: 1,
channel: 1,
_feedId: 1,
transactionId: 1,
_subscriberId: 1,
_environmentId: 1,
providerId: 1,

messageSchema.index({
_environmentId: 1,
providerId: 1,
channel: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
channel: 1,

@djabarovgeorge djabarovgeorge merged commit 7ba0249 into next Apr 13, 2023
13 checks passed
@djabarovgeorge djabarovgeorge deleted the NV-1928/messages-collection-indexes-refactor branch April 13, 2023 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants