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

Chore: Remove role requirement to use change streams #27281

Merged
merged 6 commits into from
Nov 24, 2022

Conversation

sampaiodiego
Copy link
Member

@sampaiodiego sampaiodiego commented Nov 16, 2022

Proposed changes (including videos or screenshots)

We used to require the clusterMonitor role to be able to use Change Streams because we performed a serverStatus command, to be able to check if the requirements for change streams are met (like using wired tiger for example). But since our minimum MongoDB supported version is now 4.2, and MMAPv1 was removed on this version, there is no need to check that.

The new approach is just try to use change streams, if it fails for any reason, fallback to oplog.

If oplog fails as well, we're now halting the process, since it wasn't able to establish a connection for reading real time data, something that is crucial for the application. An additional check was added to make sure $MONGO_OPLOG_URL is pointing to the local database.

Issue(s)

Closes #20017

Steps to test or reproduce

Further comments

@codecov
Copy link

codecov bot commented Nov 16, 2022

Codecov Report

Merging #27281 (6b43763) into develop (54b2b7f) will increase coverage by 1.00%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #27281      +/-   ##
===========================================
+ Coverage    40.24%   41.25%   +1.00%     
===========================================
  Files          847      819      -28     
  Lines        18430    17933     -497     
  Branches      2052     1988      -64     
===========================================
- Hits          7417     7398      -19     
+ Misses       10727    10247     -480     
- Partials       286      288       +2     
Flag Coverage Δ
e2e 41.25% <ø> (+1.00%) ⬆️

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

@geekgonecrazy geekgonecrazy added this to the 5.4.0 milestone Nov 24, 2022
@geekgonecrazy
Copy link
Contributor

Just for context before this.. frequently people who start using permissions with mongo but missed this permission start encountering reactivity issues that look like Rocket.Chat is buggy.

But then as soon as they added it things started working fine. One common one was connectivity services, but also anything that depended on change stream to populate setting cache.

The permission is only needed for something informational that provides little value. So requiring is costing way more than we lose by removing it.

IMO for customer experience this is a must for the release. If a release wasn't coming I'd argue for a patch that's how impactful it is.

@ggazzo ggazzo merged commit dc8b46d into develop Nov 24, 2022
@ggazzo ggazzo deleted the remove-requirement-for-change-streams branch November 24, 2022 22:17
MartinSchoeler pushed a commit that referenced this pull request Nov 28, 2022
Co-authored-by: Aaron Ogle <geekgonecrazy@users.noreply.github.com>
MartinSchoeler pushed a commit that referenced this pull request Nov 28, 2022
Co-authored-by: Aaron Ogle <geekgonecrazy@users.noreply.github.com>
@ggazzo ggazzo mentioned this pull request Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change stream does not need db admin priviledge
3 participants