Lock Module Implementation #1567
Conversation
|
Hello @jackyzha0, Thanks for updating this PR. There are currently no PEP 8 issues detected in this PR. Cheers! 🍻 Comment last updated at 2021-05-17 18:44:37 UTC |
Codecov Report
@@ Coverage Diff @@
## master #1567 +/- ##
==========================================
- Coverage 68.72% 65.75% -2.98%
==========================================
Files 154 164 +10
Lines 10255 11554 +1299
==========================================
+ Hits 7048 7597 +549
- Misses 3207 3957 +750
Continue to review full report at Codecov.
|
bentoml/yatai/db/__init__.py
Outdated
| @@ -99,7 +102,8 @@ def create_all_or_upgrade_db(self): | |||
| inspector = inspect(self.engine) | |||
| tables = inspector.get_table_names() | |||
|
|
|||
| if 'deployments' not in tables or 'bentos' not in tables: | |||
| table_names = ['deployments', 'bentos', 'labels', 'locks'] | |||
| if not all([table_name in tables for table_name in table_names]): | |||
There was a problem hiding this comment.
This should be "only create table if none of the table exist", right?
if all([table_name in tables for table_name not in table_names]):
bentoml/yatai/db/stores/lock.py
Outdated
|
|
||
| @staticmethod | ||
| def release(sess, resource_id): | ||
| lock = LockStore._find_lock(sess, resource_id) |
There was a problem hiding this comment.
It doesn't verify the owner of the lock. A worker may release the lock held by others.
There was a problem hiding this comment.
I was thinking that we don't expose this acquire/release API to users and only lock using the context manager in yatai/locking/lock.py. That way, we don't need to store lock owner in the table.
| with db.create_session() as sess: | ||
| lock_objs = [] | ||
| # acquire all locks in lock list | ||
| for (lock_identifier, lock_type) in locks: | ||
| lock_obj = LockStore.acquire( | ||
| sess, lock_type, lock_identifier, ttl_min | ||
| ) | ||
| lock_objs.append(lock_obj) | ||
|
|
||
| # try to commit all locks to db | ||
| sess.commit() |
There was a problem hiding this comment.
session is created on line 59 and changes are finally commited on line 69. this may cause lock contention if another yataiservice tries to acquire a lock in the time that the for loop runs for
tests/integration/test_locking.py
Outdated
|
|
||
| def run_delayed_thread(t1, t2): | ||
| t1.start() | ||
| time.sleep(1) |
There was a problem hiding this comment.
anything less than 0.5s here causes some instability in the guarantee that t1 acquires lock before t2 here (related to concern above)
* fix: makefile sphinx deps * docs: added batch_serving docs and linked to it in quickstart * refactor database operations into db class * remove old db code * migrate store related functionality to yatai/db/stores module * makefile + sources typo * refactor to static classes * finish static refactor * fix yatai_service_impl * fix bad patch in test_usage_stats * patch metadatastore property naming bug * lint + format * more lint + fmt * lint + format (again) * add base lock table and orm impl * add locking module impl with retry * wrap yatai service with Lock * fix retry mechanism * add proper lock release * lint + fmt * ttl implementation * lint + fmt * fix broken label test and added ability to pass sesison to lock fn * multilock support * lint + fmt (again) * whitespace fix * add better documentation * line shortening and more docstrings * base multithreaded test * clean up lock test * linter pass * fix up tests * unit test for expiring/lock * lint + fmt
Description
Closes #1540
Motivation and Context
See #1540
How Has This Been Tested?
Types of changes
Component(s) if applicable
Checklist:
./dev/format.shand./dev/lint.shscript have passed(instructions).