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

refactor: modify the rpc addr startup args of all components #241

Merged
merged 11 commits into from
Feb 19, 2025

Conversation

daviderli614
Copy link
Member

@daviderli614 daviderli614 commented Feb 10, 2025

related issue: #240
wait for greptimedb release new version.

Summary by CodeRabbit

  • Refactor
    • Standardized RPC address configuration across the system for improved clarity.
    • Renamed command-line flags and configuration file keys to clearly distinguish between binding and server addresses.
    • Updated configuration structures for datanodes and flownodes to reflect new naming conventions.
    • Maintained existing functionality while streamlining configuration and deployment processes.

Copy link
Contributor

coderabbitai bot commented Feb 10, 2025

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (1)
  • tests/e2e/testdata/sql/cluster/flow_basic.sql is excluded by !tests/e2e/testdata/**

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The pull request updates various parts of the codebase by renaming RPC-related fields, variables, and command-line arguments across configuration generation, test files, deployment argument parsing, and database configuration structures. No underlying logic or control flow was modified; the changes solely reflect a standardization of naming conventions for clarity in specifying binding and server addresses.

Changes

File(s) Change Summary
cmd/initializer/internal/config_generator.go,
cmd/initializer/internal/config_generator_test.go,
cmd/initializer/internal/testdata/datanode-config.toml,
cmd/initializer/internal/testdata/flownode-config.toml
Renamed RPC-related fields and variables (e.g., RPCAddrRPCBindAddr, RPCHostNameRPCServerAddr) to update configuration parameters and associated test validations.
controllers/greptimedbcluster/deployers/frontend.go,
controllers/greptimedbcluster/deployers/meta.go,
controllers/greptimedbstandalone/deployer.go
Updated command-line flags by replacing outdated RPC address arguments (e.g., --rpc-addr, --bind-addr, --server-addr) with the new standardized forms (--rpc-bind-addr, --rpc-server-addr).
pkg/dbconfig/datanode_config.go,
pkg/dbconfig/flownode_config.go
Modified struct field declarations by removing deprecated RPC field names and introducing RPCBindAddr and RPCServerAddr for consistent configuration of datanode and flownode modules.

Possibly related PRs

  • refactor!(api): refine the default setting of *port field of cluster #170: The changes in the main PR, which involve renaming fields in the datanodeCfg and flownodeCfg structures, are related to the modifications in the retrieved PR that also focus on renaming service port fields in the GreptimeDBCluster and GreptimeDBStandalone structures, indicating a consistent effort to standardize naming conventions across the codebase.
  • refactor: sperate ports config from frontend instance and service #191: The changes in the main PR, which involve renaming fields related to RPC addresses in configuration structs, are related to the modifications in the retrieved PR that also involve updates to RPC-related configurations, specifically in how command-line arguments for RPC addresses are structured. Both PRs focus on the same RPC address concepts, indicating a direct connection at the code level.

Suggested reviewers

  • zyy17

Poem

I'm a hop-happy rabbit in a code-filled glen,
Renaming fields with a twitch of my whiskered pen.
No logic was harmed, just names set anew,
In config and test realms, clarity grew.
With joyful hops, I cheer this code day bright –
Coding carrots in order, all bounding in light!


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 generate docstrings to generate docstrings for this PR. (Beta)
  • @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.

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

🧹 Nitpick comments (1)
pkg/dbconfig/datanode_config.go (1)

27-29: Consider standardizing TOML mapping format.

While the field names are consistent with FlownodeConfig, the TOML mapping format differs:

  • FlownodeConfig uses: grpc.bind_addr
  • DatanodeConfig uses: rpc_bind_addr

This inconsistency might cause confusion.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3db8697 and f87cc06.

📒 Files selected for processing (9)
  • cmd/initializer/internal/config_generator.go (2 hunks)
  • cmd/initializer/internal/config_generator_test.go (2 hunks)
  • cmd/initializer/internal/testdata/datanode-config.toml (1 hunks)
  • cmd/initializer/internal/testdata/flownode-config.toml (1 hunks)
  • controllers/greptimedbcluster/deployers/frontend.go (1 hunks)
  • controllers/greptimedbcluster/deployers/meta.go (1 hunks)
  • controllers/greptimedbstandalone/deployer.go (1 hunks)
  • pkg/dbconfig/datanode_config.go (1 hunks)
  • pkg/dbconfig/flownode_config.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • cmd/initializer/internal/config_generator.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build the project
  • GitHub Check: Run e2e
🔇 Additional comments (7)
pkg/dbconfig/flownode_config.go (1)

25-27: LGTM! Clear and consistent field naming.

The new field names RPCBindAddr and RPCServerAddr better represent their purposes and align with standard networking terminology.

cmd/initializer/internal/config_generator_test.go (1)

83-90: LGTM! Tests properly updated.

Test cases have been correctly updated to reflect the new field names while maintaining the same test coverage and validation logic.

Also applies to: 92-99, 144-151, 153-160

controllers/greptimedbcluster/deployers/meta.go (1)

357-359: LGTM! Command-line arguments updated consistently.

The command-line argument names have been updated to match the new configuration field names, maintaining consistency across the codebase.

controllers/greptimedbcluster/deployers/frontend.go (1)

250-250: LGTM! RPC address argument updated.

The change from --rpc-addr to --rpc-bind-addr aligns with the standardization of RPC address arguments across components.

controllers/greptimedbstandalone/deployer.go (1)

400-400: LGTM! RPC address argument updated.

The change from --rpc-addr to --rpc-bind-addr maintains consistency with the frontend component and aligns with the standardization effort.

cmd/initializer/internal/testdata/flownode-config.toml (1)

5-6: LGTM! RPC configuration parameters updated.

The GRPC configuration parameters have been updated to use the new standardized naming:

  • addrbind_addr
  • hostnameserver_addr

The values remain unchanged, ensuring backward compatibility.

cmd/initializer/internal/testdata/datanode-config.toml (1)

4-5: LGTM! RPC configuration parameters updated.

The RPC configuration parameters have been updated to use the new standardized naming:

  • rpc_addrrpc_bind_addr
  • rpc_hostnamerpc_server_addr

The values remain unchanged, ensuring backward compatibility.

@daviderli614 daviderli614 marked this pull request as ready for review February 10, 2025 09:54
@daviderli614
Copy link
Member Author

daviderli614 commented Feb 18, 2025

enable-flow e2e test failed:

mysql> CREATE FLOW ngx_aggregation
    -> SINK TO ngx_statistics
    -> AS
    -> SELECT
    ->     status,
    ->     count(client) AS total_logs,
    ->     min(size) as min_size,
    ->     max(size) as max_size,
    ->     avg(size) as avg_size,
    ->     sum(case when `size` > 550::double then 1::double else 0::double end) as high_size_count,
    ->     date_bin(INTERVAL '1 minutes', access_time) as time_window,
    -> FROM ngx_access_log
    -> GROUP BY
    ->     status,
    ->     time_window;
ERROR 1149 (42000): (InvalidSyntax): Invalid SQL syntax: sql parser error: INTERVAL requires a unit after the literal value at Line: 11, Column: 34

issues: GreptimeTeam/greptimedb#5563

@daviderli614 daviderli614 requested a review from zyy17 February 19, 2025 03:56
Copy link
Collaborator

@zyy17 zyy17 left a comment

Choose a reason for hiding this comment

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

lgtm

@daviderli614 daviderli614 merged commit 846bad1 into GreptimeTeam:main Feb 19, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants