[Azure Service Bus] Replace Stopwatch with ValueStopwatch

[danielmarbach]

Introduces

https://github.com/dotnet/aspnetcore/blob/master/src/Shared/ValueStopwatch/ValueStopwatch.cs and
https://github.com/dotnet/aspnetcore/blob/master/src/Shared/test/Shared.Tests/ValueStopwatchTest.cs

to avoid allocating Stopwatches all over the place. While taking the file from ASPNETCore I aligned the license with the license of this code. The original license was Apache. I'll let you figure out if that is ok.

[azuresdkci]

Can one of the admins verify this patch?

[pakrym]

The change looks good, cc @AlexGhiondea for the license question.

[jsquire]

Thanks, @danielmarbach! I'm interested in @AlexGhiondea's decision on this; I'd like to make use of this in the Event Hubs client as well. @pakrym: Assuming that the license change isn't an issue, should we consider moving this to Core?

[pakrym]

@jsquire if you are going to use it definitely.

[danielmarbach]

According to my limited research it would also be possible to leave it under Apache 2 in the repo since MIT and Apache 2 are compatible. If that is done the owner doesn't have to be contacted because the license is not changed.

[danielmarbach]

Alternatively I can also update the PR to take in https://github.com/dotnet/orleans/blob/master/src/Orleans.Core/Timers/ValueStopwatch.cs which is MIT license already.

Let me know what you think

[AlexGhiondea]

@danielmarbach let me look into the license question.

[danielmarbach]

Yeah sorry didn't want to cause stress. Was merely thinking out loud about other options. Bad habit of me

[AlexGhiondea]

@danielmarbach no worries! Here is what we should do:

In this PR, please use the original copyright header for the file you are copying over.

We’ll have a separate PR to ensure the right attribution happens in the notices.txt file.

[danielmarbach]

Brought back the original license and excluded the SA1636 warning in the file. No CI fails with certificate expired failures.

[jsquire]

Cool.... looks like we're unblocked for moving forward.

@pakrym: Would you prefer that this PR merge as-is and then we do a follow-up to shift things to Core, or should we discuss shifting to Core as part of Daniel's efforts?

[pakrym]

Whatever is easier to do. Seems like adding it to core is just a case of moving the source and test files so I don't see a big reason to wait.

[danielmarbach]

Who should wait for what?

I wasn't sure based on the above communication what is expected from whom nor the direction to take. So I went ahead and moved the source files to core assuming what I'm doing is somehow aligned with what you have been thinking behind the scenes.

In general it would be very helpful if external contributors get a bit more guidelines on the PR. Something like:

Daniel, would you mind moving the file to Core\bla under namespace foobar and the edit bla.props.foobar to include it as a shared resource? If you don't have time don't worry we can take the further steps before merging the PR.

I hope this doesn't come across in weird ways. I'm reflecting about what would have helped me personally to know what the next steps are from your perspective plus it shortens the communication paths quite a bit. It also gives me the possibility to say "I'm sorry but at this point I've reached the level of how far I can go as a community contributor and I would appreciate if you take it forward from this point"

With ❤️ Daniel

[pakrym]

Thank you for the feedback!

My bad, I'll do better. You did the right thing here, just one small comment.

[jsquire]

Who should wait for what?

I wasn't sure based on the above communication what is expected from whom nor the direction to take. So I went ahead and moved the source files to core assuming what I'm doing is somehow aligned with what you have been thinking behind the scenes.

Apologies; I'm at fault here. It wasn't my intent to talk over you with the sidebar and I meant to follow-up yesterday with a more explicit set of details around what we had been discussing. I got a bit distracted and when I just looped around to do so, it appears that you're already in motion and Pavel was engaged for feedback.

Thank you, again, for taking the time to contribute and to work through the lack of clarity. Your efforts are very much appreciated.

[danielmarbach]

@pakrym @jsquire No worries I've been in similar situations on the other side of PRs and have a lot of empathy for things like that. Nonetheless I appreciate that you took the time to listen to the feedback

[jsquire]

LGTM in general and from the Service Bus perspective. I'll defer to Pavel for the authoritative approval here.

[JoshLove-msft]

@danielmarbach would you be so kind as to rebase this so it can be merged in?

[danielmarbach]

@JoshLove-msft done

I also sent in #11530 which is a branch of top of this one that also fixes Event Hub to make @jsquire a happy person ;) Once this one is merged I will rebase the other one on top of master

[jsquire]

that also fixes Event Hub to make @jsquire a happy person ;)

Indeed it does. Thank you very much for that!