Fix caching #545
Merged
Fix caching #545
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I think there is something odd going on with the base you started your PR from. |
|
Right, it should have been a draft there is also some other things I need to remove |
JoranAngevaare
commented
Oct 7, 2021
| @@ -433,7 +433,9 @@ def _context_hash(self): | |||
| # Also take into account the versions of the plugins registered | |||
| base_hash_on_config.update( | |||
| {data_type: (plugin.__version__, plugin.compressor, plugin.input_timeout) | |||
| for data_type, plugin in self._plugin_class_registry.items()}) | |||
| for data_type, plugin in self._plugin_class_registry.items() | |||
| if not data_type.startswith('_temp_') | |||
|
stupid codefactor, I didn't touch that part of the code |
Sorry, I do not fully get this part. I tried to recall how it works based on #483 and #485. Do you really mean context hash? I thought several plugins are stored under the same context hash, but different keys for the individual plugins. Why are we deleting the context hash when removing a _temp- plugin? Okay nvm, was just too late yesterday. |
WenzDaniel
approved these changes
Oct 14, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the problem / what does the code in this PR do
Dominick noticed this error first, where a temporary plugin caused the context_hash to change during the computation of multiple runs.
The issue is that we add some temp. plugin here and then delete it here, in between there is get_components which builds the cache (if applicable). Since Dominick was doing this with 4 workers, they each got their own context hash as they were registering and de-registering the temporary plugins in parallel. Because of that, there was a chance that the context hash was removed from the cache by the time it was requested in another thread.
There would also be another way of dealing with this, and that is allowing multiple context hashes to be cached, however, this would be slow and also not needed, for the temporary plugins aren't re-used anyway (each thread generates another random hash).