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

Update Mongoid to 5.x release (mongodb 3.x + compatibility) #132

Merged
merged 2 commits into from
Dec 22, 2015

Conversation

macdiesel
Copy link
Contributor

All tests passing on mongodb 3.x installed locally. We will need to run a load test on this as the Identify mapping (caching) functionality was dropped from mongoid in the 4.x version.

@macdiesel macdiesel force-pushed the bbeggs/mongodb-3.0-PLAT-730 branch 5 times, most recently from ca6f93e to 4f14a1f Compare August 10, 2015 14:33
@smarnach
Copy link

smarnach commented Sep 7, 2015

@macdiesel Great to see the progress of the Mongo3 upgrade! We would like to run one or our Open edX instances on Mongo3 only. I understand that we'd need to include this PR to make the forums work on Mongo 3. Is this correct? Is there a timeline for reviewing and merging this PR?

@macdiesel
Copy link
Contributor Author

@smarnach yes you would need this work to use mongodb 3.x+. My concern is that since the caching layer was removed from mongoid we could see a big hit in performance of the application. I have some time schedule to do load testing on Thursday (9/10/2015). This should give us a better handle on what the impact of the removal of identity mapping will be.

@smarnach
Copy link

smarnach commented Sep 8, 2015

@macdiesel Thanks for the prompt reply! We aren't that concerned about performance since the instance we would be using this for has a relatively small number of users, at least compared to edx.org. I'm watching the JIRA ticket for this, and will be interested in the results. Thanks for doing this!

@macdiesel
Copy link
Contributor Author

@smarnach ticketing for the load test is here: https://openedx.atlassian.net/browse/PLAT-830

@smarnach
Copy link

smarnach commented Sep 8, 2015

@macdiesel Thanks for the link. I already added myself to the watchlist of https://openedx.atlassian.net/browse/PLAT-768, which seems to be a duplicate.

@@ -30,12 +6,6 @@ GIT
mongoid (>= 3.0, <= 4.0)

Choose a reason for hiding this comment

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

This dependency results in mongoid being installed at exactly the version 4.0.0, instead of the bugfix release version 4.0.2. I can't really imagine this is the actual intention.

@smarnach
Copy link

@macdiesel I tested this PR on a local devstack with MongoDB 3.0 installed, both manually and by running the tests. All tests are passing for me as well, and I couldn't find any issues. I also had a quick glance at the code. It looks all reasonable to me, but my Ruby knowledge is rather cursory, so that's probably not worth a lot. :)

I noticed the following comment in two places:

TODO: Pull in protected_attributes gem to fix this functionality

What functionality would be affected by this? Would this be something blocking us from using this patch in a production environment?

Thanks again for doing this!

@macdiesel macdiesel force-pushed the bbeggs/mongodb-3.0-PLAT-730 branch 2 times, most recently from 2702262 to 6611a8c Compare September 21, 2015 13:53
@macdiesel
Copy link
Contributor Author

@smarnach I am still working on the load testing and should have a better idea of the release date after the next two days.

As to attr_accessible: http://guides.rubyonrails.org/v3.2.13/security.html#mass-assignment
It's a white list of fields that can be assigned when using the constructor of a model. This is something that I will finish before the final merge, thanks for the reminder, it had slipped my mind.

@mtyaka
Copy link
Contributor

mtyaka commented Sep 24, 2015

@macdiesel This PR upgrades the i18n dependency to 0.7.0. The enforce_available_locales configuration option default has changed from nil (meaning log warning) to true (raise exception).
That means that trying to access the comments service from an edx-platform installation in a language other than the few available by default raises an error.

It would good to set I18n.enforce_available_locales = false somewhere in app.rb to maintain backwards compatibility.

@mtyaka
Copy link
Contributor

mtyaka commented Oct 2, 2015

@macdiesel We found out that mongoid 4.x doesn't properly work with mongo 3. We had to upgrade to mongoid 5.0. We're working on it here: edx/cs_comments_service@bbeggs/mongodb-3.0-PLAT-730...open-craft:mongodb-3.0

We'll open a proper PR once we fix some remaining issues and update the specs.

@macdiesel
Copy link
Contributor Author

@mtyaka Interesting, what kind of issues did you find? One of the reasons I had stopped at mongoid 4.x was because of voteable_mongo. but it seems like you may have found a mongoid 5.x compatible branch?

I'm really interested to hear about what difficulties you ran in to during your testing.

@mtyaka
Copy link
Contributor

mtyaka commented Oct 6, 2015

@macdiesel Mongoid 4 failed to connect to our mongolab hosted databases that were migrated to mongo 3. It failed with an authentication error. Mongolab only supports the new SCRAM SHA authentication schema which doesn't work with Mongoid 4.x (see mongoid/moped#367).

@macdiesel
Copy link
Contributor Author

@mtyaka Done, and thank you for the help.

@openedx-webhooks openedx-webhooks added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 3, 2015
@macdiesel macdiesel changed the title Make forums work with mongodb 3.x Update Mongoid to 5.x release (mongodb 3.x + compatibility) Dec 3, 2015
@sarina sarina added the open-source-contribution PR author is not from Axim or 2U label Dec 3, 2015
@macdiesel
Copy link
Contributor Author

First pass load test results can be found here: https://openedx.atlassian.net/wiki/display/PLAT/Mongoid+5.0.0+load+test+results

@macdiesel
Copy link
Contributor Author

@BenjiLee @feanil @e0d Please take a look, I would like to get the mongoid upgrade merged back in to master.

@BenjiLee
Copy link
Contributor

@macdiesel My only concern is that our loadtests are not simulating the same type of traffic on production. If this were to roll out, is there a reasonable rollback plan?

@e0d
Copy link
Contributor

e0d commented Dec 22, 2015

@macdiesel somehow I missed the @ but I'm in favor of merging. Correct me if I'm wrong, but I don't think the rollback plan is complex. There are no alterations to persisted data, for example.

@macdiesel
Copy link
Contributor Author

Since there are now data migrations the rollback plan for this would be to simply revert the PR and put the prior AMIs back in the load balancer.

@e0d @BenjiLee thumbs then? good to merge?

@e0d
Copy link
Contributor

e0d commented Dec 22, 2015

I'm 👍 to merge.

@macdiesel macdiesel mentioned this pull request Dec 22, 2015
@BenjiLee
Copy link
Contributor

👍

macdiesel pushed a commit that referenced this pull request Dec 22, 2015
Update Mongoid to 5.x release (mongodb 3.x + compatibility)
@macdiesel macdiesel merged commit 26fc3a6 into master Dec 22, 2015
@clintonb clintonb deleted the bbeggs/mongodb-3.0-PLAT-730 branch May 4, 2016 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants