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

Prevent stale objects in stream cache of route_to_stream function #6788

Merged
merged 9 commits into from Nov 25, 2019
Merged

Conversation

@thll
Copy link
Contributor

thll commented Nov 13, 2019

Description

Fixes an issue with stale stream objects in the cache used by the route_to_stream function.

The cache is now rebuilt by loading all streams from the database on every change to a stream. This adds a performance penalty and additional load on the database on every change to streams. But it makes the code much simpler and still offers the same performance on cache lookups.

Motivation and Context

The Multimap used in the previous implementation was not replacing any stream objects if an object with that stream ID was already present. Therefore the stream object fetched from the cache by the function would not reflect the most current state. That had the effect, that when e.g. the index set of a stream was changed, the function would still use the old index set and not see the latest change.

Fixes #4954

How Has This Been Tested?

Reproduced the issue as described in #4954 before the change. Wasn't able to reproduce it after the change.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
thll added 3 commits Nov 12, 2019
One test is currently failing, as the cache doesn't store the changed
stream on update, which is a bug.
Previously we were storing the stream itself in the nameToStream
MultiMap but were using only the id of the stream as the comparator for
the underlying set. This would lead to stale stream objects in that
cache if the cache was updated with a changed stream which was already
cached.

By using a real cache to load the streams by name on demand and
invalidating if a stream changes, we impose a penalty on first lookup by
name after a change but avoid the stale items.

fixes #4954
@thll thll requested review from bernd and mpfz0r Nov 13, 2019
@bernd bernd self-assigned this Nov 13, 2019
thll added 6 commits Nov 14, 2019
Just to be safe.
This reverts commit 8f1c30c
This reverts commit 935628b
The implementation felt way to complicated for what it was doing. For
the benefit of having clearer semantics we simply reload all streams
when a stream changes. This will put additional load on the database for
cache updating but cache lookup times will stay stable.
@bernd
bernd approved these changes Nov 25, 2019
Copy link
Member

bernd left a comment

LGTM! 👍

@bernd bernd merged commit e9dc74a into master Nov 25, 2019
4 checks passed
4 checks passed
ci-web-linter Jenkins build graylog-pr-linter-check 5102 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
graylog-project/pr Jenkins build graylog-project-pr-snapshot 6781 has succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@bernd bernd deleted the issue-4954 branch Nov 25, 2019
linuspahl added a commit that referenced this pull request Nov 27, 2019
)

Previously we were storing the stream itself in the nameToStream
MultiMap but were using only the id of the stream as the comparator for
the underlying set. This would lead to stale stream objects in that
cache if the cache was updated with a changed stream which was already
cached.

The implementation felt way to complicated for what it was doing. For
the benefit of having clearer semantics we simply reload all streams
when a stream changes. This will put additional load on the database for
cache updating but cache lookup times will stay stable.

Fixes #4954
bernd added a commit that referenced this pull request Nov 28, 2019
)

Previously we were storing the stream itself in the nameToStream
MultiMap but were using only the id of the stream as the comparator for
the underlying set. This would lead to stale stream objects in that
cache if the cache was updated with a changed stream which was already
cached.

The implementation felt way to complicated for what it was doing. For
the benefit of having clearer semantics we simply reload all streams
when a stream changes. This will put additional load on the database for
cache updating but cache lookup times will stay stable.

Fixes #4954

(cherry picked from commit e9dc74a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.