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

docs(hooks.md, model.js): add warnings about memory limitations of individualHooks #9881

Merged
merged 1 commit into from Sep 13, 2018

Conversation

@jedwards1211
Copy link
Contributor

@jedwards1211 jedwards1211 commented Sep 5, 2018

Pull Request check-list

Please make sure to review and check all of these items:

Not applicable, this is a document only change

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

I was very disappointed to realize today that if were to update or destroy a billion instances with individual hooks, Sequelize would try to fetch all billion of them into memory before running the individual hooks. I had mistakenly assumed it would iterate through them in batches or using cursors of some kind...

I wish I had been warned about this before I decided to start using hooks, so I'm adding this warning to the docs.

@jedwards1211 jedwards1211 changed the title add warning about memory limits of individual hooks add warning about memory limitations of individualHooks Sep 5, 2018
@codecov
Copy link

@codecov codecov bot commented Sep 5, 2018

Codecov Report

Merging #9881 into master will decrease coverage by 4.7%.
The diff coverage is n/a.

@jedwards1211 jedwards1211 changed the title add warning about memory limitations of individualHooks add warnings about memory limitations of individualHooks Sep 5, 2018
@jedwards1211
Copy link
Contributor Author

@jedwards1211 jedwards1211 commented Sep 5, 2018

Codecov report that coverage will decrease is totally bogus, I haven't changed any code at all. Are there race conditions in the tests?

@jedwards1211 jedwards1211 changed the title add warnings about memory limitations of individualHooks docs: add warnings about memory limitations of individualHooks Sep 5, 2018
@jedwards1211 jedwards1211 changed the title docs: add warnings about memory limitations of individualHooks docs(hooks.md, model.js): add warnings about memory limitations of individualHooks Sep 5, 2018
@@ -2664,7 +2664,7 @@ class Model {
* @param {Object} options destroy options

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 6, 2018
Contributor

Remove all changes from this file, it is ok to have this as a documentation only warning.

This comment has been minimized.

@jedwards1211

jedwards1211 Sep 6, 2018
Author Contributor

@sushantdhiman what do you think is the downside to also warning in this file?
The Sequelize documentation is large and a warning in the tutorial could be easily missed (IIRC I learned about individualHooks in the reference docs before seeing it in the tutorial docs.) Having this warning in more places makes it more likely that devs will see it.

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 6, 2018
Contributor

API docs should not cover such warnings or expanded explanation, they should only explain working of any give option and platform limitation if any

This comment has been minimized.

@jedwards1211

jedwards1211 Sep 6, 2018
Author Contributor

Okay, would it be okay for the API docs to simply say "all instances will be fetched into memory before running hooks"? That is explaining what it does...

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 6, 2018
Contributor

SELECT all records matching the where parameter... it should be clear by that. SELECTED records will be loaded to memory and large SELECT surface will consume more memory

This comment has been minimized.

@jedwards1211

jedwards1211 Sep 13, 2018
Author Contributor

Alright, I removed the changes to model.js

@jedwards1211 jedwards1211 force-pushed the jedwards1211:patch-2 branch from 0b15bed to 72d9886 Sep 13, 2018
@sushantdhiman sushantdhiman merged commit b196273 into sequelize:master Sep 13, 2018
3 of 4 checks passed
3 of 4 checks passed
codecov/project 91.33% (-4.71%) compared to 31ee6a7
Details
codecov/patch Coverage not affected when comparing 31ee6a7...72d9886
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants