ADO.NET Refactoring

[JorgeCandeias]

This is a first pass at refactoring the ADO.NET provider projects.

This pass focuses on freeing the code from the compiler directives in linked files, while moving common code to a core project.

This core project is still tightly coupled with the other ADO.NET projects as of now.

There is no effort yet to integrate with DI, that's left for a second pass.

While this PR is very intrusive, it is not meant to have any breaking changes.

Microsoft Reviewers: Open in CodeFlow

[veikkoeeva]

Hey, @JorgeCandeias looks good! I'm all good for removing the pragmas and modernizing the C#.

One thing to note is that if you prefer, for instance e.g. https://github.com/dotnet/orleans/pull/9024/files#diff-454dbc7924a5a6ce62641db0fd5bc4429ec52a78c0e63f44b6c3b0e7b156541e could be basically the queries in a dictionary. I mention this since then it likely would be easier to actually supply them through options e.g. in startup or later do differently altogether (this was the original way).

[JorgeCandeias]

@veikkoeeva Thanks for reviewing! Moving the query keys to an options class is on the list for the next PR, along with moving services to DI. In addition, the ability to specify the full schema/name for the queries table itself needs to be added. Let's do one thing at a time so the PRs are easier to review.

[ReubenBond]

This requires that the API surface for this project won't change, unless developers are careful to always upgrade these packages in-sync with each other. It doesn't necessarily require changing, but it's worth noting that this could introduce a paper cut for consumers.

[JorgeCandeias]

This stuff is here because the code is still tightly coupled. This will go away on the next clean up, as the services are decoupled into DI. I didn't want to include that effort now or the diff for this PR would become even more unreadable.

[benjaminpetit]

What's the plan to get rid of these dependencies?

[JorgeCandeias]

What's the plan to get rid of these dependencies?

As per this comment:

• This is PR 1 of 3 for refactoring the adonet codebase. The intent is to remove all the compiler directives and move shared code to a core project to enable further improvements. No user breaking changes are intended.

• Step 2 is to move the core classes to Keyed DI under appropriate interfaces. This will allow these reverse dependencies to be removed. No user breaking changes are intended. However more configuration options will become available.

• Step 3 is to finally modernize the C# code, reduce overhead where possible, etc.

I had intended to split the above work in 3 steps to reduce review burden and make bugs easier to spot.
Let me know if you prefer a big bang PR instead.

[veikkoeeva]

@veikkoeeva Thanks for reviewing! Moving the query keys to an options class is on the list for the next PR, along with moving services to DI. In addition, the ability to specify the full schema/name for the queries table itself needs to be added. Let's do one thing at a time so the PRs are easier to review.

Sounds like a plan. To give a bit of history (this has been also in Gitter and now Discord, but a few words here too): It could be useful to be able to change queries or even storage layout if data traffic patterns or storage and processing changes. As in getting more data or more I/O capacity on the fly and if its possible to react without downtime. So consequently, not only bring things like this into startup options but to the DI to be changed on the fly. Some industrial and other uses cases are a bit hard to run down and preferably one can avoid that to fix or change things. This hasn't been the Orleans focus but might be useful to know.

[JorgeCandeias]

It could be useful to be able to change queries or even storage layout if data traffic patterns or storage and processing changes.

We can start by making the query entries configurable via some options class. Later on we can expose some interface to allow the user to bypass the fixed-signature query stuff altogether. The reason is that each database engine can provide specific features that can better implement streaming, yet conflict with the polling design. For example, SQL Server provides the Service Broker, which is more appropriate to real-time streaming than the current table based implementation. However its blocking nature conflicts with the fast polling nature of the generic polling agents. So something needs to give along the way. It's a challenge to abstract and specialize at the same time.

As in getting more data or more I/O capacity on the fly and if its possible to react without downtime. So consequently, not only bring things like this into startup options but to the DI to be changed on the fly.

Got a feeling IOptionsMonitor<T> and some forever loops will be our long term friends.

Some industrial and other uses cases are a bit hard to run down and preferably one can avoid that to fix or change things.

Can you elaborate?

[veikkoeeva]

@JorgeCandeias

Some industrial and other uses cases are a bit hard to run down and preferably one can avoid that to fix or change things.

Can you elaborate?

Factory systems (consider 24/7, with e.g. yearly maintenance window of a day), large scale logistics systems and some payment systems are my experience. One can plan for it, better if it were possible to be able to change things on the fly.

As an aside:

For example, SQL Server provides the Service Broker,

Is this still dog slow? Though a good example, or the PostgreSQL extensions.

[benjaminpetit]

Is there any changes to the source from this project compared to the files stored in the shared folder (appart from the suppression of the compiler directive and c# modernization)?

Not sure if it's github or if we are losing the change history with this move.

[JorgeCandeias]

Is there any changes to the source from this project compared to the files stored in the shared folder (appart from the suppression of the compiler directive and c# modernization)?

No changes are intended apart from:

1. Removing compiler directives.
2. Moving shared code to a core project.

This includes no great intention for C# modernization just yet. The objective is to lessen the review burden as there are already 89 files to look at. There were a few small bits that were refactored now but only to apply analyser fixes, such as the switch expression change in AdoNetFormatProvider.

The lack of further refactoring is why the InternalsVisibleTo attributes are there for now. The original provider code was tightly coupled to the shared code via linked files. This coupling did not change as of this PR, it has only been made formal.

The next PR will deal with moving the core classes into DI so the forward dependencies can be removed.

Not sure if it's github or if we are losing the change history with this move.

I did use git mv to keep file history where appropriate and I did commit each move on its own. This can be verified in the individual "moved file" commits. I'm not sure why Github is not showing more files as moved.