-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(firestore): adds Bulkwriter support to Firestore client #5946
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last thing about closing the bulk writer.
Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot. |
// After calling End(), calling any additional method automatically returns | ||
// with an error. This method completes when there are no more pending writes | ||
// in the queue. | ||
func (bw *BulkWriter) End() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there might be a minor race condition here. Here's the idea:
- Goroutine 1 in
checkWriteCondition
acquires the lock, sees thatisOpen
is true, and then releases the lock - Goroutine 2 in
End
acquires the lock, setsisOpen
to false, releases the lock, and then flushes - Goroutine 1 meanwhile starts a write
The question is: does the write succeed or not in this case?
One possible option would be to use a read write mutex, where the read mutex wraps the entire write attempt and the write mutex wraps isOpen
here in End
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good point.
Here's what I've done to address this:
- Changed
BulkWriter.isOpenLock
to aRWMutex
. - Added a
Lock()
and deferredUnlock()
at the top of each DB mutator/write function (Create()
,Delete()
, etc).
Each mutator will hold the lock while queueing up the write for the Bundler. If End()
is called during this time, it will will block while the mutator holds the lock.
* chore: release main (#6351) * test(profiler): compile busybench before running backoff test (#6375) * chore(bigquery/storage/managedwriter): augment test logging (#6373) * chore(storage): RewriteObject implementation (#6313) * chore(storage): RewriteObject implementation * address feedback * refactor source/destination object types * address feedback * address feedback * fix test * chore(main): release storage 1.24.0 (#6336) Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Chris Cotter <[email protected]> * chore(internal/gapicgen): update microgen v0.31.2 (#6383) Only includes fixes to regapic generation. * test(bigquery/storage/managedwriter): relax error checking (#6385) When a user issues a large request, the response from the backend is a bare "InvalidArgument". This PR removes additional validation on information that is only attached when interrogating the backend from a known client; it's stripped in the normal case. Internal issue 239740070 was created to address the unactionable nature of the response. Fixes: #6361 * feat(firestore): adds Bulkwriter support to Firestore client (#5946) * feat: adds Bulkwriter support to Firestore client * test(storage): unflake TestIntegration_ACL (#6392) Few minor changes to make sure potentially flaky and/or eventually consistent operations for ACLs are retried appropriately. Fixes #6379 Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Amarin (Um) Phaosawasdi <[email protected]> Co-authored-by: shollyman <[email protected]> Co-authored-by: Noah Dietz <[email protected]> Co-authored-by: Chris Cotter <[email protected]> Co-authored-by: Eric Schmidt <[email protected]>
Adds support for BulkWriter to the Golang client.