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

Durable Entities #184

Merged
merged 35 commits into from
Dec 3, 2020
Merged

Durable Entities #184

merged 35 commits into from
Dec 3, 2020

Conversation

davidmrdavid
Copy link
Collaborator

@davidmrdavid davidmrdavid commented Aug 25, 2020

Addresses: #96

Durable Entities for Python

This PR enables Durable Entities for Python, enabling our users to manage small pieces of state, the Durable way.

At a high-level, this PR includes a handful of changes:

  1. It introduces a whole set of Entity utility classes, in the /models/entities/ directory, which help to encapsulate the different kinds of objects that the durable-extension returns. For the most part, these resemble their DurableJS counterparts.
  2. It specifies the a syntax for declaring Entities in python, which you can find in our new Counter-based sample.
  3. It introduces a handful of TODO comments over the codebase, based on observations I made while studying the DurableJS implementation of Entities. I want to keep these TODOs as they specify areas of the codebase that need more thought and design, but that are left as-is because they match the behaviour of JS. Admittedly, some other ToDos may just be actually leftover tasks.
  4. Introduces a very simple, barebones, unit test for Durable Entities. We'll want to test this feature more throughly than it currently is, but this PR takes a first stab. In the future, we'll want to the not just the DurableOrchestrationContext as it makes calls to Durable Entities, but also a DurableEntityContext as it receives mocked requests from orchestrators.

Finally, these changes are dependent on changes on the Python worker that have not been merged yet. Those changes are in this small PR: Azure/azure-functions-python-library#68 . In any case, we do not want to merge this until that PR is merged.

@davidmrdavid
Copy link
Collaborator Author

Bug found:
Ah, I just found one bug where I'm not looking up correctly the result of a callEntity in the state array. Oh well, that's one for tomorrow :)

@davidmrdavid
Copy link
Collaborator Author

Bug fixed 🛩️

Copy link

@ConnorMcMahon ConnorMcMahon left a comment

Choose a reason for hiding this comment

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

Very good start to this.

Since we have a bit of time before this merges, I would like to see some units tests for the entity itself instead of just the call_entity API for the orchestration. I would also like to see some basic signals working if at all possible.

azure/durable_functions/entity.py Outdated Show resolved Hide resolved
bool
True if the Entity was newly constructed. False otherwise.
"""
# TODO: not updating this atm

Choose a reason for hiding this comment

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

Not sure if this even gets updated when C# handles a batch either, so this may not be necessary. We should consult Sebastian about this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sebastianburckhardt:

Is there a use for a isNewlyConstructed flag for entities? Or can it be safely removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll be tracking this here: #222

azure/durable_functions/models/DurableEntityContext.py Outdated Show resolved Hide resolved
self._exists = True
self._result = result

def destruct_on_exit(self) -> None:

Choose a reason for hiding this comment

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

I'm not sure how we handle this in JS or C#, but there is the question of how to handle subsequent operations after this within a batch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's check-in with the team then 😉
@cgillum , @sebastianburckhardt:

What are the semantics for a destructed entity (via destructOnExit) when there are still operations to perform on a batch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll be tracking this here: #222

azure/durable_functions/models/utils/entity_utils.py Outdated Show resolved Hide resolved
@wenhzha wenhzha mentioned this pull request Oct 6, 2020
@davidmrdavid davidmrdavid mentioned this pull request Nov 13, 2020
2 tasks
@davidmrdavid
Copy link
Collaborator Author

I believe this is ready for merge!

carlvitzthum and others added 4 commits November 18, 2020 15:39
Co-authored-by: cvitzthum-nasuni <cvitzthum@nasuni.com>
Create links for sample folder. Co-authored with @kemurayama
* Replace datetime.strptime

* Deleted unnecessary format string

* Delete whitespace
Copy link

@ConnorMcMahon ConnorMcMahon left a comment

Choose a reason for hiding this comment

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

Mostly nits, but a few of these I would want addressed before we merge.

Overall, exciting stuff though!

azure/durable_functions/entity.py Outdated Show resolved Hide resolved
azure/durable_functions/entity.py Show resolved Hide resolved
azure/durable_functions/entity.py Outdated Show resolved Hide resolved
azure/durable_functions/entity.py Outdated Show resolved Hide resolved
azure/durable_functions/models/DurableEntityContext.py Outdated Show resolved Hide resolved
azure/durable_functions/orchestrator.py Show resolved Hide resolved
Copy link

@ConnorMcMahon ConnorMcMahon left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

5 participants