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

[FIXED] Unnecessary account reloads #1387

Merged
merged 1 commit into from
May 12, 2020
Merged

[FIXED] Unnecessary account reloads #1387

merged 1 commit into from
May 12, 2020

Conversation

matthiashanel
Copy link
Contributor

Fixes #1372 by updating s.sys.account pointer.

This issue also showed that accounts are unnecessarily reloaded.
This happened because account imports were not copied and thus,
deepEqual detected a difference were none was.
This was addressed by making the copy less shallow.

Furthermore did deepEqual detects a difference when it compared
slices that were appended to while processing a map.
This was fixed by sorting before comparison.

Noticed that Account.clients stored an unnecessary pointer.
Removed duplicated code in systemAccount.

Signed-off-by: Matthias Hanel mh@synadia.com

I did not outright eliminate sys.account pointer as we do not necessarily always have a name that is specified.

@matthiashanel matthiashanel requested a review from kozlovic May 11, 2020 22:15
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe no need for unlock/lock? Shortening the requestChan name may have resulted in less visible change in the struct/init :-)

server/events.go Outdated Show resolved Hide resolved
server/events.go Show resolved Hide resolved
server/events.go Show resolved Hide resolved
server/server.go Show resolved Hide resolved
@matthiashanel
Copy link
Contributor Author

@kozlovic , incorporated your comments. added waiting for disconnect event to unit test and moved to single re-lock in configureAccounts.

server/reload_test.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Fixes #1372 by updating s.sys.account pointer.

This issue also showed that accounts are unnecessarily reloaded.
This happened because account imports were not copied and thus,
deepEqual detected a difference were none was.
This was addressed by making the copy less shallow.

Furthermore did deepEqual detects a difference when it compared
slices that were appended to while processing a map.
This was fixed by sorting before comparison.

Noticed that Account.clients stored an unnecessary pointer.
Removed duplicated code in systemAccount.

Signed-off-by: Matthias Hanel <mh@synadia.com>
@kozlovic kozlovic changed the title [FIXES] Unnecessary account reloads and pointer to old accounts [FIXED] Unnecessary account reloads May 12, 2020
@kozlovic kozlovic merged commit 09a58cb into master May 12, 2020
@kozlovic kozlovic deleted the reload branch May 12, 2020 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

configuration reload that changes the system account does not take effect everywere
2 participants