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

Problems with AddAdoNetStorageProvider() extension method #3088

Closed
dandago opened this Issue Jun 5, 2017 · 11 comments

Comments

Projects
None yet
7 participants
@dandago
Copy link
Contributor

dandago commented Jun 5, 2017

The AddAdoNetStorageProvider() method:

  1. Does not provide a means of setting the AdoInvariant property, which is essential when not using SQL Server.
  2. Has a default serialization format of XML, whereas the default is usually binary. That means that if you switch between this method and the usual registration calls, you end up with deserialization errors.

Since changing the default value of a parameter is generally a bad idea, I suggest deprecating this method altogether.

@sergeybykov sergeybykov added this to the Triage milestone Jun 8, 2017

@sergeybykov

This comment has been minimized.

Copy link
Member

sergeybykov commented Jun 8, 2017

Tagging @shayhatsor and @veikkoeeva for their opinions, but also everybody who uses the SQL provider.

@veikkoeeva

This comment has been minimized.

Copy link
Contributor

veikkoeeva commented Jun 8, 2017

For 1. is an oversight and should be fixed. For 2. it mimicks that of other providers and I believe using binary as the default should be discouraged (tagging @jdom).

@jdom

This comment has been minimized.

Copy link
Member

jdom commented Jun 8, 2017

Yes, binary should be really discouraged. It was a bad thing to have it even as an option, but worse making it the default in our initial implementations of the providers.

@dandago

This comment has been minimized.

Copy link
Contributor

dandago commented Jun 9, 2017

@veikkoeeva I am not sure what you are pointing out in the link you shared, as the referenced code does not use XML as default.

The point here is that it is a breaking change for application code to switch between AddAdoNetStorageProvider() and RegisterStorageProvider() since the defaults are different.

I understand that binary as default is a bad idea, but having different defaults is worse in my opinion. This is confusing and error-prone for people using these APIs.

@sergeybykov sergeybykov modified the milestones: Triage, 2.0.0 Sep 22, 2017

@sergeybykov sergeybykov assigned ReubenBond and unassigned attilah Jan 6, 2018

@ReubenBond

This comment has been minimized.

Copy link
Contributor

ReubenBond commented Feb 2, 2018

Am I correct in understanding that to satisfy 1., we simply need to add an adoInvariant parameter which sets properties["AdoInvariant"]?

For 2., I don't think we should fix that - we should discourage the internal binary serializer for storage scenarios

@veikkoeeva

This comment has been minimized.

Copy link
Contributor

veikkoeeva commented Feb 2, 2018

Would this become deprecated as per #3952? Also related are #3957 and #3954 and especially #3908 and linked.

The new IoC would also mean ADO.NET provider does not need to take dependency to any of the current methods as anything the developer wants to can be loaded via the IoC. Technically, internally, currently an enumeration is used and based on that a serializer-deserializer pair is chosen that use a common interface. This could be reworked so that the user now directly choose the component used.

There are a few other API issues to think of too.

  1. ADO.NET divides storage to slots by type (currently binary, JSON and XML) due to in-storage efficiency reasons, but this is an implementation detail to the one who writes a (de)serializer wrapper and loads it in.

  2. ADO.NET storage can choose currently, internally, either the serializer or deserializer based on various parameters such as cluster ID, grain type, grain ID and whatever is in the state class (and if the user wants to provide some other logic). It would make sense to consider how to expose this. This could be used to migrate from binary to JSON, for instance, as checking the incoming data type in deserialization, choosing the right deserializer (binary) and the serialize everything in JSON.

@ReubenBond

This comment has been minimized.

Copy link
Contributor

ReubenBond commented Feb 6, 2018

@veikkoeeva yes, this is related to #3952. What do you propose the new APIs/options should look like?

@sergeybykov sergeybykov assigned jason-bragg and unassigned ReubenBond Feb 7, 2018

@sergeybykov sergeybykov modified the milestones: 2.0.0, 2.0.0-rc Feb 8, 2018

@veikkoeeva

This comment has been minimized.

Copy link
Contributor

veikkoeeva commented Feb 8, 2018

@ReubenBond I don't have a definite answer yet. I'm frankly over-extending myself a bit currently so I need a bit to think about this. If it helps, I would want to have easy and visible defaults to configure XML and JSON with defaults and customer settings. I would also want to have a way to easily expose the current canonical (de)serializers and see if the API surface is clear enough to communicate:

  1. One can wrap whatever that makes sense and then mark the tag approprioately (e.g. binary (e.g. Protobuf?), JSON (Jil?) or XML) and use that too.
  2. One can write custom logic to check the state going into database or coming from database to write whatever transformations are needed.
  3. It might be great if there were a mechanism, or some other way, to modify the configuration at runtime (resolving from DI container from every call, having a function and a lock, notification of some sort).
  4. The provider already does streaming to the other direction. I likely explore streaming to and from the grain at some point, which as discussed earlier, is basically patching/merging functionality.

The goal for 1., 2., and 3. is to make way for one to make it easier to mix and match (de)serialization decisions with great granularity. This will help fix "data bugs", refactor "data schema" (after all, even XML with a Schema would need to evolve at some point, or JSON without schema) and evolve the data in such ways as changing the storage format (e.g. make something more amenable to in-storage processing while compression some to "cold storage").

  1. Is a more future looking idea for bigger chunks of data.

I think the current APIs work pretty much as-is currently, but it would be great if some things, such as (de)serializers together with the container would be injected and maybe refactored if there are ideas. Especially the logic to choose a (de)serializer from the container I'd like to ponder a bit more than I did, but I suppose it's not on the core mainter roadmap, so I'm happy to do that later myself later if it's not completely out of picture. :)

@sergeybykov sergeybykov assigned ReubenBond and unassigned jason-bragg Feb 8, 2018

@sergeybykov

This comment has been minimized.

Copy link
Member

sergeybykov commented Feb 8, 2018

We need to decide ASAP if it's worth holding 2.0.0-rc1 for this.

@veikkoeeva

This comment has been minimized.

Copy link
Contributor

veikkoeeva commented Feb 8, 2018

@sergeybykov If it's possible to decide between using binary, JSON and XML, there isn't a need to hold this.

<edit: And one needs to be able to set the invariant to be able to load the right ADO.NET library, but that's possible in the legacy at least.

@sergeybykov

This comment has been minimized.

Copy link
Member

sergeybykov commented Feb 8, 2018

This will be handled as part of #3952.

@sergeybykov sergeybykov closed this Feb 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment