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.

Sign up for GitHub

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

Jump to bottom

Call EnsureLegalAccess from EntityFeature in dotnet-isolated #2633

Merged
merged 4 commits into from
Oct 10, 2023
Merged

Call EnsureLegalAccess from EntityFeature in dotnet-isolated #2633

merged 4 commits into from
Oct 10, 2023

Conversation

jviau
Copy link
Contributor

@jviau jviau commented Oct 10, 2023 •
edited
Loading

Removes a check that asserts the TaskOrchestrationContext was accessed. It does not add much value and instead causes issues:

  1. As we add extensions / extra functionality to the TaskOrchestrationContext, we have to ensure everything is wrapped to make the IsAccessed=true call. This is cumbersome.
  2. An orchestration could validly have 0 work off the context and be purely computational based on its inputs, we should not block this.
    1. e.g: an orchestration could follow a branch that based on the input it decides there is 0 work to be done and just returns.

Issue describing the changes in this PR

resolves #issue_for_this_pr

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation PR is ready to merge and referenced in pending_docs.md
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)
  • My changes do not require any extra work to be leveraged by OutOfProc SDKs
    • Otherwise: That work is being tracked here: #issue_or_pr_in_each_sdk
  • My changes do not change the version of the WebJobs.Extensions.DurableTask package
    • Otherwise: major or minor version updates are reflected in /src/Worker.Extensions.DurableTask/AssemblyInfo.cs
  • My changes do not add EventIds to our EventSource logs
    • Otherwise: Ensure the EventIds are within the supported range in our existing Windows infrastructure. You may validate this with a deployed app's telemetry. You may also extend the range by completing a PR such as this one.
  • My changes should be added to v3.x branch.
    • Otherwise: This change only applies to Durable Functions v2.x and will not be merged to branch v3.x.

@jviau jviau requested a review from cgillum October 10, 2023 20:27
@jviau jviau changed the base branch from dev to feature/core-entities October 10, 2023 20:27
@jviau jviau changed the title Remove async check Remove orchestration async check in dotnet isolated Oct 10, 2023
@cgillum
Copy link
Member

cgillum commented Oct 10, 2023

Just to make sure I understand, can you provide a concrete example of this?

an orchestration could follow a branch that based on the input it decides there is 0 work to be done and just returns.

The closest thing I can think of is when an orchestration is waiting on an external event, in which case it does access the context, but what's another scenario you had in mind where the code doesn't access the context?

@jviau
Copy link
Contributor Author

jviau commented Oct 10, 2023

Just to make sure I understand, can you provide a concrete example of this?

an orchestration could follow a branch that based on the input it decides there is 0 work to be done and just returns.

The closest thing I can think of is when an orchestration is waiting on an external event, in which case it does access the context, but what's another scenario you had in mind where the code doesn't access the context?

I believe there are cases where a customer could reasonably write an orchestration that never accesses a context. I don't have a concrete example, but I could see an orchestration being written where it examines the input and decides there is no work to be done, returning immediately. I think overall this check is unnecessarily restrictive without much benefit.

  1. We will still throw based on the thread changing.
  2. This error is not even descriptive for why it happens. When I saw the error, I was confused why it was happening.
  3. The check does not even work that well, we saw customers with async middleware and it would fail when they accessed the context. This check did not even catch that.

sebastianburckhardt
sebastianburckhardt approved these changes Oct 10, 2023
View reviewed changes
Copy link
Collaborator

@sebastianburckhardt sebastianburckhardt left a comment

Choose a reason for hiding this comment

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

some minor comments, looks good to me.

release_notes.md Show resolved Hide resolved
src/Worker.Extensions.DurableTask/FunctionsOrchestrator.cs Outdated Show resolved Hide resolved
@jviau
Copy link
Contributor Author

jviau commented Oct 10, 2023

I added the async check back in, but made a wrapper to TaskOrchestrationEntityFeature to call EnsureLegalAccess. We can evaluate removing this check separately.

@jviau jviau changed the title Remove orchestration async check in dotnet isolated Call EnsureLegalAccess from EntityFeature in dotnet-isolated Oct 10, 2023
@jviau jviau merged commit cc0d0ed into Azure:feature/core-entities Oct 10, 2023
sebastianburckhardt added a commit that referenced this pull request Oct 19, 2023
@sebastianburckhardt @bachuv @jviau
Merge feature/core-entities to dev (#2574)
bbd7ad6 
* udpate readme.

* update durability provider class for new core-entities support. (#2570)

* update durability provider class for new core-entities support.

* add configuration setting for max entity concurrency to DurableTaskOptions

* minor fixes.

* update DurableClient to take advantage of native entity queries (#2571)

* update DurableClient to take advantage of native entity queries if available

* fix minor errors.

* address PR feedback

* implement passthrough middleware for entities (#2572)

* implement passthrough middleware for entities.

* propagate changes to protocol

* update/simplify protobuf format

* address PR feedback

* implement entity queries for grpc listener (#2573)

* implement entity queries for grpc listener

* propagate changes to protocol

* update/simplify protobuf format

* Various fixes (#2585)

* durability provider must implement and pass-through IEntityOrchestrationService since it wraps the orchestration service

* simple mistake

* fix misunderstanding of initializer syntax (produced null, not empty list)

* fix missing failure details

* fix missing compile-time switch for trigger value type

* fix missing optional arguments

* fix  missing override

* simplify how entities are excluded from instance queries (#2586)

* add an entity example to the DotNetIsolated smoke test project. (#2584)

* add an entity example to the DotNetIsolated smoke test project.

* remove superfluous argument.

* address PR feedback

* Entities: Add worker side entity trigger and logic (#2576)

* Add worker side entity trigger and logic

* update comments

* Address PR comments

* another small fix that got lost somewhere. (#2596)

* Update packages and version for entities preview (#2599)

* Switch to Microsoft.DurableTask.Grpc (#2605)

* Fix grpc core (#2616)

* pass entity parameters for task orchestration. (#2611)

* Core entities/various fixes and updates (#2619)

* assign the necessary AzureStorageOrchestrationServiceSettings

* propagate changes to query name and metadata parameters

* add missing override for TaskOrchestrationEntityFeature

* Update to entities preview 2 (#2620)

* Add callback handler for entity dispatching (#2624)

* Core entities/propagate changes (#2625)

* add configuration for EnableEntitySupport

* rename includeStateless to includeTransient

* Rev dependencies to entities-preview.2 (#2627)

* Call EnsureLegalAccess from EntityFeature in dotnet-isolated  (#2633)

* create a better error message in situations where client entity functions are called on a backend that does not support entities (#2630)

* Rev package versions, update release notes (#2638)

* Address smoke test build issue (#2647)

* fix translation of legacy query to new entity query support (#2648)

* fix translation of legacy query to new entity query support

* comment out CleanEntityStorage_Many

* try to enable CI on feature branch

* Revert "comment out CleanEntityStorage_Many"

This reverts commit aeaa4b8.

* update to preview.2 packages

---------

Co-authored-by: Varshitha Bachu <[email protected]>
Co-authored-by: Jacob Viau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgillum cgillum cgillum left review comments

@sebastianburckhardt sebastianburckhardt sebastianburckhardt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jviau @cgillum @sebastianburckhardt