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
Cache: Coherence #13137
Cache: Coherence #13137
Conversation
The cache would fail to notify if you changed from a value and back to the original value. This is because the underlying hash value was never written. This has two consequences: 1. The cache could never emit a change of the original value 2. Because we never cached the new value, if the new value was the same as the old one, we would still emit the event. The fix is easy, just save the hash after the notify and we can have better cache coherency.
@@ -156,6 +156,7 @@ func (w *ConfigWatcher) configChanged(topic string, value interface{}) { | |||
return | |||
} | |||
w.notify() | |||
w.hash = hash |
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.
This is the actual fix
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.
Did you play around with whether this should happen before or after the notify? (I think after is correct, but I wanted to make sure we thought about it.)
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.
Yeah I did, but the conclusion I came to was the same.
@@ -156,6 +156,7 @@ func (w *ConfigWatcher) configChanged(topic string, value interface{}) { | |||
return | |||
} | |||
w.notify() | |||
w.hash = hash |
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.
Did you play around with whether this should happen before or after the notify? (I think after is correct, but I wanted to make sure we thought about it.)
|
#13142 Merge forward 2.8 into 2.9 2ecb859 (upstream/2.8, origin/2.8, 2.8) Merge pull request #13137 from SimonRichardson/cache-coherence c973d15 (achilleasa/2.8) Merge pull request #13108 from achilleasa/2.8-support-ssh-with-proxy-to-fan-subnets f505b12 Merge pull request #13097 from achilleasa/2.8-logsink-error-if-persisting-logs-to-db-fails Conflicts: CONFLICT (content): Merge conflict in cmd/juju/commands/ssh_unix_test.go CONFLICT (content): Merge conflict in cmd/juju/commands/ssh_machine.go CONFLICT (content): Merge conflict in cmd/juju/commands/ssh.go CONFLICT (content): Merge conflict in cmd/juju/commands/scp.go
The cache would fail to notify if you changed from a value and back to
the original value (flip flopping). This is because the underlying hash value
was never written.
This has two consequences:
as the old one, we would still emit the event.
The fix is easy, just save the hash after the notify and we can have
better cache coherency.
QA steps
If you change the model-config logging-config to a new value and back to the
old value, it should now correctly go back to the old value. Flip-flopping of values
in the logging-config exposes this relatively easily.
View
$ juju debug-log -m controller
to ensure that the flip-flop happens.