Merge common code bases for TdsParserStateObject.cs

[panoskj]

Related to #1261 and prerequisite for working on #593 without hindering the merge process.

This PR is a work in progress right now, but I would like some feedback while working on it.

Though it should be mergeable as it is too.

My questions for now:

1. Do the commits look good so far?
2. The netfx version uses ConstrainedRegions. Should they be removed or preserved?

[dnfadmin]

All CLA requirements met.

[Wraith2]

The changes themselves look fine.
Constrained regions should be preserved, everything should be preserved including CAS so that the Microsoft version of this library is as close as possible to the System version.

There are a lot of other open PR's to merge other files in the solution and they're just sitting waiting to be reviewed and merged. The longer it's left the more change is likely to happen between you opening it and it being reviewed and merged meaning you have to keep rebasing. The rate of change in this repo isn't due to lack of will to make the changes it's all down to lack of time from the owners.

[Wraith2]

I think you might want to change the shared version of the file to TdsParserStateObject.Common.cs because visual studio doesn't like having two files with the same name in a single tree level and it breaks the ability for it to track the currently editing file.

@DavoudEshtehari any chance of this getting bumped to the top of the list? it's needed for string perf issues.

[DavoudEshtehari]

The netfx version uses ConstrainedRegions. Should they be removed or preserved?
I agree with Wraith2.
Generally speaking, we strive to keep a PR, single-purpose, to avoid mixing up changes as much as possible.

any chance of this getting bumped to the top of the list? it's needed for string perf issues.
@Wraith2 thanks for the comment. 😊 We'll back to this after the next preview release.

[DavoudEshtehari]

LGTM
Please, consider the following improvements:

• Removing the unnecessary using statements.
• Making the PacketData & StateSnapshot private and sealed.

[panoskj]

I pushed two more commits, merging more methods.

Regarding the second commit, @Wraith2 and @DavoudEshtehari do you think using #if directives is a good idea in these cases?

There are many methods that differ by only a few lines. Some common cases are:

1. Exceptions that are handled in different ways (e.g. here and here).
2. Logging with SqlClientEventSource.Log.TryAdvancedTraceEvent is different.
3. Constrained Regions and Reliability Sections that we discussed previously.

In some cases it may be better to use a partial method, e.g. a wrapper for this method.

Also note netcore version didn't have the TdsParser.Is2005OrNewer property, but netfx did. Does this mean netcore version doesn't support SQL server 2005 while netfx does? I added the property in the second commit, but the same result could be achieved using an #if directive here.

Let me know how to proceed.

[Wraith2]

Regarding the second commit, @Wraith2 and @DavoudEshtehari do you think using #if directives is a good idea in these cases?

In general. No. That doesn't mean there aren't situations where they aren't the right thing to do but in general if there is behavioural difference it should either be abstracted away or dealt with by having a separate version.

Also note netcore version didn't have the TdsParser.Is2005OrNewer property, but netfx did. Does this mean netcore version doesn't support SQL server 2005 while netfx does?

I've asked the same question in the past. It isn't actively supported but it also isn't something that should be removed as dead.

When we reached this point in the process of merging the codebases my idea was to move all the things which are identical to common and then have a PR per one or two functions at a time to make the reviews easier. As you've seen the time needed to review and merge these PR's outweighs the speed at which we can produce them. We need to make the process of reviewing as simple as possible which means the simplest PR's.

[panoskj]

In general. No. That doesn't mean there aren't situations where they aren't the right thing to do but in general if there is behavioural difference it should either be abstracted away or dealt with by having a separate version.

So, what do you think about the specific cases of the last commit? Most of the methods in this file have such "conflicts", so having a separate version doesn't seem practical.

Edit: about the PR getting large, I'll wait for the reviewers' feedback, but I could move the new commits from this branch to another and make a new PR, if needed.

[Wraith2]

If the netfx version provides better information than the netcore version we should use the netfx approach. Can you trace down whether these cause different messages to be output and if they do identify which is more useful in terms of reporting the error?

[Wraith2]

This one needs input from the MS team. It isn't clear to me why the reliability assertions are not present in the netcore codebase and whether they are obsolete.

[johnnypham]

There doesn't seem to be any mention of ReliabilitySection in netcore.

[Wraith2]

Nope. Probably because netcore can't be hosted in sql server because it doesn't contain he reliability infrastructure that neftx does. However what I don't know is whether the netfx version of this library will ever be required to be loaded into sql server, it feels unlikely to me but if it ever was then this stuff would be needed.

Could the reliability stuff be stubbed out as no-ops in netcore or is it in the framework not this library?

[David-Engel]

I think the reliability stuff should be kept. At least as a no-op, where possible. I'm pretty sure they don't exist in the netcore code because the functions simply weren't supported on netcore when sqlclient was first ported to it. Given the status of .NET Framework and System.Data.SqlClient, I think it might be likely that SQL does eventually adopt .NET and MDS in the database. I'd hate to have to re-create all the reliability code. As it is, it would need a thorough going-through...

[Wraith2]

With the reliability requirements removed from the netcore codebase I'm pretty sure it's not possible to have netcore run in-process in sql server and never will be.
However I do agree that the reliability code should be kept. It may be possible to continue to use netfx based versions.

[Wraith2]

This is just event logging and i think it should be safe to unconditionally include this in all builds.

[Wraith2]

Can you check if this simply causes event logging output? If it does it can probably be unconditionally included in all builds.

[Wraith2]

Merging on master is best but working on your own branch before it's merged into master you have a choice. I find it cleaner to rebase if there are only a few comments and a lot of changes have happened on the main branch. As long as the result is working code then I'm not sure it really matters.

[panoskj]

Thanks @Wraith2, that's what I meant. I almost always prefer rebase over merge for my personal branches.

@JRahnama rebase only affects a single branch in a single repository (e.g. the branch of this PR, which is in my fork). It would not affect this repository at all (in fact, the end result will be the same after merging the PR, since all commits will be squashed into one).

But, since the commits of the PR may change with rebase, you may want to review them again. On the other hand, you may want to review merge commits too. My point is, both ways technically involve reviewing the whole branch again, if you want to be 100% safe.

Though personally, I find merge commits harder to review. It's like reviewing all previous commits squashed into one. But I will stick with whatever works best for the reviewers.

As for github's warning about rebase, I think it is too vague. For example, using rebase to squash commits before merging a PR is not a bad practice, that's essentially what happens in this repository. I think github is only trying to warn those who don't know exactly what they are doing. Because it's very easy to misuse rebase (especially if you have never used it before).

[JRahnama]

/azurepipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).