-
Notifications
You must be signed in to change notification settings - Fork 142
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
Fix settings Dictionary #909
Conversation
Raise event for remote servers to update its dictionary. Fixes: 895
6902785
to
d675ddb
Compare
Hold merge until confirmation fixes issue. |
With this implementation we get null for a website setting that had previously changed guid, republishing it helps untill the site is restarted. We changed row 251 to use GetDescendants instead of GetChildren, which solved this particular issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as mentioned in the Issues conversations, this fix unfortunately did not solve the problem in a load balanced environment.
@@ -192,6 +238,12 @@ public void UpdateSettings(Guid siteId, IContent content, bool isContentNotPubli | |||
SiteSettings[siteId.ToString() + $"-{contentLanguage}"][contentType] = content; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't SiteSettings[$"{siteId}-default"][contentType] = content;
be set as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I guess there is case where it already exists in the dictionary
I have a working fix, as described here. |
@lunchin can you check out my comment #895 (comment) These small changes in the |
Adding better implementation |
Raise event for remote servers to update its dictionary.
Fixes: #895