Modularize Block Factory #8499
Modularize Block Factory #8499
Conversation
itaigilo
added
the
exclude-changelog
E2E Test Results - DynamoDB Local - Local Block Adapter |
E2E Test Results - Quickstart |
cmd/lakefs/cmd/run.go
Outdated
| @@ -18,6 +18,7 @@ import ( | |||
| "github.com/go-co-op/gocron" | |||
| "github.com/spf13/cobra" | |||
| "github.com/spf13/viper" | |||
| "github.com/treeverse/lakefs/modules/block/factory" | |||
There was a problem hiding this comment.
One thing I forgot!
Let's explicitly give it a name to avoid collisions in future factories:
| "github.com/treeverse/lakefs/modules/block/factory" | |
| blockfactory "github.com/treeverse/lakefs/modules/block/factory" |
modules/block/factory/go.work
Outdated
| use ( | ||
| . | ||
| ) |
There was a problem hiding this comment.
You are importing the lakefs package to the factory module - you need to define it in the work file so it will use the local copy
| use ( | |
| . | |
| ) | |
| use ( | |
| . | |
| ../../.. | |
| ) |
pkg/catalog/catalog.go
Outdated
| @@ -23,9 +23,9 @@ import ( | |||
| "github.com/hashicorp/go-multierror" | |||
| lru "github.com/hnlq715/golang-lru" | |||
| "github.com/rs/xid" | |||
| "github.com/treeverse/lakefs/modules/block/factory" | |||
There was a problem hiding this comment.
The run file already imports and instantiate the block adapter, however catalog also instantiates it's own block adapter, this is probably for historical reasons.
Consider - passing the block adapter in run to the catalog New function
There was a problem hiding this comment.
If I get what you're suggesting -
In the run file, the BuildBlockAdapter is initialized with statsCollector, while in the catalog it isn't, no?
pkg/catalog/catalog.go
Outdated
| @@ -23,9 +23,9 @@ import ( | |||
| "github.com/hashicorp/go-multierror" | |||
| lru "github.com/hnlq715/golang-lru" | |||
| "github.com/rs/xid" | |||
| "github.com/treeverse/lakefs/modules/block/factory" | |||
There was a problem hiding this comment.
| "github.com/treeverse/lakefs/modules/block/factory" | |
| blockfactory "github.com/treeverse/lakefs/modules/block/factory" |
There was a problem hiding this comment.
Let's say this happens, and we have both imports (which we currently don't) -
Why the modules one should be blockfactory and the other just factory?
First, I don't think such convention is currently necessary.
And if it is necessary - please have a convention that reflects the fact that it's a module. I'd say moduleblockfactory, but it's too much.
Bottom line, I think we should leave it for now, since it doesn't make things clearer IMO.
There was a problem hiding this comment.
What do you mean lets say it happens - we're going to have a module called config/factory when you import both, they will both be factory. One of the will be automatically renamed but I believe the convention should be to explicitly name them!
There was a problem hiding this comment.
Ahhh, I thought you meant a module and a local with the same name.
Agree.
Updated.
pkg/config/config_test.go
Outdated
| @@ -12,8 +12,8 @@ import ( | |||
| awsconfig "github.com/aws/aws-sdk-go-v2/config" | |||
| "github.com/go-test/deep" | |||
| "github.com/spf13/viper" | |||
| "github.com/treeverse/lakefs/modules/block/factory" | |||
There was a problem hiding this comment.
| "github.com/treeverse/lakefs/modules/block/factory" | |
| blockfactory "github.com/treeverse/lakefs/modules/block/factory" |
Closes #8493.