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

Remove opData object in MongoDB ops #165

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bfirsh
Copy link
Contributor

@bfirsh bfirsh commented Jan 23, 2013

See #160

@wmertens
Copy link
Contributor

@zamfi happy with this?

@wmertens
Copy link
Contributor

Actually the loop doesn't need to create that result array. The docs array is already there and all that needs to be done is to remove the _id on all the docs.

In fact, I don't think there's an issue with the _id being in the returned doc at all (I might be wrong). How about just returning the docs array?

@bfirsh
Copy link
Contributor Author

bfirsh commented Feb 28, 2013

I did it that way just to make backwards compatibility simple. I suppose it could iterate over docs and remove opData if it exists. Or, of course, we could write a migration script.

@wmertens
Copy link
Contributor

A migration script might actually be the best, since even if you don't run it the future operations are fine and the snapshots are right where they're supposed to be. There won't be a performance hit on read then.

@bfirsh
Copy link
Contributor Author

bfirsh commented Feb 28, 2013

That sounds sensible. What about ops that haven't been snapshotted yet, though?

Perhaps we could do two steps: first, release it with backwards compatibility and the small performance hit alongside a migration script. Then, in the next version, we remove backwards compatibility.

@wmertens
Copy link
Contributor

Deal! 👍

@bfirsh
Copy link
Contributor Author

bfirsh commented Apr 12, 2013

There we go!

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.

2 participants