Skip to content

Upgrade mongoengine to the latest version (0.15.3) - #4291

Merged
Kami merged 17 commits into
masterfrom
upgrade_mongoengine
Aug 6, 2018
Merged

Kami merged 17 commits into
masterfrom
upgrade_mongoengine

Conversation

@Kami

@Kami Kami commented Aug 3, 2018

Copy link
Copy Markdown
Member

This pull request updates mongoengine to the latest version (0.15.3) from our fork (MongoEngine/mongoengine#1833) which contains a work-around for a large regression in memory usage introduced in mongoengine v0.13.0.

I confirmed reverting that commit fixes an issue with increased memory usage.

Using our fork with a reverted commit:

(virtualenv) vagrant@ubuntu-xenial:/data/stanley$ python test_mongoengine.py 
Python: 2.7.12 (default, Dec  4 2017, 14:50:18) 
[GCC 5.4.0 20160609] on Linux-4.4.0-130-generic-x86_64-with-Ubuntu-16.04-xenial
mongoengine: 0.15.3
sleeping...

(virtualenv) vagrant@ubuntu-xenial:/data/stanley$ ps -eo rss,pid,euser,args:100 --sort %mem | grep -i 'test_mongoengine' |grep -v grep | awk '{printf $1/1024 "MB"; $1=""; print }'
18.3281MB 12696 vagrant python test_mongoengine.py

Using upstream version:

Python: 2.7.12 (default, Dec  4 2017, 14:50:18) 
[GCC 5.4.0 20160609] on Linux-4.4.0-130-generic-x86_64-with-Ubuntu-16.04-xenial
mongoengine: 0.15.3
sleeping...

(virtualenv) vagrant@ubuntu-xenial:/data/stanley$ ps -eo rss,pid,euser,args:100 --sort %mem | grep -i 'test_mongoengine' |grep -v grep | awk '{printf $1/1024 "MB"; $1=""; print }'
70.6875MB 6348 vagrant python test_mongoengine.py

Latest version will allow us to support MongoDB 3.6.

When / if mongoengine merges a patch for that issue upstream, we should move away from our fork.

Kami added 2 commits August 3, 2018 12:10
and contains a fix for regression in memory usage introduced in v0.13.0.
@Kami Kami added this to the 2.9.0 milestone Aug 3, 2018
mongoengine
# Our fork contains a fix / revert of a change which was introduced in v0.13.0
# and introduces a large regression in memory usage
# See https://github.com/MongoEngine/mongoengine/pull/1833

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@arm4b arm4b left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

It's also something that we'll need to give a good amount of attention during the manual release testing, especially if we'll move to Mongo 3.4 -> 3.6 by default.

@Kami Kami mentioned this pull request Aug 3, 2018
2 of 7 tasks

@m4dcoder m4dcoder left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Need to understand our long term plan before I can agree here.

Comment thread requirements.txt
eventlet==0.23.0
flex==6.13.1
git+https://github.com/Kami/logshipper.git@stackstorm_patched#egg=logshipper
git+https://github.com/StackStorm/mongoengine.git@stackstorm_patched#egg=mongoengine

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm ok with using a fork only if our patch is going to merge upstream in the near term. But if this is indefinitely, this is harder to agree because we will have to maintain this fork moving forward.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The problem is that I'm not really aware of a good upstream fix (besides reverting that change).

I wish there was a better way, but it doesn't look like there is one and the performance regression is sadly too large for us to simply ignore it.

"Good" news is that mongoengine has relatively slow release cadence so keeping our fork up to date shouldn't be too bad (keep in mind that we haven't upgraded mongoengine in our repo for 1 year because of that regression).

@Kami

Kami commented Aug 6, 2018

Copy link
Copy Markdown
Member Author

Finally managed to also get unit tests on Travis to also run under MongoDB 3.6 - https://travis-ci.org/StackStorm/st2/builds/412583451

I will go ahead and merge this into master.

@Kami

Kami commented Aug 6, 2018

Copy link
Copy Markdown
Member Author

It looks like tests run much slower under MongoDB 3.6.

It might be related to this - https://jira.mongodb.org/browse/SERVER-34604

@Kami
Kami merged commit 4a24467 into master Aug 6, 2018
@Kami
Kami deleted the upgrade_mongoengine branch August 6, 2018 13:00
@arm4b

arm4b commented Aug 6, 2018

Copy link
Copy Markdown
Member

A lot of changes added afterwards is not what I reviewed really.

And timing for Travis longest task is back returned to almost half-an-hour :/

@Kami

Kami commented Aug 6, 2018

Copy link
Copy Markdown
Member Author

@armab See comment above - #4291 (comment).

Hopefully it will be resolved in a future 3.6.x release.

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.

3 participants