Skip to content

Conversation

@ggazzo
Copy link
Member

@ggazzo ggazzo commented Nov 14, 2023

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

@changeset-bot
Copy link

changeset-bot bot commented Nov 14, 2023

⚠️ No Changeset found

Latest commit: bf6fdba

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ggazzo ggazzo added this to the 6.5.0 milestone Nov 14, 2023
@codecov
Copy link

codecov bot commented Nov 14, 2023

Codecov Report

Merging #30948 (bf6fdba) into release-6.5.0 (edc58b9) will increase coverage by 1.47%.
Report is 1 commits behind head on release-6.5.0.
The diff coverage is 80.00%.

Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                @@
##           release-6.5.0   #30948      +/-   ##
=================================================
+ Coverage          49.86%   51.33%   +1.47%     
=================================================
  Files                784      813      +29     
  Lines              14950    15234     +284     
  Branches            2791     2817      +26     
=================================================
+ Hits                7455     7821     +366     
+ Misses              7081     6984      -97     
- Partials             414      429      +15     
Flag Coverage Δ
e2e 48.36% <80.00%> (+1.73%) ⬆️
unit 65.98% <73.91%> (+0.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: needs QA labels Nov 14, 2023
@ggazzo ggazzo marked this pull request as ready for review November 14, 2023 05:31
@ggazzo ggazzo requested review from a team as code owners November 14, 2023 05:31
@ggazzo ggazzo merged commit 4354cab into release-6.5.0 Nov 14, 2023
@ggazzo ggazzo deleted the chore/improve-logging-license branch November 14, 2023 05:32
@nmagedman
Copy link
Contributor

It appears to me that when you do a change like this:

commit 4354cab319bba11288c90dccaea58b13492ad396
Author: Guilherme Gazzo <[email protected]>
Date:   Tue Nov 14 02:32:34 2023 -0300

    ...
    * fix: Wrong index for Analytics
    ...

diff --git a/apps/meteor/server/models/raw/Analytics.ts b/apps/meteor/server/models/raw/Analytics.ts
index 254ff32965..4e95cadebb 100644
--- a/apps/meteor/server/models/raw/Analytics.ts
+++ b/apps/meteor/server/models/raw/Analytics.ts
@@ -14,7 +14,7 @@ export class AnalyticsRaw extends BaseRaw<IAnalytic> implements IAnalyticsModel
 	}

 	protected modelIndexes(): IndexDescription[] {
-		return [{ key: { date: 1 } }, { key: { 'room._id': 1, 'date': 1 }, unique: true }];
+		return [{ key: { date: 1 } }, { key: { 'room._id': 1, 'date': 1 }, unique: true, partialFilterExpression: { type: 'rooms' } }];
 	}

 	saveMessageSent({ room, date }: { room: IRoom; date: IAnalytic['date'] }): Promise<Document | UpdateResult> {

it can lead to the following fatal error on startup:

systemd[1]: Starting Rocket.Chat service...
rocket.chat[2187053]: Some indexes for collection 'rocketchat_analytics' could not be created:
rocket.chat[2187053]:         An existing index has the same name as the requested index. When index names are not specified, they are auto generated and can cause conflicts. Please refer to our documentation. Requested index: { v: 2, unique: true, key: { room._id: 1, date: 1 }, name: "room._id_1_date_1", partialFilterExpression: { type: "rooms" } }, existing index: { v: 2, unique: true, key: { room._id: 1, date: 1 }, name: "room._id_1_date_1" }

That is, you can't just simply change the options for an index. You have to somehow handle the transition from the old options to the new options.

@nmagedman
Copy link
Contributor

This PR adds a migration that drops the old index, however the migrations don't run until after the index creation was attempted. After the migration runs, we have no index at all, neither with its new or old options. We don't attempt again to create the index until RC is restarted again, so for that entire run, those queries will require COLLSCANs.

Is there a way of having the migrations not only drop the old index but also immediately trigger the re-creation? A possible solution is to name the index explicitly, mentioning the options, e.g. "room._id_1_date_1___ partialFilterExpression_type_rooms".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants