refactor: Stats metadata providers #8849
Conversation
E2E Test Results - DynamoDB Local - Local Block Adapter |
E2E Test Results - Quickstart |
nopcoder
added
exclude-changelog
There was a problem hiding this comment.
Looking great -
Awesome initiative, thanks for handling this change.
Add some suggestions,
But the only reason that I'm blocking is because it's adelicate change in an important area, and I'd be happy to get some details about how was this tested.
And again, looks great.
pkg/block/metadata_provider.go
Outdated
| const MetadataBlockstoreTypeKey = "blockstore_type" | ||
|
|
||
| // SingleTypeMetadataProvider is a metadata provider that reports a single blockstore type. | ||
| type SingleTypeMetadataProvider struct { |
There was a problem hiding this comment.
Suggestion, looking ahead at the MSB:
Create a BuildMetadataProvider(cfg config.StorageConfig) in modules/block/factory that will return this SingleTypeMetadataProvider, and have BuildMetadataProviders call this build function (instead of create the Provider instance directly).
It will save an extra PR later on.
There was a problem hiding this comment.
The current code should handle both cases as it works with the configuration interface.
The "single" here is just to have a metadata provider for a single type - but used by multi storage.
let me know if I missed something.
There was a problem hiding this comment.
Oh, right. Missed the ids := cfg.GetStorageIDs() part, and was also confused by the name, I guess.
Maybe rename to BlockstoreTypeMetadataProvider, for example?
(But it's a matter of style and non-blocking.)
pkg/stats/metadata_test.go
Outdated
|
Small refactor to the pkg/cloud to enable testing the detect execution. |
| if id, err := detector(); err == nil { | ||
| cloudType, cloudID, cloudDetected = name, id, true | ||
| return | ||
| } |
There was a problem hiding this comment.
If we're ignoring the err on purpose, maybe it's worth documenting.
pkg/block/metadata_provider.go
Outdated
| const MetadataBlockstoreTypeKey = "blockstore_type" | ||
|
|
||
| // SingleTypeMetadataProvider is a metadata provider that reports a single blockstore type. | ||
| type SingleTypeMetadataProvider struct { |
There was a problem hiding this comment.
Oh, right. Missed the ids := cfg.GetStorageIDs() part, and was also confused by the name, I guess.
Maybe rename to BlockstoreTypeMetadataProvider, for example?
(But it's a matter of style and non-blocking.)
|
@NoyDavidson2 adding "blockstore_count" with the number of adapters configured, if more than 1 |
| } | ||
|
|
||
| // BuildMetadataProviders returns metadata providers for each unique blockstore type in the storage config. | ||
| func BuildMetadataProviders(cfg config.StorageConfig) []*BlockstoreTypeMetadataProvider { | ||
| // NewMetadataProvider returns metadata provider to report blockstore type(s) based on storage config. |
There was a problem hiding this comment.
Add more information
We are returning the type with unique blockstore types
and the count of ids, because to identify we have many adapters, even they are from the same type
Cloud detect code was tested manually on different accounts.
Note that as long as we use AWS SDK, it will require environment where the SDK can be initialized to acquire the information.
Tested manually by building a binary that run cloud detect and used 3 cloud accounts that configured to run lakeFS to test it out.
Close #8850