-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Require the storage to be explicitly set for persistent queue #5784
Require the storage to be explicitly set for persistent queue #5784
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5784 +/- ##
=======================================
Coverage 91.56% 91.56%
=======================================
Files 192 192
Lines 11470 11470
=======================================
Hits 10502 10502
Misses 773 773
Partials 195 195 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
112a044
to
1455326
Compare
1455326
to
f8c9aff
Compare
expectedError: nil, | ||
desc: "obtain storage extension by name", | ||
numStorages: 2, | ||
storageID: "1", |
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.
Shouldn't this be?
storageID: "1", | |
storageID: "file_storage/1", |
I thought ComponentID always includes the component type name.
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 just a test parameter, in the actual test code we create numStorages
storage extensions, and this is simply the index of the one we want to use in the test. I'll change it to storageIndex
so it's less confusing.
if storageExtension != nil { | ||
return nil, errMultipleStorageClients | ||
func (qCfg *QueueSettings) getStorageExtension(extensions map[config.ComponentID]component.Extension) (storage.Extension, error) { | ||
if qCfg.StorageID != nil { |
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.
Few suggestions:
- Usually prefer to check for conditions first and return than indenting the code.
if qCfg.StorageID == nil { return nil, nil }
- Better to pass a non pointer
ComponentID
and then no need for the func to be on the QueueSettings. You already check for nil in the caller code.
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.
Good point, thx. I also noticed I missed a test for the wrong extension type that was in my original PR, re-added while making these changes.
f8c9aff
to
7b19438
Compare
You need a rebase |
7b19438
to
eab0b3f
Compare
Description:
The persistent queue needs a storage client to run. Currently, this simply takes the one storage extension defined, and errors if there are multiple available. This is both restrictive and arbitrary, and mostly works because it's currently unusual for users to define multiple storage extensions.
This change forces the users to explicitly set the storage extension in queue config if they want to use the persistent queue. This setting doubles as both choosing the storage and enabling the persistent queue - when
storage: nil
(the default), we use the in-memory queue, otherwise, we use the persistent queue with the set storage extension.This was originally discussed in #5711, and it's part of that change. As it's a breaking change, I'd like to do it before we enable the persistent queue by default in the build.
Link to tracking Issue: Related to open-telemetry/opentelemetry-collector-contrib#10915
Testing:
Updated the unit tests.
Documentation:
Updated the queue readme.