-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add DBSync Module #118
Add DBSync Module #118
Conversation
5e2b9dc
to
a53e57c
Compare
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.
Overall looks great! No blocking comments, left some minor things though
Height: r.config.TrustHeight, | ||
Hash: r.config.TrustHashBytes(), | ||
} | ||
if err := r.waitForEnoughPeers(ctx, 2); err != 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.
Might be helpful to add some info logs before and after this
@@ -1257,6 +1260,43 @@ func (cfg *InstrumentationConfig) ValidateBasic() error { | |||
return nil | |||
} | |||
|
|||
type DBSyncConfig struct { | |||
Enable bool `mapstructure:"db-sync-enable"` |
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.
Nice! Thanks for the changing the config name
@@ -31,6 +32,7 @@ type Application interface { | |||
OfferSnapshot(context.Context, *RequestOfferSnapshot) (*ResponseOfferSnapshot, error) // Offer a snapshot to the application | |||
LoadSnapshotChunk(context.Context, *RequestLoadSnapshotChunk) (*ResponseLoadSnapshotChunk, error) // Load a snapshot chunk | |||
ApplySnapshotChunk(context.Context, *RequestApplySnapshotChunk) (*ResponseApplySnapshotChunk, error) // Apply a shapshot chunk | |||
LoadLatest(context.Context, *RequestLoadLatest) (*ResponseLoadLatest, error) |
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.
Can we add a small comment for what this interface do? Like the above ones?
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.
added
config/config.go
Outdated
SnapshotDirectory: "", | ||
TimeoutInSeconds: 0, | ||
NoFileSleepInSeconds: 5, | ||
FileWorkerCount: 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.
Can we have better and more common defaults here? I'd suggest let's provide some ready to be used default value for:
- SnapshotInterval
- TimeoutInSeconds
- NoFileSleepInSeconds
- FileWorkerCount
- FileWorkerTimeout
- TrustPeriod
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.
updated
internal/dbsync/reactor.go
Outdated
syncer := NewSyncer(logger, config, baseConfig, reactor.requestMetadata, reactor.requestFile, reactor.commitState, reactor.postSync) | ||
reactor.syncer = syncer | ||
|
||
reactor.BaseService = *service.NewBaseService(logger, "BlockSync", reactor) |
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 be DbSync?
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.
nice catch
6b5c89a
to
37f0c27
Compare
b1dba5b
to
b03c748
Compare
b03c748
to
78ec60b
Compare
## Describe your changes and provide context This adds the midblock interfaces and module manager functionality to sei-cosmos ## Testing performed to validate your change Added unit test for module manager behavior and also tested with local sei with sei-chain changes
Describe your changes and provide context
Added a new
internal
module calleddbsync
that enables syncing application DB files directly. Its discovery mechanism resembles that ofstatesync
with some simplification. Still a WIPTesting performed to validate your change
unit tests
tested in loadtest cluster