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

feat(firestore): adds Bulkwriter support to Firestore client #5946

Merged
merged 92 commits into from
Jul 21, 2022

Conversation

telpirion
Copy link
Contributor

@telpirion telpirion commented Apr 26, 2022

Adds support for BulkWriter to the Golang client.

@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: firestore Issues related to the Firestore API. labels Apr 26, 2022
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels May 19, 2022
@product-auto-label product-auto-label bot added the stale: old Pull request is old and needs attention. label May 27, 2022
@telpirion telpirion requested a review from codyoss July 14, 2022 17:51
Copy link
Member

@enocom enocom left a 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.

firestore/bulkwriter.go Show resolved Hide resolved
firestore/bulkwriter.go Outdated Show resolved Hide resolved
firestore/bulkwriter.go Show resolved Hide resolved
@telpirion telpirion added the automerge Merge the pull request once unit tests and other checks pass. label Jul 19, 2022
@gcf-merge-on-green
Copy link
Contributor

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.

@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jul 20, 2022
// 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() {
Copy link
Member

@enocom enocom Jul 20, 2022

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 that isOpen is true, and then releases the lock
  • Goroutine 2 in End acquires the lock, sets isOpen 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.

Copy link
Contributor Author

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 a RWMutex.
  • Added a Lock() and deferred Unlock() 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.

@telpirion telpirion requested a review from enocom July 20, 2022 23:00
firestore/bulkwriter.go Outdated Show resolved Hide resolved
firestore/bulkwriter.go Outdated Show resolved Hide resolved
@enocom enocom self-requested a review July 21, 2022 16:19
@telpirion telpirion merged commit 20b6c1b into main Jul 21, 2022
@telpirion telpirion deleted the firestore-bulkwriter branch July 21, 2022 18:02
noahdietz added a commit that referenced this pull request Jul 21, 2022
* 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 <cjcotter@google.com>

* 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 <amchiclet@users.noreply.github.com>
Co-authored-by: shollyman <shollyman@google.com>
Co-authored-by: Noah Dietz <noahdietz@users.noreply.github.com>
Co-authored-by: Chris Cotter <cjcotter@google.com>
Co-authored-by: Eric Schmidt <erschmid@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the Firestore API. size: l Pull request size is large. stale: extraold Pull request is critically old and needs prioritization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants