-
Notifications
You must be signed in to change notification settings - Fork 94
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
feat(spooler): Add initialization of stacks #3967
Conversation
tokio::spawn(async move { | ||
let mut buffer = PolymorphicEnvelopeBuffer::from_config(&config, memory_checker) | ||
.await | ||
.expect("Envelope buffer couldn't be initialized from the config"); |
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.
We might want to fallback to the memory buffer impl in case we fail disk initialization, but we still have to handle Err
given the method signature.
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.
Have you tried how this behaves locally? E.g. by giving the path to a directory that does not exist?
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.
The implementation creates a folder with the file, so it will not fail unless there is filesystem failure. What we could test is to not have the path in the config but this will make the implementation switch to memory. I need to artificially try to return an error and see what is the behavior.
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.
Looks good overall, I left a few questions.
tokio::spawn(async move { | ||
let mut buffer = PolymorphicEnvelopeBuffer::from_config(&config, memory_checker) | ||
.await | ||
.expect("Envelope buffer couldn't be initialized from the config"); |
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.
Have you tried how this behaves locally? E.g. by giving the path to a directory that does not exist?
let mut buffer = PolymorphicEnvelopeBuffer::from_config(&config, memory_checker) | ||
.await | ||
.expect("Envelope buffer couldn't be initialized from the config"); | ||
buffer.initialize().await; |
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.
Any reason we don't call initialize()
inside of from_config()
? Is it the difference in error handling?
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.
Initialize is designed to be infallible but initially, I wanted to design the buffer with the initialization being independent of instantiation, because you don't need the initialization, this separation might be also useful for testing.
Co-authored-by: Joris Bayer <[email protected]>
…elay into riccardo/feat/loading-buffer
Self::new(own_key, sampling_key) | ||
} | ||
|
||
pub fn iter(&self) -> impl Iterator<Item = ProjectKey> { |
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.
pub fn iter(&self) -> impl Iterator<Item = ProjectKey> { | |
/// Returns an iterator over the contained distinct project keys. | |
pub fn iter(&self) -> impl Iterator<Item = ProjectKey> { |
This PR implements the initialization logic of the envelope buffer. The initialization logic has to be run separately from the construction of the buffer since they are two separate things.
In addition, the PR enables the SQLite implementation of the spooler which can be enabled via the configuration.
https://github.com/getsentry/team-ingest/issues/519