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

Edit Setup.ts to reflect database needs #1772

Closed
Cioppolo14 opened this issue Jan 31, 2024 · 24 comments
Closed

Edit Setup.ts to reflect database needs #1772

Cioppolo14 opened this issue Jan 31, 2024 · 24 comments
Assignees
Labels
bug Something isn't working no-issue-activity No issue activity

Comments

@Cioppolo14
Copy link
Contributor

Cioppolo14 commented Jan 31, 2024

Describe the bug
Currently, Talawa-api needs to have access to either a local database with a replica set or mongoDB atlas to use the retry writes option. Our setup.ts script does not support the user in ensure that one of these options is met.

Expected behavior
Setup.ts needs to guide the user to make sure the database will work with retry rewrites.

Actual behavior
Setup does not give the user this guidance.

Potential internship candidates
Please read this if you are planning to apply for a Palisadoes Foundation internship PalisadoesFoundation/talawa#359

@Cioppolo14 Cioppolo14 added the bug Something isn't working label Jan 31, 2024
@github-actions github-actions bot added the unapproved Unapproved for Pull Request label Jan 31, 2024
@adi790uu
Copy link
Contributor

I would like to work on this.

@Cioppolo14 Cioppolo14 removed the unapproved Unapproved for Pull Request label Feb 1, 2024
@adi790uu
Copy link
Contributor

adi790uu commented Feb 1, 2024

@Cioppolo14 why do we need to have access to local database with a replica set.

@Cioppolo14
Copy link
Contributor Author

Cioppolo14 commented Feb 2, 2024

@adi790uu Its the developer's choice, either to run mongo atlas cluster or a local database converted to a replica set. If the local database doesn't have a replica set, retry writes won't work.

Does that make sense? My wording may not have been great in the issue.

@palisadoes
Copy link
Contributor

It's going to have to be a mandatory requirement for non Atlas URLs for both the API database and API test database (which is created simultaneously)

@adi790uu
Copy link
Contributor

adi790uu commented Feb 2, 2024

ok @Cioppolo14 do you want me to write a script which automatically makes the local instance to be a replica set, something like this :

`
const { execSync } = require('child_process');

function executeCommand(command) {
return execSync(command, { encoding: 'utf-8' });
}

const ports = [27017, 27018];
ports.forEach(port => {
executeCommand(mongod --port ${port} --dbpath /path/to/data/db${port});
});

executeCommand(mongo --port 27017 --eval "rs.initiate({_id: 'myReplicaSet', members: [{ _id: 0, host: 'localhost:27017' },{ _id: 1, host: 'localhost:27018' }]})");

console.log(mongodb://localhost:27017,localhost:27018/myDatabase?retryWrites=true);
`

@adi790uu
Copy link
Contributor

adi790uu commented Feb 2, 2024

And in case of atlas Url make sure retryWrites=true parameter is present in the url? like this: mongodb+srv://<username>:<password>@<cluster-name>.mongodb.net/<database-name>?retryWrites=true

@adi790uu
Copy link
Contributor

adi790uu commented Feb 3, 2024

@palisadoes can you review the above comments and give me your opinion.

@palisadoes
Copy link
Contributor

We are awaiting feedback from @EshaanAgg on this topic

@adi790uu
Copy link
Contributor

adi790uu commented Feb 3, 2024

Okay @palisadoes

@EshaanAgg
Copy link
Contributor

EshaanAgg commented Feb 3, 2024

No need for a script. The user should be responsible for providing the correct URL. We should just inform the user of this difference through the setup script, and issue a warning if the user gives a local Mongo URL with retry writes enabled. The user should be able to bypass the warning if he choses to.

@adi790uu
Copy link
Contributor

adi790uu commented Feb 3, 2024

@EshaanAgg atleast we can attach retryWrites=true parameter to the Atlas url if not present already.

@adi790uu
Copy link
Contributor

adi790uu commented Feb 3, 2024

@palisadoes got it.

@palisadoes
Copy link
Contributor

After doing more research on this topic, @EshaanAgg is right.

Keep in mind that not all persons installing the app will be technically knowledgeable. The script must make installation as foolproof as possible.

@palisadoes
Copy link
Contributor

@adi790uu @EshaanAgg @Cioppolo14

  1. The issue is caused by the latest implementation of event calendar entries.
  2. If you are running on localhost, and you create an event you get this error.
    1. We are trying to address this with this issue
    2. Fix Recurring Events #1583 (comment)
  3. This wasn't the case previously. Therefore this may be a temporary fix. How best can we address this in the meantime?

image

@varshith257
Copy link
Member

varshith257 commented Feb 4, 2024

@palisadoes I encountered same while exploring talawa-admin. The potential reason for the recommendation to add retryWrites=false to MongoDB connection string could be a mismatch between the version of MongoDB driver (mongoose) and the MongoDB server that support retryWrites as most of us setup db locally in Atlas.

For a temporary fix, adding retryWrites=false to end of connection string can be a workaround to address the immediate issue. However, we need the retryWrites for robust functionality of events. For future proof solutions we need make ensure compatibility between driver and server if need require to setup replicaSet as suggestions made as following:
https://stackoverflow.com/questions/58589631/mongoerror-this-mongodb-deployment-does-not-support-retryable-writes-please-ad

https://www.mongodb.com/docs/manual/core/retryable-writes/

https://mongoosejs.com/docs/compatibility.html

@adi790uu
Copy link
Contributor

adi790uu commented Feb 5, 2024

@palisadoes I have made the thing work based on this article: https://dev.to/salim_angelo/how-to-convert-your-localhost-standalone-mongodb-cluster-into-a-replica-set-using-run-rs-npm-package-on-mac-os-54hd , I created a replication set and I am not facing the error for local instance of database.

Screenshot 2024-02-05 at 9 24 26 PM Screenshot 2024-02-05 at 9 24 36 PM

@adi790uu
Copy link
Contributor

adi790uu commented Feb 5, 2024

Though when we stop the run-rs session all the data will be lost so we need to keep it on during the development process which is not desirable.

@palisadoes
Copy link
Contributor

This was triggered when we created recurring events. @meetulr the previous behavior needs to be restored as part of your revised recurring event issue.

I'm going to close this issue because we are working on a solution in the issue above.

@palisadoes palisadoes closed this as not planned Won't fix, can't repro, duplicate, stale Feb 5, 2024
@meetulr
Copy link
Contributor

meetulr commented Feb 6, 2024

This was triggered when we created recurring events. @meetulr the previous behavior needs to be restored as part of your revised recurring event issue.

I'm going to close this issue because we are working on a solution in the issue above.

Do you mean we shouldn't use transactions? Replica set would be required for transactions.
If I removed transactions, things would work as before, but we wouldn't have that surety of everything passes or everything fails, as discussed here.

@palisadoes palisadoes reopened this Feb 6, 2024
@palisadoes
Copy link
Contributor

@meetulr What would you suggest as the solution to make setup.ts work correctly for local and hosted Mongo instances?

Reopening

@meetulr
Copy link
Contributor

meetulr commented Feb 9, 2024

@palisadoes I'm sorry I missed your comment.

Actually though, I was thinking about it.
I realized that rather than making a replica set mandatory, we could make session optional?

What I mean is that we could add a check for it in db.ts. i.e. After connecting to our mongoDB instance, we could check if it's a replica set or not, if it is, we'd start a session, if it's not, the session would not be initiated.

I've tried it locally, it's working.

This way we won't have problems with local single node standalone instances, and we can have transactions support in deployed versions.

@palisadoes
Copy link
Contributor

@adi790uu

Can you implement this for this issue?

@adi790uu
Copy link
Contributor

@palisadoes sure I want to do this, I am already assigned two other issues I will unassign myself from one of them.

adi790uu added a commit to adi790uu/talawa-api that referenced this issue Feb 17, 2024
adi790uu added a commit to adi790uu/talawa-api that referenced this issue Feb 17, 2024
adi790uu added a commit to adi790uu/talawa-api that referenced this issue Feb 19, 2024
adi790uu added a commit to adi790uu/talawa-api that referenced this issue Feb 19, 2024
EshaanAgg added a commit to adi790uu/talawa-api that referenced this issue Feb 19, 2024
Copy link

This issue did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please check if the develop branch has fixed it and report again or close the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working no-issue-activity No issue activity
Projects
None yet
Development

No branches or pull requests

6 participants