-
Notifications
You must be signed in to change notification settings - Fork 5
Add durable orchestration and entity examples #14
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
Conversation
Btw if it wasn't already clear - I think the durable files in this PR are essentially synonymous with the templates we will be using for tooling and I reviewed with that in mind |
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.
A few questions
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 think we're almost there.
Left some recommendations on naming, and a recommendation to simplify the getClient
method.
const id: string = req.params.id; | ||
const entityId = new df.EntityId(entityName, id); | ||
const client = df.getClient(context, clientInput); | ||
|
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 just realized: there's only 1 type of DF input bindings: the durableClient
.
With that in mind, do we really need this second parameter? Can we not just call df.getClient(context)
and have the method always retrieve the durableClient
within?
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.
With that in mind, do we really need this second parameter?
Sadly, yes. Under the hood, df.getClient()
calls context.getInput()
, which expects an input binding as an argument (clientInput
in this case), so the functions needs to also pass that argument to the SDK. In the old model, this wasn't as necessary because all the bindings would live inside the context
, so the SDK could just loop over all of them until it finds the client input binding, but that is no longer the case in the new model
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.
This would be another advantage of something like df.client()
that I had before, because the SDK would be managing the durable client input binding on behalf of the user and the user doesn't have to worry about it or pass it to the SDK or even know it exists
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 see. At this time, this is my biggest pain point with the new programming model. Moving forward, I would like for us to prioritize removing this boilerplate, as it is rather clunky and DF apps always need a client.
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 agree with this 100%. But I think we agreed that that shouldn't be in this first iteration. Let me create an issue for this on the Durable SDK to track this so we don't block this PR.
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.
In the old model, this wasn't as necessary because all the bindings would live inside the context, so the SDK could just loop over all of them until it finds the client input binding, but that is no longer the case in the new model
Ftr, the new model could do the same thing where it loops over the context to find the durable client. The library package might not allow you to loop over bindings right now, but we can always change it. That may be helpful outside of durable as well, which you should always keep an eye out for.
If we can come up with a fancy new design to improve on the client experience, that's great - but in the interest of time we should keep the "match the old model" option on the table.
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 think the new programming makes sense, though I remain concerned about the boilerplate of df.getClient
which I believe should be able to retrieve a "durableClient"
binding without any parameters. I would like for us to prioritize providing an alternative, less verbose, alternative.
However, for the sake of gathering feedback on the entirety of the experience, I'm satisfied with the proposed samples.
Approving, but per offline discussion please merge to a durable-specific branch instead of main |
No description provided.