-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
feat(store/v2): build the migration manager in the root store factory #22336
base: main
Are you sure you want to change the base?
Changes from 36 commits
6362846
e7bd885
3bb541b
1e6e4f1
cb86a77
31eb8fd
79d978b
eb8029c
01a470e
f3091f5
19c6da3
f58431a
4ef10b7
b854054
ddaf4da
1b93050
03fdd2a
ae75111
a10a72f
5f2cb83
99a813a
2e13d91
2b9419e
2106a3b
010bc58
31a4aed
6ee0260
ba62dd5
cb687e2
e04b614
759cb53
1c5b07d
76aeebe
f9da53b
4f8ca79
9a55795
b5b2fa0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
syntax = "proto3"; | ||
package cosmos.store.v2; | ||
|
||
import "gogoproto/gogo.proto"; | ||
import "google/protobuf/timestamp.proto"; | ||
|
||
option go_package = "cosmossdk.io/store/v2/proof"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incorrect Go package path in The option on line 7 specifies the Go package path: option go_package = "cosmossdk.io/store/v2/proof"; Ensure that this path correctly reflects the actual package location. If the package is intended to be |
||
|
||
// CommitInfo defines commit information used by the multi-store when committing | ||
// a version/height. | ||
message CommitInfo { | ||
int64 version = 1; | ||
repeated StoreInfo store_infos = 2; | ||
google.protobuf.Timestamp timestamp = 3 [(gogoproto.nullable) = false, (gogoproto.stdtime) = true]; | ||
bytes commit_hash = 4; | ||
} | ||
|
||
// StoreInfo defines store-specific commit information. It contains a reference | ||
// between a store name and the commit ID. | ||
// | ||
message StoreInfo { | ||
string name = 1; | ||
CommitID commit_id = 2; | ||
string structure = 3; | ||
} | ||
|
||
// CommitID defines the commitment information when a specific store is | ||
// committed. | ||
message CommitID { | ||
option (gogoproto.goproto_stringer) = false; | ||
|
||
int64 version = 1; | ||
bytes hash = 2; | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the change here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when a component returned an error, the check would look at the first value in the channel and if its nil it would not continue to look for errors. This fix allows errors from components to be propagated to higher levels. The second case ctx.Done() allows the system to still catch ctrl+c as when first adding the loop it would hang on ctrl+c |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,8 +101,15 @@ func (s *Server[T]) Start(ctx context.Context) error { | |
}() | ||
} | ||
|
||
if err := <-resCh; err != nil { | ||
return fmt.Errorf("failed to start servers: %w", err) | ||
for i := 0; i < len(s.components); i++ { | ||
select { | ||
case err := <-resCh: | ||
if err != nil { | ||
return fmt.Errorf("failed to start servers: %w", err) | ||
} | ||
case <-ctx.Done(): | ||
return nil | ||
} | ||
Comment on lines
+104
to
+112
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Address potential goroutine leaks and error handling. The concurrent startup implementation has a few concerns:
Consider this improved implementation: for i := 0; i < len(s.components); i++ {
select {
case err := <-resCh:
if err != nil {
- return fmt.Errorf("failed to start servers: %w", err)
+ // Cancel context to stop other goroutines
+ return fmt.Errorf("failed to start component: %w", err)
}
case <-ctx.Done():
- return nil
+ // Wait for all goroutines to finish and collect errors
+ var startupErrors error
+ for j := i; j < len(s.components); j++ {
+ if err := <-resCh; err != nil {
+ startupErrors = errors.Join(startupErrors, err)
+ }
+ }
+ if startupErrors != nil {
+ return fmt.Errorf("startup incomplete due to context cancellation with errors: %w", startupErrors)
+ }
+ return ctx.Err()
}
} Additionally, consider adding timeouts for individual component startups: // At the start of the function
startupTimeout := time.Minute // or configurable
componentCtx, cancel := context.WithTimeout(ctx, startupTimeout)
defer cancel()
// Use componentCtx when starting components
go func(component ServerComponent[T]) {
resCh <- component.Start(componentCtx)
}(mod) |
||
} | ||
|
||
<-ctx.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.
Import statement refers to a non-existent file
The import on line 4:
refers to a file that does not exist, as indicated by the static analysis hint:
This will lead to compilation errors. Please ensure that the correct path to
gogo.proto
is specified and that the file exists in the expected location.🧰 Tools
🪛 buf (1.47.2)
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)