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

Avoid sending cluster manifest to client if client already has the latest version #8728

Merged
merged 13 commits into from
Dec 15, 2023

Conversation

453873
Copy link
Contributor

@453873 453873 commented Nov 16, 2023

Fixes #8722

Microsoft Reviewers: Open in CodeFlow

@453873
Copy link
Contributor Author

453873 commented Nov 16, 2023

@dotnet-policy-service agree

@ReubenBond ReubenBond self-assigned this Nov 17, 2023
@ReubenBond ReubenBond changed the title fixed #8722 Avoid sending cluster manifest to client if client already has the latest version. Nov 17, 2023
@ReubenBond ReubenBond changed the title Avoid sending cluster manifest to client if client already has the latest version. Avoid sending cluster manifest to client if client already has the latest version Nov 17, 2023
@HermesNew
Copy link
Contributor

Sorry, we found that there was a code omission when submitting the PR. We will submit another PR later.

private ConcurrentDictionary<SiloAddress, MajorMinorVersion> siloAddressVersionMap = new ConcurrentDictionary<SiloAddress, MajorMinorVersion>();

@453873
Copy link
Contributor Author

453873 commented Nov 18, 2023

I find bug, every silo address need compare clusterManifest version,new patch is commit.

@HermesNew
Copy link
Contributor

@ReubenBond There are two items that have not passed the test. Could you please check them?

@ReubenBond
Copy link
Member

I pushed a change, I think this is the right approach and should achieve the behavior you're looking for.

@HermesNew
Copy link
Contributor

The key point of this issue is comparing versions of the same SiloHost.So it is necessary to update the version according to Silohost.

@ReubenBond
Copy link
Member

@HermesNew the current version in this PR should accomplish that - do you see issues with the implementation here?

@ReubenBond
Copy link
Member

@HermesNew @453873, please take a look at the current version and let me know if you have any issues with it. I will merge and we can include it in the next set of releases (7.2.5 & 8.0.0)

@ReubenBond ReubenBond added the Needs: backport PRs which should be backported to a (prior) release branch label Dec 5, 2023
@HermesNew
Copy link
Contributor

@ReubenBond Sorry, I've been very busy these past few days. Tomorrow we will release a version ourselves to verify whether this issue has been resolved.

@ReubenBond
Copy link
Member

No problem at all, thank you @HermesNew!

@HermesNew
Copy link
Contributor

HermesNew commented Dec 6, 2023

@ReubenBond I don't see any code for this issue in the 7. x branch. We are using. net7.0, so we can only release this version. My fork is the main code, and the. net SDK is already .net8.0.

@ReubenBond
Copy link
Member

@HermesNew We can backport this to the 7.x branch - usually, we would do that after merging the PR

@ReubenBond ReubenBond merged commit d54dc1e into dotnet:main Dec 15, 2023
19 checks passed
ReubenBond added a commit to ReubenBond/orleans that referenced this pull request Dec 15, 2023
…test version (dotnet#8728)

Co-authored-by: Reuben Bond <[email protected]>
Co-authored-by: ReubenBond <[email protected]>
Co-authored-by: HermesNew <[email protected]>
ReubenBond added a commit that referenced this pull request Dec 16, 2023
* Avoid sending cluster manifest to client if client already has the latest version (#8728)

Co-authored-by: Reuben Bond <[email protected]>
Co-authored-by: ReubenBond <[email protected]>
Co-authored-by: HermesNew <[email protected]>

* No newer cluster manifest handle

There was no newer cluster manifest, so wait for the next refresh interval and try again.

* PR feedback

---------

Co-authored-by: swam <[email protected]>
Co-authored-by: HermesNew <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Jan 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Needs: backport PRs which should be backported to a (prior) release branch
Projects
None yet
4 participants