-
Notifications
You must be signed in to change notification settings - Fork 15
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
global db support #55
Conversation
wip globaldb
connectors/storage/v2/contract.go
Outdated
) | ||
|
||
const ( | ||
globalDBYamlKey = "$wintr/connectors/storage/v2" |
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.
What's with the '$' ?
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 have in some configs global key wintr/connectors/storage/v2 for tenant-db and then it is used in modules by anchor, it'll be duplicated. So it is to separate them or we have to include it in every module
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.
I don't understand; give me an example
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.
dev configs especially https://github.com/ice-blockchain/eskimo/blob/master/application.yaml#L32 and used by anchor: https://github.com/ice-blockchain/eskimo/blob/master/application.yaml#L49
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.
i see. Then lets make it explicit, :
global:
wintr/connectors/storage/v2:
...
connectors/storage/v2/storage.go
Outdated
var cfg storageCfg | ||
appcfg.MustLoadFromKey(globalDBYamlKey, &cfg) | ||
if cfg.PrimaryURL != "" { | ||
initGlobalDBCtx, cancel := context.WithTimeout(context.Background(), 30*stdlibtime.Second) //nolint:gomnd,mnd // . |
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.
Use background cuz otherwise pool will close in 30 secs
connectors/storage/v2/storage.go
Outdated
func init() { | ||
var cfg storageCfg | ||
appcfg.MustLoadFromKey(globalDBYamlKey, &cfg) | ||
if cfg.PrimaryURL != "" { |
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.
Or len replicas > 0
connectors/storage/v2/storage.go
Outdated
} | ||
} | ||
|
||
func GlobalDB() *DB { |
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.
What if we dont do this and we just use it of its != nil. Wherever a *DB is needed check if u can use the globalDB instead.
And in case of MustConnect, u can just run the ddl on the globalDB
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.
Here could be an issue probably if some package will use both tenant and global db. We dont have them rn, but probably would be an issue
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.
so we need some kind on flag at least like
MustConnect(ctx context.Context, ddl, applicationYAMLKey string, global ...bool) *DB {
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.
no; we`ll use one or the other; with globalDB having priority and being used if its !=nil
connectors/storage/v2/storage.go
Outdated
func MustConnect(ctx context.Context, ddl, applicationYAMLKey string) *DB { | ||
var cfg config | ||
appcfg.MustLoadFromKey(applicationYAMLKey, &cfg) | ||
if globalDB != nil && cfg.WintrStorage.PrimaryURL == "" && len(cfg.WintrStorage.ReplicaURLs) == 0 && !cfg.WintrStorage.RunDDL { |
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.
No, only if globalDB != 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.
We'll have issue with coin distribution and tokenomics. As var is global, coin distiribution will try to use global db as well, it seems to be unexpected, but tokenomics uses global for boosts
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.
Ok, add a flag in the cfg named ignoreGlobal which if set to true, will use the package level connector
Btw, make sure close() is idempotent. Cuz it will be run multiple times |
No description provided.