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

Combine: Move SqlEnvChange to shared and sync usage #1200

Closed
wants to merge 4 commits into from

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Aug 1, 2021

As the title says. Makes SqlEnvChange shared and changes netfx to use the same logic as netcore which has better a better memory allocation profile. Later on after this and #1199 are merged the SqlEnvChange objects can be pooled which will further reduce allocations on reconnection and query.

@cheenamalhotra cheenamalhotra added the ➕ Code Health Issues/PRs that are targeted to source code quality improvements. label Aug 20, 2021
@cheenamalhotra cheenamalhotra added this to the 4.0.0-preview3 milestone Sep 21, 2021
cheenamalhotra
cheenamalhotra previously approved these changes Sep 22, 2021
johnnypham
johnnypham previously approved these changes Oct 4, 2021
@JRahnama
Copy link
Contributor

JRahnama commented Oct 8, 2021

@Wraith2 can you address the conflict that I can merge the code please?

@Wraith2
Copy link
Contributor Author

Wraith2 commented Oct 8, 2021

merge conflicts and feedback fixed.

JRahnama
JRahnama previously approved these changes Oct 14, 2021
@JRahnama
Copy link
Contributor

@Wraith2 the failures are consistent and only happening on netfx. could be checked here

@Wraith2
Copy link
Contributor Author

Wraith2 commented Oct 15, 2021

I went through the original commits from corefx and I think I've found the problem and corrected it. I can't debug the DTC related tests locally though because I can't get DTC working (spent my friday night trying) so if this doesn't work either I'll need some help or we'll have to close and retry this another time.

@johnnypham
Copy link
Contributor

I went through the original commits from corefx and I think I've found the problem and corrected it. I can't debug the DTC related tests locally though because I can't get DTC working (spent my friday night trying) so if this doesn't work either I'll need some help or we'll have to close and retry this another time.

What have you tried? Adding a firewall rule for msdtc.exe worked for me.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Oct 18, 2021

Firewall rules. security permissions in component services. process monitoring. various KB support suggestions. I'm not new to debugging windows stuff. Sadly it just doesn't work for me, no idea why at this point.
The CI seems to have passed so if I fix up that project conflict it should be ok to merge.

@cheenamalhotra cheenamalhotra dismissed stale reviews from DavoudEshtehari and JRahnama October 18, 2021 20:20

Review needed again

@cheenamalhotra cheenamalhotra dismissed johnnypham’s stale review October 18, 2021 20:20

Review needed again

@cheenamalhotra cheenamalhotra dismissed their stale review October 18, 2021 20:20

Review needed again

@johnnypham
Copy link
Contributor

Have you tried "allow an app through windows firewall"? For the CI agent machines, simply adding a firewall rule didn't work but this additional change did.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Oct 18, 2021

Yes, even tried with the firewall off. Didn't work. I'm not about to start uninstalling it to see if that helps so I'm just going to leave it alone for the moment. The CI will work for that test and the CI needs to pass before merge anyway so that'll be the coverage required.

I also removed a Debugger.Launch() from a test that got mistakenly merged in one of my other PR's.

@cheenamalhotra cheenamalhotra removed this from the 4.0.0-preview3 milestone Oct 19, 2021
@Wraith2
Copy link
Contributor Author

Wraith2 commented Oct 20, 2021

The errors don't seem related. Could someone re-run the ci steps that failed?

@Wraith2
Copy link
Contributor Author

Wraith2 commented Oct 27, 2021

What is blocking this PR please?

@cheenamalhotra
Copy link
Member

@Wraith2

The last preview was the last window for pushing code changes and we've merged many changes. We are on code freeze and only bug fixes or test related changes are going to be merged till GA 4.0 is released. Our team is busy with security testing and other release related activities, we will merge when the time is right.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Oct 27, 2021

Ok, so where do I find that sort of information so I know what's going on? dotnet/runtime publish all this information about release dates code cutoffs etc, so do EFCore. Purely from the perspective of this PR there's no apparent reason that it missed an arbitrary deadline I couldn't see.

When I have been aware of deadlines like the 3.0 I've prepared code changes weeks in advance and still had the code skipped. I see nothing happening then sometimes something will get merged and then silence for weeks on end. From an external perspective there is no way to know why anything happens. I'm still hanging on here trying to collaborate and it's still frustrating and not changing.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Feb 23, 2022

This is another PR which I'm going to have to re-do because it's been open so long the rebase can't be done cleanly. There are errors around Server namespace types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
➕ Code Health Issues/PRs that are targeted to source code quality improvements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants