feat(schema/indexer)!: implement start indexing #21636
Conversation
WalkthroughWalkthroughThe changes involve significant refactoring of the indexing system, particularly in the PostgreSQL indexer and its configuration management. Key modifications include the introduction of new structures for configuration, a shift from function-based to struct-based initialization, and enhancements to logging capabilities. Several functions and types were removed or renamed to improve clarity and maintainability. Additionally, unit tests were added to ensure robust functionality across the new configurations and initialization processes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant IndexerManager
participant Logger
User->>IndexerManager: Start Indexing
IndexerManager->>Logger: Initialize Logger
IndexerManager->>IndexerManager: Validate Configuration
IndexerManager->>IndexerManager: Register Indexers
IndexerManager->>User: Indexing Started
Possibly related PRs
Suggested reviewers
Recent review detailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| "context" | ||
| "encoding/json" | ||
| "fmt" | ||
| "reflect" |
Check notice
Code scanning / CodeQL
Sensitive package import Note
schema/indexer/start.go
Outdated
| for targetName, targetCfg := range cfg.Target { | ||
| init, ok := indexerRegistry[targetCfg.Type] | ||
| if !ok { | ||
| return IndexingTarget{}, fmt.Errorf("indexer type %q not found", targetCfg.Type) | ||
| } | ||
|
|
||
| logger.Info("Starting indexer", "target", targetName, "type", targetCfg.Type) | ||
|
|
||
| if targetCfg.Filter != nil { | ||
| return IndexingTarget{}, fmt.Errorf("indexer filter options are not supported yet") | ||
| } | ||
|
|
||
| childLogger := logger | ||
| if canScopeLogger { | ||
| childLogger = scopeableLogger.WithContext("indexer", targetName).(logutil.Logger) | ||
| } | ||
|
|
||
| targetCfg.Config, err = unmarshalIndexerCustomConfig(targetCfg.Config, init.ConfigType) | ||
| if err != nil { | ||
| return IndexingTarget{}, fmt.Errorf("failed to unmarshal indexer config for target %q: %w", targetName, err) | ||
| } | ||
|
|
||
| initRes, err := init.InitFunc(InitParams{ | ||
| Config: targetCfg, | ||
| Context: ctx, | ||
| Logger: childLogger, | ||
| AddressCodec: opts.AddressCodec, | ||
| }) | ||
| if err != nil { | ||
| return IndexingTarget{}, err | ||
| } | ||
|
|
||
| listener := initRes.Listener | ||
| listeners = append(listeners, listener) | ||
|
|
||
| indexerInfos[targetName] = IndexerInfo{ | ||
| View: initRes.View, | ||
| } | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
aaronc
requested review from
testinginprod,
a team and
cool-develope
as code owners
|
@aaronc your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (15)
- indexer/postgres/indexer.go (2 hunks)
- indexer/postgres/tests/config.go (0 hunks)
- indexer/postgres/tests/init_schema_test.go (1 hunks)
- indexer/postgres/tests/postgres_test.go (2 hunks)
- schema/decoding/resolver.go (2 hunks)
- schema/decoding/resolver_test.go (2 hunks)
- schema/decoding/sync.go (1 hunks)
- schema/indexer/config.go (1 hunks)
- schema/indexer/indexer.go (1 hunks)
- schema/indexer/manager.go (0 hunks)
- schema/indexer/registry.go (1 hunks)
- schema/indexer/registry_test.go (2 hunks)
- schema/indexer/start.go (1 hunks)
- schema/indexer/start_test.go (1 hunks)
- schema/logutil/logger.go (1 hunks)
Files not reviewed due to no reviewable changes (2)
- indexer/postgres/tests/config.go
- schema/indexer/manager.go
Files skipped from review due to trivial changes (1)
- schema/decoding/resolver.go
Additional context used
Path-based instructions (12)
schema/indexer/registry.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/indexer/registry_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"schema/logutil/logger.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/decoding/sync.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.indexer/postgres/indexer.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/indexer/indexer.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/indexer/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.indexer/postgres/tests/init_schema_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"indexer/postgres/tests/postgres_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"schema/decoding/resolver_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"schema/indexer/start_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"schema/indexer/start.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
GitHub Check: CodeQL
schema/indexer/start.go
[notice] 7-7: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism
[warning] 104-142: Iteration over map
Iteration over map may be a possible source of non-determinism
Additional comments not posted (30)
schema/indexer/registry.go (4)
11-13: LGTM!The additional check to ensure that the
InitFuncwithin theInitializeris not nil is a good addition. It enhances error handling during the registration of indexers and the specific error message will help in debugging if an indexer is registered without an initialization function.
15-15: LGTM!The change in the registry structure to a map of
Initializerinstead of a map ofInitFuncis necessary to accommodate the change in theRegisterfunction signature. It is consistent with the rest of the changes in the file.
18-18: LGTM!The change in the
indexerRegistryvariable declaration to a map ofInitializerinstead of a map ofInitFuncis necessary to accommodate the change in the registry structure. It is consistent with the rest of the changes in the file.
6-6: LGTM! Verify the impact of the breaking change.The change in the function signature to accept an
Initializertype instead of anInitFunctype is a good improvement. It likely encapsulates more information and improves the robustness of the registration mechanism.However, this is a breaking change. Please ensure that all the code that calls this function has been updated to pass the
Initializertype.Run the following script to verify the function usage:
Verification successful
Breaking change successfully implemented.
The change to the
Registerfunction signature to use theInitializertype has been correctly applied across the relevant parts of the codebase. All instances ofindexer.Registerare using theInitializertype as expected.
schema/indexer/registry.go: TheRegisterfunction is correctly using theInitializertype.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `Register` pass the `Initializer` type. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'indexer\.Register'Length of output: 317
Script:
#!/bin/bash # Description: Verify all function calls to `Register` use the `Initializer` type. # Search for all instances of the `Register` function in the codebase. rg --type go -A 5 'Register\('Length of output: 54666
schema/indexer/registry_test.go (4)
6-9: LGTM!The change from directly using a function to using an
Initializerstruct improves code structure and maintainability. TheInitFuncimplementation is a valid placeholder.
12-18: Great job using the comma-ok idiom!Using the comma-ok idiom to check for the existence of a key in a map is a more idiomatic and safer approach compared to directly comparing the value to
nil. The test cases cover both the positive and negative scenarios, which is excellent.
25-28: Excellent test case for duplicate registration!The test case correctly checks for the panic scenario when attempting to register an indexer with a duplicate name. Using the defer statement to check for the panic after the registration attempt is a good practice.
Line range hint
1-32: Code style adheres to the Uber Golang style guide.The code in this file follows the Uber Golang style guide. The test function name is clear and concise, and the test cases are well-organized, covering different scenarios. There are no deviations from the style guide.
schema/decoding/sync.go (1)
30-30: LGTM!The change from
resolver.IterateAlltoresolver.AllDecodersenhances code clarity by explicitly indicating that the method returns all decoders. This change does not introduce any functional differences or negatively impact the existing logic of filtering modules based onopts.ModuleFilter.The code segment also conforms to the Uber Go Style Guide.
indexer/postgres/indexer.go (2)
33-38: LGTM!The
initfunction correctly registers thestartIndexerfunction as an initializer for the "postgres" indexer type. The usage of theinitfunction for registration purposes aligns with the common practices in Go.
Line range hint
40-88: LGTM!The
startIndexerfunction correctly initializes the PostgreSQL indexer. The changes enhance the modularity of the code and simplify the initialization logic. The function follows the common patterns in database and indexer initialization. The implementation aligns with the Uber Golang style guide.schema/indexer/indexer.go (1)
12-18: Approve the newInitializerstruct and the shift towards a function-centric approach.The introduction of the
Initializerstruct and the removal of theConfigstruct aligns with the PR objectives and simplifies the indexer initialization process by focusing on the initialization function and its associated metadata.This change promotes a more streamlined and maintainable approach to indexer configuration. The
InitFuncfield encapsulates the initialization logic, while theConfigTypefield specifies the expected configuration object type, providing clarity and type safety.A few questions to consider:
- What was the motivation behind this change, and how does it improve upon the previous approach?
- How will the configuration options that were previously defined in the
Configstruct be managed going forward?Addressing these questions in the PR description or documentation would provide valuable context for the change and help ensure a smooth transition for users of the indexer package.
schema/indexer/config.go (2)
11-20: LGTM!The
Configstruct follows the Uber Golang style guide and is well-documented. The warning comment is helpful to prevent misuse of the struct.
23-39: LGTM!The
FilterConfigstruct follows the Uber Golang style guide and is well-documented. It provides a flexible way to filter the data stream.indexer/postgres/tests/init_schema_test.go (1)
36-49: LGTM!The refactoring of the indexing configuration setup improves the clarity and maintainability of the code. The changes align with the Uber Go Style Guide and maintain the same functionality.
indexer/postgres/tests/postgres_test.go (4)
57-68: Excellent refactoring for improved clarity and modularity!The changes in this code segment significantly enhance the clarity and organization of the Postgres indexer configuration and initialization process within the
testPostgresIndexerfunction. The shift from directly creating a configuration object to utilizing theindexer.StartIndexingfunction with anIndexingOptionsstruct provides a more modular and extensible approach.The new configuration structure, which organizes the Postgres settings under a
Targetmap, aligns well with best practices for clear and concise code structure, as recommended by the Uber Go Style Guide. The nesting of theDatabaseURLandDisableRetainDeletionsparameters within this structure results in a cleaner and more unified configuration management approach.Overall, these changes positively contribute to the maintainability and readability of the codebase without introducing any new functionality or altering the existing test logic. Great work!
74-74: Minor cleanup for removing unused variable.This change removes an unused error variable assignment, which is a small but positive cleanup. It aligns with the Uber Go Style Guide's recommendation to remove unused variables and helps keep the codebase tidy.
77-77: Necessary updates to align with the refactored code structure.The changes in these code segments are essential for aligning the test code with the refactored indexing setup and initialization process. Updating the listener reference from
pgIndexer.Listenertores.Listenerensures that the test logic interacts with the correct listener component after the refactoring.Similarly, modifying the retrieval of the indexer view to use
res.IndexerInfos["postgres"].Viewinstead ofpgIndexer.Viewguarantees that the test references the correct indexer view, taking into account the new structure introduced by the refactoring.These changes maintain the same functional behavior of the test while adapting to the refactored code structure, which is crucial for the test's correctness and reliability. The updates do not introduce any new functionality or alter the existing test logic, so they should not have any negative impact on the test coverage or behavior.
Also applies to: 85-87
104-104: Enhanced debugging capabilities with debug log in assertion error message.This change adds the debug log string to the assertion error message, which is a valuable improvement for debugging purposes. When an assertion fails, having the debug log information readily available in the error message provides more context and details, making it easier to identify and fix issues.
The change enhances the test's maintainability and troubleshooting capabilities without introducing any new functionality or altering the existing test logic. It should not have any negative impact on the test coverage or behavior.
schema/decoding/resolver_test.go (2)
46-46: LGTM!The change aligns the test with the updated method name
AllDecoders, which was previously namedIterateAll. The test logic and assertions remain the same, indicating that the behavior ofAllDecodersis expected to be equivalent toIterateAll. This change is unlikely to affect the test coverage or outcomes.
131-131: LGTM!Similar to the previous change, this update aligns the test with the new method name
AllDecoders, which replacesIterateAll. The test logic and assertions are unchanged, suggesting thatAllDecodersis expected to behave the same asIterateAllin the error scenario. This change should not impact the test coverage or results.schema/indexer/start_test.go (5)
13-80: LGTM!The
TestStartfunction is a well-structured unit test that follows the AAA (Arrange, Act, Assert) pattern. It effectively tests theStartIndexingfunction by registering two indexers with custom configurations and checking if the commit listener is called the expected number of times. The test also uses aWaitGroupto wait for the indexing to complete and a context to cancel the indexing, which are good practices for testing asynchronous code and graceful shutdown.
82-93: LGTM!The
callCommitfunction is a helpful utility that reduces code duplication in the tests. It calls the commit listener and checks for errors, failing the test if any occur. This is a good practice for ensuring the robustness of the tests.
95-147: LGTM!The
TestUnmarshalIndexingConfigfunction is a well-organized unit test that uses subtests to test theunmarshalIndexingConfigfunction with different input types. It checks if the unmarshaled config is equal to the expected config, which is a good way to test the correctness of the unmarshaling. The test covers all the input types that theunmarshalIndexingConfigfunction supports, ensuring the robustness of the function.
149-211: LGTM!The
TestUnmarshalIndexerConfigfunction is a well-organized unit test that uses subtests to test theunmarshalIndexerCustomConfigfunction with different input types. It checks if the unmarshaled config is equal to the expected config, which is a good way to test the correctness of the unmarshaling. The test covers all the input types that theunmarshalIndexerCustomConfigfunction supports, ensuring the robustness of the function.
213-219: LGTM!The
testConfigandtestConfig2structs are simple and focused, containing only the fields that are used in the tests. This is a good practice for keeping the tests maintainable. The structs also use JSON tags to specify the field names in the JSON representation, ensuring the correctness of the unmarshaling.schema/indexer/start.go (4)
83-169: LGTM!The
StartIndexingfunction is well-structured and follows the Uber Golang style guide. It correctly handles edge cases, such as nil logger and context, and returns errors appropriately. The function also sets up the indexer manager and middleware correctly using theappdatapackage.The static analysis hint about potential non-determinism due to iteration over a map is a false positive in this case, as the order of iteration does not affect the correctness of the function.
Tools
GitHub Check: CodeQL
[warning] 104-142: Iteration over map
Iteration over map may be a possible source of non-determinism
171-197: LGTM!The
unmarshalIndexingConfigfunction is well-structured and follows the Uber Golang style guide. It correctly handles different input types using type assertions and a type switch, and returns errors appropriately. The function also correctly marshals and unmarshals JSON using thejsonpackage.
199-212: LGTM!The
unmarshalIndexerCustomConfigfunction is well-structured and follows the Uber Golang style guide. It correctly handles the case when the input is already assignable to the expected type, and uses reflection to create a new instance of the expected type and assign the unmarshaled value to it. The function also correctly marshals and unmarshals JSON using thejsonpackage, and returns errors appropriately.
3-15: LGTM!The imports section follows the Uber Golang style guide. The imports are grouped into standard library and external packages, ordered alphabetically within each group, and do not use any aliases. There are no unused imports.
The static analysis hint about potential non-determinism due to the import of certain system packages is a false positive in this case, as the imported packages do not introduce any non-determinism in the code.
Tools
GitHub Check: CodeQL
[notice] 7-7: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism
| // ScopeableLogger is a logger that can be scoped with key/value pairs. | ||
| // It is implemented by all the loggers in cosmossdk.io/log. | ||
| type ScopeableLogger interface { | ||
| // WithContext returns a new logger with the provided key/value pairs set. | ||
| WithContext(keyVals ...interface{}) interface{} | ||
| } | ||
|
|
There was a problem hiding this comment.
Approve the addition of ScopeableLogger interface with a suggestion
The introduction of the ScopeableLogger interface and the WithContext method is a great addition to enhance the logging functionality by allowing loggers to be scoped with key/value pairs. The interface and method are well-documented, explaining their purpose and behavior clearly.
However, I suggest changing the return type of the WithContext method from interface{} to a concrete type that implements the ScopeableLogger interface. This aligns better with the Uber Golang Style Guide's recommendation to avoid returning interface types.
Consider updating the WithContext method signature to return a concrete type:
type ScopeableLogger interface {
- WithContext(keyVals ...interface{}) interface{}
+ WithContext(keyVals ...interface{}) ScopeableLogger
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ScopeableLogger is a logger that can be scoped with key/value pairs. | |
| // It is implemented by all the loggers in cosmossdk.io/log. | |
| type ScopeableLogger interface { | |
| // WithContext returns a new logger with the provided key/value pairs set. | |
| WithContext(keyVals ...interface{}) interface{} | |
| } | |
| // ScopeableLogger is a logger that can be scoped with key/value pairs. | |
| // It is implemented by all the loggers in cosmossdk.io/log. | |
| type ScopeableLogger interface { | |
| // WithContext returns a new logger with the provided key/value pairs set. | |
| WithContext(keyVals ...interface{}) ScopeableLogger | |
| } |
schema/indexer/config.go
Outdated
| type ModuleFilterConfig struct { | ||
| // Include specifies a list of modules whose state the indexer will | ||
| // receive state updates for. | ||
| // Only one of include or exclude modules should be specified. | ||
| Include []string `json:"include"` | ||
|
|
||
| // Exclude specifies a list of modules whose state the indexer will not | ||
| // receive state updates for. | ||
| // Only one of include or exclude modules should be specified. | ||
| Exclude []string `json:"exclude"` | ||
| } |
There was a problem hiding this comment.
Consider adding a validation method.
The ModuleFilterConfig struct follows the Uber Golang style guide and is well-documented. The warning comment is helpful to prevent misuse of the struct.
However, consider adding a validation method to ensure that only one of include or exclude modules is specified. This will help catch misconfigurations at runtime.
Here's an example of how you could implement the validation method:
func (c *ModuleFilterConfig) Validate() error {
if len(c.Include) > 0 && len(c.Exclude) > 0 {
return errors.New("only one of include or exclude modules should be specified")
}
return nil
}Then, you can call this method during the indexer initialization to validate the configuration.
| // NOTE: it is an error for an indexer to change its common options, such as adding | ||
| // or removing indexed modules, after the indexer has been initialized because this | ||
| // could result in an inconsistent state. | ||
| type Config struct { |
There was a problem hiding this comment.
will this be set in the app.toml or?
There was a problem hiding this comment.
Yes the idea is that this is a section in app.toml, probably under the [indexer] section
There was a problem hiding this comment.
Can we then add the corresponding toml struct tag then? (Like done in other server/v2 config struct)
There was a problem hiding this comment.
Which tags do we want? toml, mapstructure, json? One of these? All three?
There was a problem hiding this comment.
toml, mapstructure and comment: https://github.com/cosmos/cosmos-sdk/blob/main/server/v2/api/grpc/config.go
There was a problem hiding this comment.
I went ahead and added toml, mapstructure and comment and also kept JSON: c967f78. Does that look good?
Description
This implements the start indexing functionality for
cosmossdk.io/schema/indexerwhich will be called as part of server initialization and receive config parameters from the server config. With this in place, once we patch the changes back toBaseApp, state indexing will be more or less usable. There is a lot left to do but we will be able to start doing trial runs indexing real data.There are a few small breaking refactorings here to improve the ergonomics of the previous API. I'm expecting we'll want to get a few more API refactorings in before creating another tag to integrate with BaseApp.
There are tests for start indexing and config unmarshaling but a lot of the testing will need to be done with integration testing. The postgres indexer implementation has been updated to use this for its testing so we have a bit of that in this PR.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
ScopeableLoggerinterface for enhanced contextual logging.Refactor
Bug Fixes
Tests