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

Fix usage of assembly (fix db context class discovery) #16

Closed
morrisjdev opened this issue Jul 23, 2020 · 5 comments
Closed

Fix usage of assembly (fix db context class discovery) #16

morrisjdev opened this issue Jul 23, 2020 · 5 comments
Labels
enhancement New feature or request todo

Comments

@morrisjdev
Copy link
Collaborator

The current sync manager uses Assembly.GetEntryAssembly() to find the DbContext-Type used during a transfer of a change.
This makes in impossible to define the DbContext in a different assembly than the Entry Assembly.

It should be possible to use a different Assembly. A way to avoid this issue is to all registered DbContext-Types in a storage class.

@morrisjdev morrisjdev added enhancement New feature or request todo labels Jul 23, 2020
@morrisjdev morrisjdev added this to the Feature development milestone Jul 23, 2020
@morrisjdev
Copy link
Collaborator Author

Fixed. Will be published in version 2.2.2

@hwanders
Copy link

I hate to be the bad guy, but I would like you to remind that changing the public API (SendChangesRequest) by renaming a property is a breaking change as it requires all clients to update to a newer version which also changed that property.

If you are not doing semantic versioning though, everything should be fine.

@morrisjdev
Copy link
Collaborator Author

Definitely not the bad guy. Like to see you reviewing my commits.

This specific API is intended to be used by two or more equal deployments of the server application to sync changes between them.

But yes you are right it could break the server application during rollout of an update.

I think I'll add it in the next minor release.

@hwanders
Copy link

Thank you for the clarification, I'm just getting acquainted to the code base.

It's fine if it's just an internal API, I thought it was an API directly if indirectly accessible by the actual clients (because I wasn't looking deep enough).

My thoughts regarding updates in a cluster:

For a smooth upgrade path you might try to leave the old property in the request class for a while and cope with only one of them being set or create a new class for the new request altogether (and handle both separately).
The next version can then remove the old property.

Of course that requires the users to perform a clean migration via one intermediate version with the behavior described above.

@morrisjdev
Copy link
Collaborator Author

Yes that is a good idea but after considering carefully I decided to not add the additional property.

The benefit is too small and it would make less sense when you're rolling out an update because the server applications would not have to be synced on startup anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request todo
Projects
None yet
Development

No branches or pull requests

2 participants