-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add support for isolated entities #358
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions but overall looks good
<PackageReference Include="Microsoft.Azure.WebJobs.Extensions.DurableTask" Version="2.12.0" /> | ||
<PackageReference Include="Microsoft.Azure.WebJobs.Extensions.DurableTask" Version="2.13.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to first release the current DF Extension release prior to getting this out, and then update this dependency to that upcoming release. We had to remove the latest DF Extension release due to a regression in OOProc serialization, so I'd like to avoid upgrading Netherite to depending on that faulty extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, maybe that means we should not include this PR in the 1.5.0 release?
Since we probably want this out sooner than later to make progress on the "partition corruption" theme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we probably want the fix in Azure/azure-functions-durable-extension#2748 to be included so I think it makes sense to wait for the next DF Extension release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, maybe that means we should not include this PR in the 1.5.0 release?
Yeah I'd say let's hold off, but prioritize this release shortly after.
|
||
internal bool IsSet => this.HasRuntimeStatus || !string.IsNullOrWhiteSpace(this.InstanceIdPrefix) | ||
|| !(this.CreatedTimeFrom is null) || !(this.CreatedTimeTo is null); | ||
|| !(this.CreatedTimeFrom is null) || !(this.CreatedTimeTo is null) || this.ExcludeEntities; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does IsSet
represent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means "this query has some filtering going on". I will rename it to make that clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually looks like this property is not used anywhere (anymore?) so I will remove it altogether.
/// </summary> | ||
public int MaxConcurrentOrchestratorFunctions { get; set; } = 100; | ||
|
||
/// <summary> | ||
/// Gets or sets the maximum number of work items that can be processed concurrently on a single node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's make this more precise by saying "entity" explicitly
/// Gets or sets the maximum number of work items that can be processed concurrently on a single node. | |
/// Gets or sets the maximum number of Entity work items that can be processed concurrently on a single node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the comments for activities, entities, and orchestrations.
if (request.ReleaseOrphanedLocks && status.LockedBy != null) | ||
{ | ||
tasks.Add(CheckForOrphanedLockAndFixIt(state, status.LockedBy)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have we always had this functionality of releasing orphaned locks? Maybe I missed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not "always", but it has been there for a long time now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, we were always planning to call this automatically every once in a while, but never actually implemented that since it always seemed like it should be done by the same periodic task that does the autopurge, and we haven't gotten around to that one yet.
// list all entities (without fetching the input) and for each one that requires action, | ||
// perform that action. Waits for all actions to finish after each page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you clarify this bit about "requiring an action"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rewrote the comments for this function. Also, a bit of simplification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM though there'a conflict in the csproj
/// </summary> | ||
[DataMember] | ||
internal bool ExcludeEntities { get; set; } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Extra line break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, just removed it: a6a210e
...DurableTask.Netherite.AzureFunctions.Tests/DurableTask.Netherite.AzureFunctions.Tests.csproj
Show resolved
Hide resolved
...DurableTask.Netherite.AzureFunctions.Tests/DurableTask.Netherite.AzureFunctions.Tests.csproj
Show resolved
Hide resolved
FYI @sebastianburckhardt that I went ahead and merged this (which I had approved some time back) after resolving some merge conflicts in the csproj. |
@sebastianburckhardt @davidmrdavid Still getting Logged an issue: #390 |
@jaliyaudagedara: Thanks - seems the AssemblyInfo.cs file, which needs to be manually updated on every release, was left intact. As a result, the host process (separate from the .NET isolated process) was loading the wrong version of Netherite, which explains this issue. I'm fixing that here: #393 |
@davidmrdavid , got it! thanks for the fix! |
Adds support for the new entity implementation used by dotnet-isolated.
Also, updates the DF and DT dependencies to 2.13.1 and 2.16.1.