Skip to content

Add state manager context#240

Merged
wcs1only merged 2 commits intodapr:masterfrom
berndverst:master
Jul 21, 2021
Merged

Add state manager context#240
wcs1only merged 2 commits intodapr:masterfrom
berndverst:master

Conversation

@berndverst
Copy link
Copy Markdown
Member

@berndverst berndverst commented Jul 16, 2021

Description

Adds context to actor state manager

Issue reference

Closes: #237

Adds #237. Required feature for #219

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@berndverst
Copy link
Copy Markdown
Member Author

@wcs1only @halspang could you take a first look at this?

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 16, 2021

Codecov Report

Merging #240 (b59d7c6) into master (e43159b) will increase coverage by 0.16%.
The diff coverage is 98.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #240      +/-   ##
==========================================
+ Coverage   88.55%   88.71%   +0.16%     
==========================================
  Files          52       52              
  Lines        2071     2101      +30     
==========================================
+ Hits         1834     1864      +30     
  Misses        237      237              
Impacted Files Coverage Δ
dapr/actor/runtime/state_manager.py 89.38% <98.24%> (+1.64%) ⬆️
dapr/actor/runtime/manager.py 93.97% <100.00%> (+0.46%) ⬆️

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 e43159b...b59d7c6. Read the comment docs.

@berndverst berndverst marked this pull request as ready for review July 19, 2021 16:24
@berndverst berndverst requested a review from a team as a code owner July 19, 2021 16:24
Copy link
Copy Markdown

@halspang halspang left a comment

Choose a reason for hiding this comment

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

Overall it looks fine.

It would be great to get an e2e/integration test that shows this works. You could do something like this:

ActorA.methodOne - add state "state" (do not commit)
ActorA.methodTwo - get state "state"
ActorA.methodTwo - mutate "state"
ActorA.methodTwo - commit "state"
ActorA.methodOne - do nothing

Then check that only the state from methodTwo had it's state committed showing it was isolated from methodOne.

state_change_tracker[state_name].change_kind == StateChangeKind.remove

def _get_contextual_state_tracker(self) -> Dict[str, StateMetadata]:
context = CONTEXT.get(None)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What does getting None do?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's the default value it returns if this context variable wasn't set at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement State Isolation for Reentrant Actor Calls

3 participants