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

Add System.Reactive.Compatibility as a dependency to DurableTask.Core #501

Merged
merged 2 commits into from
Jan 15, 2021

Conversation

ConnorMcMahon
Copy link
Contributor

@ConnorMcMahon ConnorMcMahon commented Jan 14, 2021

The Distributed Tracing work required adding System.Reactive.Core as a
dependency. However, if the customer app was using a different major
version of System.Reactive, it could lead to ambiguous compilation
issues.

Adding this package (and upgrading to use v4 of System.Reactive.Core)
should resolve this issue.

Addresses Azure/azure-functions-durable-extension#1620

Having different major versions of System.Reactive.Core and System.Reactive can lead to compilation confusions. Since this dependency does not appear to be required, we should remove it so that customers don't have to workaround this.
Copy link
Collaborator

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

Looking at the CI failures, it would appear that the correlation tracing work depends on System.Reactive.Core, no?

@ConnorMcMahon
Copy link
Contributor Author

Oddly it is building locally for me, but not in the CI, so I'm trying to understand why.

@cgillum
Copy link
Member

cgillum commented Jan 14, 2021

Where is the dependency on System.Reactive coming from?

@davidmrdavid
Copy link
Collaborator

I can confirm that System.Reactive.Core was added in the Dist.Tracing PR: https://github.com/Azure/durabletask/pull/422/files

@ConnorMcMahon
Copy link
Contributor Author

To clarify, it looks like CorrelationTraceClient uses some delegates that require types defined in System.Reactive (you can also get those types in System.Reactive.Core). It looks like we originally took on a dependency on System.Reactive.Core (v3) to address this.

The customer who encountered an issue was using v4 of System.Reactive, which leads to a compilation error. This is a well-known issue with using different major versions of System.Reactive.Core and System.Reactive.

Our options to make sure that customers don't run into this are:

  1. Remove our dependency on System.Reactive.Core if possible.
  2. Upgrade our System.Reactive.Core dependency to v4 (this would then break customers using v3 of System.Reactive)
  3. Include a reference to the System.Reactive.Compatibility package, which should resolve this issue.

I think either option 1 or 3 are viable, but since I am running into build issues with option 1, I'll probably just go with option 3.

The Distributed Tracing work required adding System.Reactive.Core as a
dependency. However, if the customer app was using a different major
version of System.Reactive, it could lead to ambiguous compilation
issues.

Adding this package (and upgrading to use v4 of System.Reactive.Core)
should resolve this issue.
@ConnorMcMahon ConnorMcMahon changed the title Remove unnecessary System.Reactive.Core dependency from DurableTask.Core Add System.Reactive.Compatibility as a dependency to DurableTask.Core Jan 15, 2021
Copy link
Collaborator

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

LGTM! Glad there already exists a compatibility layer :)

@ConnorMcMahon ConnorMcMahon merged commit 199380b into main Jan 15, 2021
@ConnorMcMahon ConnorMcMahon deleted the RemoveUnecessaryDependency branch January 15, 2021 23:53
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.

3 participants