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

[WIP] Improved robustness of concurrent schema updates #3186

Closed
wants to merge 36 commits into from

Conversation

brimoor
Copy link
Contributor

@brimoor brimoor commented Jun 12, 2023

As explained in the @todo, this is a bit of a hack to specifically avoid issues when concurrently modifying a dataset's schema. However, the underlying problem still exists whenever list fields other than DatasetDocument.sample_fields and DatasetDocument.frame_fields are concurrently edited without first reloading.

I think it makes sense to go ahead and merge this particular patch because schema updates are by far the most likely case where concurrent list edits may arise, since in many workflows dataset objects may be held in-memory for long periods of time without reloading them. Unlike samples, which are generally only loaded + modified on-demand.

@brimoor brimoor added the bug Bug fixes label Jun 12, 2023
@brimoor brimoor requested review from allenleetc and a team June 12, 2023 04:05
@brimoor brimoor self-assigned this Jun 12, 2023
@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (d0c3dff) 15.56% compared to head (4ad5b8f) 15.56%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3186   +/-   ##
========================================
  Coverage    15.56%   15.56%           
========================================
  Files          564      564           
  Lines        69319    69319           
  Branches       681      681           
========================================
  Hits         10791    10791           
  Misses       58528    58528           
Flag Coverage Δ
app 15.56% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@brimoor brimoor changed the title Improved robustness of concurrent schema updates [WIP] Improved robustness of concurrent schema updates Jun 12, 2023
@allenleetc
Copy link
Contributor

Still orienting but LGTM. One question -- theoretically race conditions are still possible as the dataset doc is not locked between reload() and the subsequent schema update (not sure exactly where that happens)?

@brimoor
Copy link
Contributor Author

brimoor commented Jun 12, 2023

Still orienting but LGTM. One question -- theoretically race conditions are still possible as the dataset doc is not locked between reload() and the subsequent schema update (not sure exactly where that happens)?

yes theoretically there still could be issues if two processes update the schema within the few lines of code between the reload() I added here and when the db transaction has happened. However, this is substantially better than status quo, where this issue can happen if any concurrent updates happen between the time when the dataset is loaded in one process and when a schema change is made (which could be hours or days).

I'd like us to properly address this issue as per #3187; ;this is just attempting to place a bandaid on the most likely case.

allenleetc
allenleetc previously approved these changes Jun 12, 2023
@brimoor brimoor marked this pull request as draft June 12, 2023 15:02
@voxel51 voxel51 deleted a comment from allenleetc Jun 14, 2023
@brimoor brimoor force-pushed the bugfix/iss-3185 branch 2 times, most recently from a55af08 to 244a959 Compare June 18, 2023 14:16
Base automatically changed from release/v0.21.1 to main June 30, 2023 20:18
@brimoor brimoor force-pushed the bugfix/iss-3185 branch from ac766ab to 7f21f2c Compare July 1, 2023 04:41
benjaminpkane and others added 13 commits July 5, 2023 13:22
* update sidebar test, fix small regressions (#3250)

* remove next index ref, fix string filter is matching default (#3249)

* release notes

* package bumps

* disabled e2e

* lint

* fix mac arm64

* Teams v1.3.2 release note

* rm e2e

* add main
* kill server on session close

* close desktop app

* cleanup

* lint

* lint

* is_open

* rm remote warning
* add frame cases to dynamic label tests

* embedded frame label fixes

* linting
brimoor and others added 20 commits July 10, 2023 18:14
* only matches

* db_field bug and path fix
* don't throw error

* add debug msg
* fixing filter_keypoints() bug

* adding dynamic doc test
* only matches

* db_field bug and path fix

* base image sample tests

* cleanup

* exclude bug

* rm onlyMatch

* frame and dynamic tests

* adding coverage

* keypoints fixes

* cleanup

* base image sample tests

* cleanup

* exclude bug

* rm onlyMatch

* frame and dynamic tests

* adding coverage

* keypoints fixes

* cleanup

* tweaks

* exclude no only matches
@brimoor brimoor changed the base branch from main to develop July 13, 2023 00:17
@brimoor
Copy link
Contributor Author

brimoor commented Jul 17, 2023

Closing in favor of #3308.

@brimoor brimoor closed this Jul 17, 2023
@brimoor brimoor deleted the bugfix/iss-3185 branch July 17, 2023 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants