-
Notifications
You must be signed in to change notification settings - Fork 0
Update modules on config update #38
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
Conversation
ebb3e29 to
c1ed22b
Compare
c1ed22b to
7af2de7
Compare
2683a49 to
67517d3
Compare
|
|
||
| FlistURL string `json:"flist_url"` | ||
| V4FlistURL string `json:"v4_flist_url"` | ||
| // FlistURL string `json:"flist_url"` |
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.
why did u remove flist url?
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.
flist urls same as storage urls should not be configurable as long as the node will break if they're changed in run time
| if !slices.Equal(subURLs, env.SubstrateURL) { | ||
| log.Debug().Strs("substrate_urls", env.SubstrateURL).Msg("updating to sub manager with url") | ||
| subURLs = env.SubstrateURL | ||
| pool = substrate.NewManager(env.SubstrateURL...) |
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 u are creating connection everytime getSubstrate is called which is against the original design this should be executed one time better to have another approach than this we should be using one connection all the time, so either we detect if connection is not working we restart the node services if we can reach substrate on another url from zos config something like monitoring
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.
nope, we're not creating a new connection every time it's called, we're creating a new connection every time it's called after the substrate URLs are changed, only then we create the connection
| if flist := config.V4FlistURL; len(flist) > 0 { | ||
| env.FlistURL = flist | ||
| } | ||
| // if flist := config.V4FlistURL; len(flist) > 0 { |
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 should come from zos-config, no?
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.
check #38 (comment)
55db330 to
fc867d1
Compare
fc867d1 to
a6949c3
Compare
|
|
||
| func (r *RedisStream) Start(ctx context.Context) { | ||
| ps := NewProcessor(r.sub, r.process, NewFileState(r.state)) | ||
| r.processor = ps |
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.
does this deals with the already started stream? or we close the old one?
pkg/events/redis.go
Outdated
|
|
||
| func (r *RedisStream) UpdateSubstrateManager(sub substrate.Manager) { | ||
| r.sub = sub | ||
| r.processor.sub = sub |
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.
also here u r just changing sub and u didn't deal with the already started stream
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 change will not have any effect on the running connection as long as it is open and healty, if the connection is closed unexpectedly, then new connection will be created with the new manager
7692ea4 to
de1de8a
Compare
pkg/events/events.go
Outdated
| // if new update is received then the connection is broken as noded only issues new update only if | ||
| // the old manager is broken so we need to resubscribe with the new manager. | ||
| e.sub = newSub | ||
| log.Info().Msg("new substrate manager received, trying to resubscribe to the new manager") |
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.
that is not correct u r printing log that there is a new substrate manager that u will try
then after that directly u print an error which will confuse the user for sure
the error should be printed first then the info for trying new connection should come after when u r actually trying
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.
also can u plz print which urls r failing and which urls u r trying?
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.
will remove the log.Info message, will just log that the subscribtion failed and will try again, noded module will log that it was updated successfully once the update is done
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.
also both noded and api-gateway modules logs the new urls that will be used, and the error messages of the modules trying to reach the chain shows the faulty urls
pkg/events/events.go
Outdated
| // if new update is received then the connection is broken as noded only issues new update only if | ||
| // the old manager is broken so we need to resubscribe with the new manager. | ||
| e.sub = newSub | ||
| log.Info().Msg("new substrate manager received, trying to resubscribe to the new manager") |
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.
also can u plz print which urls r failing and which urls u r trying?
de1de8a to
b41b6f5
Compare
Description
support modules updating substrate/relay connections if the url in zos-config was updated.
Changes
Related Issues
Checklist