Skip to content
This repository has been archived by the owner on Aug 2, 2023. It is now read-only.

Further apply read-only txn hints #529

Merged
merged 5 commits into from
Feb 14, 2022
Merged

Conversation

achimnol
Copy link
Member

@achimnol achimnol commented Feb 3, 2022

No description provided.

@achimnol achimnol added this to the 21.03 milestone Feb 3, 2022
@achimnol achimnol self-assigned this Feb 3, 2022
@codecov
Copy link

codecov bot commented Feb 3, 2022

Codecov Report

Merging #529 (4995b87) into main (20f748b) will decrease coverage by 0.00%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #529      +/-   ##
==========================================
- Coverage   48.88%   48.87%   -0.01%     
==========================================
  Files          54       54              
  Lines        9001     9002       +1     
==========================================
  Hits         4400     4400              
- Misses       4601     4602       +1     
Impacted Files Coverage Δ
src/ai/backend/manager/api/events.py 35.90% <0.00%> (ø)
src/ai/backend/manager/api/utils.py 58.29% <0.00%> (ø)
src/ai/backend/manager/registry.py 16.99% <0.00%> (ø)
src/ai/backend/manager/api/auth.py 47.91% <40.00%> (-0.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20f748b...4995b87. Read the comment docs.

@achimnol achimnol requested a review from kyujin-cho February 7, 2022 04:45
Comment on lines -445 to +446
await pipe.execute()
return pipe
Copy link
Member

Choose a reason for hiding this comment

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

I am just curious.
is this a big change or difference?

Copy link
Member Author

@achimnol achimnol Feb 8, 2022

Choose a reason for hiding this comment

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

common.redis.execute() function accepts either a lambda to return an awaitable for Redis operation or a Redis pipeline object.
This change is to make it clearer to pass the pipeline object instead of an awaitable, though the final result is the same: executing the pipeline vs. executing an awaitable that executes the pipeline. Just a matter of style, but it reduces one extra function call.

Comment on lines 925 to 928
# TODO: Remove `type: ignore` when mypy supports type inference for walrus operator
# Check https://github.com/python/mypy/issues/7316
# TODO: remove `NOQA` when flake8 supports Python 3.8 and walrus operator
# Check https://gitlab.com/pycqa/flake8/issues/599
Copy link
Member

Choose a reason for hiding this comment

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

Though this is not related to, since this PR is changing this code block: both python/mypy#7316 and PyCQA/flake8#323 (migrated from Gitlab issue) is both resolved and released, I think it'll be good to remove all #noqa annotations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! 👍🏼

src/ai/backend/manager/api/session.py Outdated Show resolved Hide resolved
src/ai/backend/manager/api/session.py Outdated Show resolved Hide resolved
@achimnol achimnol requested a review from kyujin-cho February 8, 2022 13:12
Copy link
Member

@kyujin-cho kyujin-cho left a comment

Choose a reason for hiding this comment

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

LGTM!

@achimnol achimnol merged commit 79cdac7 into main Feb 14, 2022
@achimnol achimnol deleted the fix/apply-missing-readonly-txn branch February 14, 2022 04:45
achimnol added a commit that referenced this pull request Feb 14, 2022
* fix: Apply read-only txn hint when reading allowed docker registry info
* fix: Apply the readonly attribute to more transactions
* fix: Restore skipped lint checks due to old flake8/mypy no longer used

Backported-From: main (22.03)
Backported-To: 21.09
achimnol added a commit that referenced this pull request Feb 14, 2022
* fix: Apply read-only txn hint when reading allowed docker registry info
* fix: Apply the readonly attribute to more transactions
* fix: Restore skipped lint checks due to old flake8/mypy no longer used

Backported-From: main (22.03)
Backported-To: 21.03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants