Skip to content

Update changelog to mention performance improvements in write batches #1534

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

Merged
merged 2 commits into from
Jul 17, 2018

Conversation

var-const
Copy link
Contributor

@var-const var-const commented Jul 14, 2018

Reflects #1505, #1507.

@@ -4,6 +4,10 @@
- [feature] Added `whereField(arrayContains:)` query filter to find
documents where an array field contains a specific element.
- [fixed] Fixed compilation with older Xcode versions (#1517).
- [fixed] Fixed performance issue when trying to commit a write batch with many
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more specific than it needs to be and uses internal terminology. I suggest something like the following phrasing:

Fixed a performance issue where large write batches with hundreds of changes would take a long time to read and write and consume excessive memory. Large write batches should now see no penalty.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, please include the issue number/numbers if there are any.

Copy link
Contributor

Choose a reason for hiding this comment

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

Finally, note that you can include the CHANGELOG update in the change that fixes the issue (#1533, right)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I like the rephrasing much better. I don't think there was a Github issue for this change, though.

you can include the CHANGELOG update in the change that fixes the issue

The changes I was trying to describe are #1505 and #1507, already merged.

I've added an entry accounting for #1533 to this PR to avoid merging, though I can also add the relevant entry to #1533 if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Putting these here is fine--that was most FYI for the future.

@wilhuff wilhuff assigned var-const and unassigned wilhuff Jul 14, 2018
Copy link
Contributor

@wilhuff wilhuff left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -4,6 +4,10 @@
- [feature] Added `whereField(arrayContains:)` query filter to find
documents where an array field contains a specific element.
- [fixed] Fixed compilation with older Xcode versions (#1517).
- [fixed] Fixed performance issue when trying to commit a write batch with many
Copy link
Contributor

Choose a reason for hiding this comment

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

Putting these here is fine--that was most FYI for the future.

@var-const var-const merged commit d6d273a into master Jul 17, 2018
@paulb777 paulb777 deleted the varconst/changelog branch May 26, 2019 20:47
@firebase firebase locked and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants