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

Allow Netherite provider to be implemented separately #1541

Merged
merged 2 commits into from
Nov 20, 2020

Conversation

sebastianburckhardt
Copy link
Collaborator

Support for using the EventSourced backend, as in the work-in-progress PR Azure/durabletask#461.

Besides adding a durability provider, the changes include:

  • turn off message sorter logic for backend that delivers messages in order
  • support poll-free version of WaitForOrchestration for backends that support client responses
  • change tracing options for unit tests

@davidmrdavid davidmrdavid self-requested a review October 27, 2020 19:06
@davidmrdavid
Copy link
Contributor

@sebastianburckhardt , if this is WIP, should we consider making it a draft PR? :)

@sebastianburckhardt
Copy link
Collaborator Author

Not sure what the difference is between draft and WIP?

Since we just decided to restructure the projects I can't be quite sure what will change. Currently figuring this out. My guess is that in its eventual form, this PR will stay similar, as far as modifications to existing files are concerned, but it will not include the new durability provider.

@davidmrdavid
Copy link
Contributor

davidmrdavid commented Oct 27, 2020

@sebastianburckhardt , a draft is a GitHub UI feature to acknowledge that a PR is work-in-progress. I've used it in the past to make a clean separation between PRs that need immediate reviews for immediate merge consideration and the PRs that do not. You can read about it here:
https://github.blog/2019-02-14-introducing-draft-pull-requests/

Essentially if you CTRL+F "draft" in this page, you should be able to convert this into a draft PR. Just a suggestion! I can also do it for you if you'd like

@sebastianburckhardt
Copy link
Collaborator Author

This is now ready to be reviewed. It only contains the changes needed for implementing Netherite as an external NuGet package. The changes include:

  • turn off message sorter logic for backend that delivers messages in order
  • support poll-free version of WaitForOrchestration for backends that support client responses
  • add three protected methods to DurabilityProvider that allow implementations to access functionality that is otherwise internal

@sebastianburckhardt sebastianburckhardt changed the title [WIP] Netherite preview Allow Netherite provider to be implemented separately Oct 29, 2020
Copy link
Contributor

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me, I give it my thumbs-up but I do have suggestions for improvements, please see my comments.

I'll wait for a second pair of eyes to give this the approval 😄

@davidmrdavid
Copy link
Contributor

Ah it also seems like there's a merge conflict with src/WebJobs.Extensions.DurableTask/DurableTaskExtension.cs

@davidmrdavid davidmrdavid self-requested a review October 29, 2020 23:15
Copy link
Contributor

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

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

Really happy with the latest changes. LGTM! Still waiting on a 2nd pair of eyes just in case

@davidmrdavid
Copy link
Contributor

davidmrdavid commented Oct 30, 2020

Re-started the CI build checks since it appears to have been running for over an hour and thus timed-out. Sometimes this happens, for some reason, with the CI. Hoping they pass now!

@sebastianburckhardt sebastianburckhardt added this to the Extension v.2.4.0 milestone Nov 4, 2020
Copy link
Contributor

@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.

Overall looks fairly good, just a few questions.

src/WebJobs.Extensions.DurableTask/DurabilityProvider.cs Outdated Show resolved Hide resolved
src/WebJobs.Extensions.DurableTask/DurableTaskExtension.cs Outdated Show resolved Hide resolved
src/WebJobs.Extensions.DurableTask/DurabilityProvider.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

None yet

3 participants