Skip to content
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(schema/indexer)!: implement start indexing #21636

Merged
merged 17 commits into from
Sep 23, 2024
Merged

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Sep 10, 2024

Description

This implements the start indexing functionality for cosmossdk.io/schema/indexer which 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 to BaseApp, 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...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers 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...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Introduced a new ScopeableLogger interface for enhanced contextual logging.
    • Added robust configuration structures for indexers, improving data handling flexibility.
    • Implemented an indexer manager for structured app data indexing.
  • Refactor

    • Streamlined indexer initialization processes and improved error handling.
    • Reorganized configuration management for indexers, enhancing clarity and maintainability.
  • Bug Fixes

    • Improved error handling during the registration of indexers.
  • Tests

    • Added unit tests for indexing functionality and configuration unmarshalling, ensuring robust validation.

Copy link
Contributor

coderabbitai bot commented Sep 10, 2024

Walkthrough

Walkthrough

The changes involve a 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 updated to ensure robust functionality across the new configurations and initialization processes.

Changes

File Path Change Summary
indexer/postgres/indexer.go Replaced StartIndexer with startIndexer, added init function, removed decodeConfig, and SqlLogger type definition.
indexer/postgres/tests/config.go Removed postgresConfigToIndexerConfig function, which converted PostgreSQL config to indexer format.
indexer/postgres/tests/init_schema_test.go Refactored testInitSchema to use IndexingOptions struct for configuration, simplifying logger instantiation.
indexer/postgres/tests/postgres_test.go Updated testPostgresIndexer to utilize IndexingOptions struct and modified listener references for consistency.
schema/decoding/resolver.go Renamed IterateAll method to AllDecoders in DecoderResolver interface and its implementation.
schema/decoding/resolver_test.go Updated test functions to use AllDecoders instead of IterateAll.
schema/decoding/sync.go Replaced resolver.IterateAll with resolver.AllDecoders for decoder iteration.
schema/indexer/config.go Introduced Config, FilterConfig, and ModuleFilterConfig structs for indexer configuration.
schema/indexer/indexer.go Replaced Config struct with Initializer struct focusing on initialization functions.
schema/indexer/manager.go Removed ManagerOptions, ManagerConfig, and StartManager function.
schema/indexer/registry.go Changed Register function to accept Initializer type, enhancing registration validation.
schema/indexer/registry_test.go Updated indexer registration tests to use Initializer struct.
schema/indexer/start.go Introduced IndexingOptions, IndexingConfig, IndexingTarget, and IndexerInfo structs, and implemented StartIndexing function.
schema/logutil/logger.go Introduced ScopeableLogger interface for contextual logging with WithContext method.

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
Loading

Suggested reviewers

  • testinginprod
  • tac0turtle

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    -- I pushed a fix in commit <commit_id>, please review it.
    -- Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    -- @coderabbitai generate unit testing code for this file.
    -- @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    -- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    -- @coderabbitai read src/utils.ts and generate unit testing code.
    -- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    -- @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

"context"
"encoding/json"
"fmt"
"reflect"

Check notice

Code scanning / CodeQL

Sensitive package import Note

Certain system packages contain functions which may be a possible source of non-determinism
Comment on lines 106 to 144
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

Iteration over map may be a possible source of non-determinism
@aaronc aaronc marked this pull request as ready for review September 12, 2024 18:30
Copy link
Contributor

@aaronc your pull request is missing a changelog!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between bd52dcf and 5459a26.

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 InitFunc within the Initializer is 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 Initializer instead of a map of InitFunc is necessary to accommodate the change in the Register function signature. It is consistent with the rest of the changes in the file.


18-18: LGTM!

The change in the indexerRegistry variable declaration to a map of Initializer instead of a map of InitFunc is 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 Initializer type instead of an InitFunc type 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 Initializer type.

Run the following script to verify the function usage:

Verification successful

Breaking change successfully implemented.

The change to the Register function signature to use the Initializer type has been correctly applied across the relevant parts of the codebase. All instances of indexer.Register are using the Initializer type as expected.

  • schema/indexer/registry.go: The Register function is correctly using the Initializer type.
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 Initializer struct improves code structure and maintainability. The InitFunc implementation 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.IterateAll to resolver.AllDecoders enhances 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 on opts.ModuleFilter.

The code segment also conforms to the Uber Go Style Guide.

indexer/postgres/indexer.go (2)

33-38: LGTM!

The init function correctly registers the startIndexer function as an initializer for the "postgres" indexer type. The usage of the init function for registration purposes aligns with the common practices in Go.


Line range hint 40-88: LGTM!

The startIndexer function 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 new Initializer struct and the shift towards a function-centric approach.

The introduction of the Initializer struct and the removal of the Config struct 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 InitFunc field encapsulates the initialization logic, while the ConfigType field specifies the expected configuration object type, providing clarity and type safety.

A few questions to consider:

  1. What was the motivation behind this change, and how does it improve upon the previous approach?
  2. How will the configuration options that were previously defined in the Config struct 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 Config struct 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 FilterConfig struct 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 testPostgresIndexer function. The shift from directly creating a configuration object to utilizing the indexer.StartIndexing function with an IndexingOptions struct provides a more modular and extensible approach.

The new configuration structure, which organizes the Postgres settings under a Target map, aligns well with best practices for clear and concise code structure, as recommended by the Uber Go Style Guide. The nesting of the DatabaseURL and DisableRetainDeletions parameters 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.Listener to res.Listener ensures 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"].View instead of pgIndexer.View guarantees 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 named IterateAll. The test logic and assertions remain the same, indicating that the behavior of AllDecoders is expected to be equivalent to IterateAll. 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 replaces IterateAll. The test logic and assertions are unchanged, suggesting that AllDecoders is expected to behave the same as IterateAll in the error scenario. This change should not impact the test coverage or results.

schema/indexer/start_test.go (5)

13-80: LGTM!

The TestStart function is a well-structured unit test that follows the AAA (Arrange, Act, Assert) pattern. It effectively tests the StartIndexing function by registering two indexers with custom configurations and checking if the commit listener is called the expected number of times. The test also uses a WaitGroup to 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 callCommit function 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 TestUnmarshalIndexingConfig function is a well-organized unit test that uses subtests to test the unmarshalIndexingConfig function 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 the unmarshalIndexingConfig function supports, ensuring the robustness of the function.


149-211: LGTM!

The TestUnmarshalIndexerConfig function is a well-organized unit test that uses subtests to test the unmarshalIndexerCustomConfig function 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 the unmarshalIndexerCustomConfig function supports, ensuring the robustness of the function.


213-219: LGTM!

The testConfig and testConfig2 structs 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 StartIndexing function 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 the appdata package.

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 unmarshalIndexingConfig function 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 the json package.


199-212: LGTM!

The unmarshalIndexerCustomConfig function 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 the json package, 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

Comment on lines +24 to +30
// 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{}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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
}

Comment on lines 42 to 52
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"`
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good have a question around the config

// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this be set in the app.toml or?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the idea is that this is a section in app.toml, probably under the [indexer] section

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we then add the corresponding toml struct tag then? (Like done in other server/v2 config struct)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which tags do we want? toml, mapstructure, json? One of these? All three?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So no json needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and added toml, mapstructure and comment and also kept JSON: c967f78. Does that look good?

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range and nitpick comments (4)
indexer/postgres/tests/testdata/init_schema_no_retain_delete.txt (2)

1-4: Improved logging for better observability

The addition of these logging statements enhances the observability of the indexing process. It's now clear when indexing starts and what type of indexer is being used.

Consider adding a timestamp to these log entries for better traceability. For example:

-INFO: Starting indexing
+INFO: [2023-09-21 10:15:30] Starting indexing

Line range hint 5-8: Clear logging of enum type creation

The addition of debug logs for enum type creation improves visibility into the schema initialization process. The enum types "test_my_enum" and "test_vote_type" are clearly defined.

For consistency, consider using the same format for both enum creation logs. For example:

 DEBUG: Creating enum type
   sql: CREATE TYPE "test_my_enum" AS ENUM ('a', 'b', 'c');
-DEBUG: Creating enum type
+DEBUG: Creating enum type
   sql: CREATE TYPE "test_vote_type" AS ENUM ('yes', 'no', 'abstain');
indexer/postgres/tests/testdata/init_schema.txt (2)

Line range hint 9-36: Table creation for "test_all_kinds" is well-structured

The table schema for "test_all_kinds" is comprehensive and well-designed, including a wide variety of data types which is excellent for testing purposes. The use of GENERATED ALWAYS AS for timestamp fields and a composite primary key (id, ts_nanos) are good practices.

Consider adding a comment to explain the purpose of this test table, especially since it includes such a wide variety of data types. This could be helpful for future maintainers.


Line range hint 37-52: Table creations for "test_singleton" and "test_vote" are well-implemented

Both table schemas are well-structured for their respective purposes. The "test_singleton" table correctly uses a CHECK constraint to ensure only one row can exist, and the "test_vote" table uses a composite primary key and includes a _deleted flag for soft deletes, which are good practices.

For the "test_vote" table, consider adding a foreign key constraint for the "proposal" column if it references another table. This would ensure referential integrity. If there's no reference table, you might want to add a comment explaining why.

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between b3e4280 and 544003a.

Files selected for processing (3)
  • indexer/postgres/tests/testdata/init_schema.txt (1 hunks)
  • indexer/postgres/tests/testdata/init_schema_no_retain_delete.txt (1 hunks)
  • schema/indexer/start.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • schema/indexer/start.go
Additional comments not posted (3)
indexer/postgres/tests/testdata/init_schema_no_retain_delete.txt (1)

Line range hint 9-52: Comprehensive table creation with clear logging

The addition of debug logs for table creation enhances the visibility of the schema initialization process. The table structures for "test_all_kinds", "test_singleton", and "test_vote" are well-defined with appropriate column types and constraints.

However, granting SELECT privileges to PUBLIC for all tables might be a security concern in a production environment. Please verify if this is intended behavior or if it's specific to the test environment.

If this is a common pattern in other SQL files, consider reviewing the access control strategy for the database.

indexer/postgres/tests/testdata/init_schema.txt (2)

1-4: Logging statements look good

The logging statements provide clear and useful information about the indexing process, including the target database type and name. The use of INFO and DEBUG log levels is appropriate for the content being logged.


Line range hint 5-8: Enum creation looks correct

The creation of enum types "test_my_enum" and "test_vote_type" is well-implemented. The SQL statements are correctly formatted, and the DEBUG level logging provides useful information for troubleshooting if needed.

@aaronc aaronc added this pull request to the merge queue Sep 23, 2024
Merged via the queue into main with commit d273ae0 Sep 23, 2024
74 of 75 checks passed
@aaronc aaronc deleted the aaronc/indexer-manager-impl1 branch September 23, 2024 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants