refactor!(api): refine the default setting of *port field of cluster
#170
Conversation
WalkthroughThe changes include significant updates to the GreptimeDB configurations, emphasizing the renaming of service port variables for clarity, the introduction of health endpoint specifications, and enhancements to dynamic configuration for liveness probes. The testing framework has been revised to utilize YAML data files, promoting maintainability and scalability. These updates collectively streamline the codebase and refine the deployment configurations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GreptimeDB as GreptimeDB
participant ClusterSpec as Cluster Specification
User->>ClusterSpec: Request Deployment
ClusterSpec->>GreptimeDB: Configure with new settings
GreptimeDB-->>ClusterSpec: Acknowledge configuration
ClusterSpec-->>User: Confirm deployment
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (2)
apis/v1alpha1/defaulting_test.go (2)
28-32: Ensure directory existence and handle errors gracefully.Before attempting to read directory entries, ensure that the directory exists to avoid potential errors. Consider adding a check for directory existence and handling errors more gracefully.
if _, err := os.Stat(testDir); os.IsNotExist(err) { t.Fatalf("directory %s does not exist", testDir) }
63-75: Consider using a more descriptive error message.The error message for unexpected results could be more descriptive by including the input file name. This will help in identifying which test case failed.
t.Errorf("unexpected result for %s (input: %s):\n%s", entry.Name(), inputFile, diffmatchpatch.New().DiffPrettyText(diffmatchpatch.New().DiffMain(string(rawExpectData), string(rawInputData), false)))
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (26)
- apis/v1alpha1/defaulting.go (9 hunks)
- apis/v1alpha1/defaulting_test.go (1 hunks)
- apis/v1alpha1/greptimedbcluster_types.go (4 hunks)
- apis/v1alpha1/greptimedbstandalone_types.go (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test00/expect.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test00/input.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test01/expect.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test01/input.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test02/expect.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test02/input.yaml (1 hunks)
- config/crd/resources/greptime.io_greptimedbclusters.yaml (7 hunks)
- config/crd/resources/greptime.io_greptimedbstandalones.yaml (4 hunks)
- controllers/greptimedbcluster/deployers/datanode.go (4 hunks)
- controllers/greptimedbcluster/deployers/flownode.go (4 hunks)
- controllers/greptimedbcluster/deployers/frontend.go (2 hunks)
- controllers/greptimedbcluster/deployers/meta.go (2 hunks)
- controllers/greptimedbstandalone/deployer.go (5 hunks)
- go.mod (3 hunks)
- manifests/greptimedb-operator-crd.yaml (11 hunks)
- manifests/greptimedb-operator-deployment.yaml (11 hunks)
- tests/e2e/greptimedbcluster_test.go (3 hunks)
- tests/e2e/greptimedbstandalone_test.go (1 hunks)
- tests/e2e/testdata/basic-cluster-with-flownode/cluster.yaml (1 hunks)
- tests/e2e/testdata/basic-cluster/cluster.yaml (1 hunks)
- tests/e2e/testdata/basic-standalone/standalone.yaml (1 hunks)
- tests/e2e/testdata/cluster-with-remote-wal/cluster.yaml (1 hunks)
Files skipped from review due to trivial changes (4)
- tests/e2e/testdata/basic-cluster-with-flownode/cluster.yaml
- tests/e2e/testdata/basic-cluster/cluster.yaml
- tests/e2e/testdata/basic-standalone/standalone.yaml
- tests/e2e/testdata/cluster-with-remote-wal/cluster.yaml
Additional context used
yamllint
apis/v1alpha1/testdata/greptimedbcluster/test02/expect.yaml
[error] 62-62: no new line character at the end of file
(new-line-at-end-of-file)
Additional comments not posted (56)
apis/v1alpha1/testdata/greptimedbcluster/test00/input.yaml (1)
1-16: Configuration aligns with PR objectives.The
GreptimeDBClusterconfiguration specifies thehttpPort, which aligns with the PR's goal of refining port settings. The setup appears consistent and clear.apis/v1alpha1/testdata/greptimedbcluster/test02/input.yaml (1)
1-19: Configuration aligns with PR objectives.The
GreptimeDBClusterconfiguration includesremoteWalwith Kafka broker endpoints, enhancing the cluster's functionality. This aligns with the PR's focus on clarity and functionality.apis/v1alpha1/testdata/greptimedbcluster/test01/input.yaml (1)
1-36: Configuration aligns with PR objectives.The
GreptimeDBClusterconfiguration includes resource requests and limits, and uses templates for different components. This aligns with the PR's focus on improving structure and maintainability.apis/v1alpha1/testdata/greptimedbcluster/test00/expect.yaml (1)
1-57: Configuration looks good!The YAML configuration for
GreptimeDBClusteris well-structured and includes appropriate liveness probes for health checks.apis/v1alpha1/testdata/greptimedbcluster/test01/expect.yaml (1)
1-93: Configuration looks good!The YAML configuration for
GreptimeDBClusteris comprehensive, including resource management and liveness probes for health checks.tests/e2e/greptimedbstandalone_test.go (1)
61-61: Verify the impact of the field name change.Ensure that all references to
MySQLServicePorthave been updated toMySQLPortthroughout the codebase to prevent any inconsistencies or runtime errors.Run the following script to verify the field name change:
Verification successful
Field Name Change Verified Successfully
The field name change from
MySQLServicePorttoMySQLPorthas been implemented consistently across the codebase. No occurrences of the old field name were found, and the new field name is used in relevant files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all occurrences of `MySQLServicePort` have been updated to `MySQLPort`. # Test: Search for the old field name. Expect: No occurrences of `MySQLServicePort`. rg --type go 'MySQLServicePort'Length of output: 31
Script:
#!/bin/bash # Description: Verify that `MySQLPort` is used in the codebase. # Test: Search for occurrences of `MySQLPort`. rg --type go 'MySQLPort'Length of output: 1883
go.mod (3)
12-12: Dependency addition:github.com/sergi/go-diff.The addition of
go-diffis appropriate for text diffing functionalities. Ensure that this library is used effectively in the codebase.
24-24: Dependency addition:sigs.k8s.io/yaml.The addition of
yamlis appropriate for YAML processing. Ensure that this library is used effectively in the codebase.
24-24: Verify the removal of the indirect dependency.Ensure that the removal of the indirect dependency for
sigs.k8s.io/yamldoes not affect other parts of the codebase that might rely on it.Run the following script to verify the impact of the removal:
apis/v1alpha1/greptimedbstandalone_types.go (1)
35-35: Field Renaming Approved: Ensure Consistent Usage.The renaming of service port fields to
HTTPPort,RPCPort,MySQLPort, andPostgreSQLPortimproves clarity and consistency. Ensure that all references to these fields in the codebase have been updated accordingly.Run the following script to verify the usage of these fields:
Also applies to: 38-38, 41-41, 44-44
Verification successful
Field Renaming Verified: Consistent Usage Across Codebase
The renaming of service port fields to
HTTPPort,RPCPort,MySQLPort, andPostgreSQLPorthas been consistently applied throughout the codebase. All references have been updated accordingly, ensuring clarity and uniformity.
- Verified in test files, controller files, command files, and API type definitions.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to renamed fields in the codebase. # Test: Search for the usage of renamed fields. Expect: Only occurrences of the new names. rg --type go -e 'HTTPPort|RPCPort|MySQLPort|PostgreSQLPort'Length of output: 10083
apis/v1alpha1/greptimedbcluster_types.go (1)
43-43: Field Renaming and Addition Approved: Ensure Consistent Usage.The renaming and addition of fields such as
RPCPort,HTTPPort,MySQLPort, andPostgreSQLPortimprove clarity and configurability. Ensure that all references to these fields in the codebase have been updated accordingly.Run the following script to verify the usage of these fields:
Also applies to: 47-47, 83-83, 87-87, 100-100, 132-132, 135-135, 138-138, 141-141
Verification successful
Field Renaming and Addition Verified: Consistent Usage Confirmed
The references to the fields
RPCPort,HTTPPort,MySQLPort, andPostgreSQLPorthave been consistently updated throughout the codebase. No outdated references were found, confirming that the renaming and addition have been properly applied.
- Verified in files such as
tests/e2e/greptimedbstandalone_test.go,controllers/greptimedbstandalone/deployer.go, and others.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to renamed and added fields in the codebase. # Test: Search for the usage of renamed and added fields. Expect: Only occurrences of the new names. rg --type go -e 'RPCPort|HTTPPort|MySQLPort|PostgreSQLPort'Length of output: 10083
tests/e2e/greptimedbcluster_test.go (1)
66-66: Port Reference Update Approved: Run Tests to Ensure Success.The update to use
MySQLPortin the test cases aligns with the renaming of fields in the specifications. Ensure that the tests pass with the updated port reference.Ensure that the tests are executed to verify their success with the updated port reference.
Also applies to: 126-126, 186-186
apis/v1alpha1/defaulting.go (4)
33-36: LGTM! Variable renaming improves clarity.The renaming of service port variables enhances clarity and consistency in the codebase.
209-211: LGTM! Dynamic probe reconfiguration enhances maintainability.The introduction of
reconfigureProbeensures that probe settings are dynamically updated based on the current configuration.
215-232: LGTM! Encapsulated probe configuration logic.The
reconfigureProbefunction encapsulates the logic for adjusting probe settings, enhancing maintainability.
Line range hint
233-257: LGTM! Consistent use of new health endpoint.The use of
defaultHealthEndpointinGreptimeDBStandalonealigns with the updated naming conventions and improves clarity.controllers/greptimedbcluster/deployers/frontend.go (3)
245-250: LGTM! Updated container arguments for new ports.The command-line arguments now correctly reference the new port names, ensuring proper configuration.
311-328: LGTM! Service port names updated for clarity.The service port definitions now use the new naming conventions, enhancing clarity and consistency.
336-353: LGTM! Container port names updated for clarity.The container port definitions now use the new naming conventions, enhancing clarity and consistency.
controllers/greptimedbcluster/deployers/meta.go (3)
338-340: LGTM! Updated container arguments for new ports.The command-line arguments now correctly reference the new port names, ensuring proper configuration.
349-356: LGTM! Service port names updated for clarity.The service port definitions now use the new naming conventions, enhancing clarity and consistency.
364-371: LGTM! Container port names updated for clarity.The container port definitions now use the new naming conventions, enhancing clarity and consistency.
controllers/greptimedbcluster/deployers/flownode.go (4)
240-240: LGTM!The change to use
Flownode.RPCPortaligns with the new naming convention and is correct.
275-275: LGTM!The change to use
Flownode.RPCPortaligns with the new naming convention and is correct.
351-353: LGTM!The change to use
Flownode.RPCPortand rename the service to "rpc" aligns with the new naming convention and is correct.
363-363: LGTM!The change to use
Flownode.RPCPortand rename the port to "rpc" aligns with the new naming convention and is correct.controllers/greptimedbstandalone/deployer.go (3)
Line range hint
355-370: LGTM!The renaming of service ports for clarity and consistency aligns with the new naming convention and is correct.
Line range hint
380-395: LGTM!The renaming of container ports for clarity and consistency aligns with the new naming convention and is correct.
404-407: LGTM!The update to use the new port names in the command-line arguments aligns with the new naming convention and is correct.
controllers/greptimedbcluster/deployers/datanode.go (5)
215-215: LGTM!The change to use
RPCPortin the URL construction aligns with the new naming convention and is correct.
443-444: LGTM!The update to use
RPCPortandHTTPPortin the command-line arguments aligns with the new naming convention and is correct.
501-501: LGTM!The change to use
Datanode.RPCPortaligns with the new naming convention and is correct.
588-595: LGTM!The change to use
Datanode.RPCPortandDatanode.HTTPPortaligns with the new naming convention and is correct.
603-610: LGTM!The change to use
Datanode.RPCPortandDatanode.HTTPPortaligns with the new naming convention and is correct.config/crd/resources/greptime.io_greptimedbstandalones.yaml (4)
2683-2683: Renaming tohttpPortimproves clarity.The change from
grpcServicePorttohttpPortbetter reflects the service's purpose.
2712-2712: Renaming tomysqlPortsimplifies the field name.The change from
mysqlServicePorttomysqlPortmaintains clarity and consistency.
2761-2761: Renaming topostgreSQLPortaligns with convention.The change from
postgresServicePorttopostgreSQLPortaligns with standard naming conventions.
2785-2787: Addition ofrpcPortenhances configuration flexibility.The new
rpcPortfield allows for specifying RPC-related configurations, adding flexibility.config/crd/resources/greptime.io_greptimedbclusters.yaml (6)
2695-2697: Addition ofhttpPortindatanodeis appropriate.The
httpPortfield is correctly added with a format ofint32.
2702-2704: Addition ofrpcPortindatanodeis appropriate.The
rpcPortfield is correctly added with a format ofint32.
5374-5376: Addition ofrpcPortinflownodeis appropriate.The
rpcPortfield is correctly added with a format ofint32.
10701-10703: Addition ofhttpPortinmetais appropriate.The
httpPortfield is correctly added with a format ofint32.
10708-10710: Addition ofrpcPortinmetais appropriate.The
rpcPortfield is correctly added with a format ofint32.
Line range hint
13351-13400: Renaming ofmysqlPortandpostgreSQLPortis appropriate.The renaming enhances clarity and consistency within the configuration.
manifests/greptimedb-operator-crd.yaml (6)
2694-2696: Addition ofhttpPortis consistent.The
httpPortproperty is correctly added with the appropriate type and format.
2701-2703: Addition ofrpcPortis consistent.The
rpcPortproperty is correctly added with the appropriate type and format.
5373-5375: Addition ofrpcPortis consistent.The
rpcPortproperty is correctly added with the appropriate type and format.
10700-10702: Addition ofhttpPortis consistent.The
httpPortproperty is correctly added with the appropriate type and format.
10707-10709: Addition ofrpcPortis consistent.The
rpcPortproperty is correctly added with the appropriate type and format.
Line range hint
16200-16304: Addition ofhttpPort,mysqlPort,postgreSQLPort, andrpcPortis consistent.The properties are correctly added with the appropriate type and format.
manifests/greptimedb-operator-deployment.yaml (6)
2701-2703: Addition ofhttpPortis well-structured.The
httpPortproperty is correctly defined withformat: int32andtype: integer.
2708-2710: Addition ofrpcPortis well-structured.The
rpcPortproperty is correctly defined withformat: int32andtype: integer.
5380-5382: Addition ofrpcPortinflownodeis well-structured.The
rpcPortproperty is correctly defined withformat: int32andtype: integer.
10707-10709: Addition ofhttpPortinmetais well-structured.The
httpPortproperty is correctly defined withformat: int32andtype: integer.
10714-10716: Addition ofrpcPortinmetais well-structured.The
rpcPortproperty is correctly defined withformat: int32andtype: integer.
Line range hint
13357-13406: Renaming of port properties enhances clarity.The renaming of
mysqlServicePorttomysqlPortandpostgresServicePorttopostgreSQLPortimproves consistency.
Comments failed to post (4)
apis/v1alpha1/testdata/greptimedbcluster/test02/expect.yaml
62-62: Add a newline at the end of the file.
The file is missing a newline at the end, which is a common convention in YAML files.
Apply this diff to add a newline:
+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.port: 4000Tools
yamllint
[error] 62-62: no new line character at the end of file
(new-line-at-end-of-file)
apis/v1alpha1/defaulting_test.go
34-40: Handle file read errors properly.
The error handling for reading files should use
t.Fatalfinstead oft.Errorfto stop the test execution if a file cannot be read. This ensures that the test does not proceed with invalid data.if err != nil { t.Fatalf("failed to read %s: %v", inputFile, err) }
42-46: Handle file read errors properly.
Similar to the input file, use
t.Fatalffor the expected file read error to ensure the test does not proceed with invalid data.if err != nil { t.Fatalf("failed to read %s: %v", expectFile, err) }apis/v1alpha1/defaulting.go
30-30: Fix typo in variable name.
The variable name
defautlHealthEndpointcontains a typo. It should bedefaultHealthEndpoint.Apply this diff to fix the typo:
-var defautlHealthEndpoint = "/health" +var defaultHealthEndpoint = "/health"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.var defaultHealthEndpoint = "/health"
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
apis/v1alpha1/testdata/greptimedbcluster/test02/expect.yaml (1)
62-62: Add a newline at the end of the file.The file lacks a newline at the end, which is a common convention for text files.
Apply this diff to add a newline:
- port: 4000 + port: 4000 +Tools
yamllint
[error] 62-62: no new line character at the end of file
(new-line-at-end-of-file)
controllers/greptimedbcluster/deployers/frontend.go (1)
Old Port Names Still Present in Codebase
The search results indicate that the old port name
HTTPServicePortis still present in several files. These occurrences need to be updated to the new naming conventions to maintain consistency across the codebase.
controllers/greptimedbstandalone/deployer.goapis/v1alpha1/greptimedbstandalone_types.goapis/v1alpha1/defaulting.goPlease update these references to use the new port naming conventions.
Analysis chain
Line range hint
245-353: LGTM! Verify the usage of updated port names.The changes improve clarity and consistency in port naming conventions.
Ensure that all references to the updated port names are correct throughout the codebase.
Run the following script to search for occurrences of the old port names:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for occurrences of old port names. # Test: Search for old port names. Expect: No occurrences of old port names. rg --type go -e 'GRPCServicePort|HTTPServicePort|MySQLServicePort|PostgresServicePort'Length of output: 569
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (26)
- apis/v1alpha1/defaulting.go (9 hunks)
- apis/v1alpha1/defaulting_test.go (1 hunks)
- apis/v1alpha1/greptimedbcluster_types.go (4 hunks)
- apis/v1alpha1/greptimedbstandalone_types.go (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test00/expect.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test00/input.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test01/expect.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test01/input.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test02/expect.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test02/input.yaml (1 hunks)
- config/crd/resources/greptime.io_greptimedbclusters.yaml (7 hunks)
- config/crd/resources/greptime.io_greptimedbstandalones.yaml (4 hunks)
- controllers/greptimedbcluster/deployers/datanode.go (4 hunks)
- controllers/greptimedbcluster/deployers/flownode.go (4 hunks)
- controllers/greptimedbcluster/deployers/frontend.go (2 hunks)
- controllers/greptimedbcluster/deployers/meta.go (2 hunks)
- controllers/greptimedbstandalone/deployer.go (5 hunks)
- go.mod (3 hunks)
- manifests/greptimedb-operator-crd.yaml (11 hunks)
- manifests/greptimedb-operator-deployment.yaml (11 hunks)
- tests/e2e/greptimedbcluster_test.go (3 hunks)
- tests/e2e/greptimedbstandalone_test.go (1 hunks)
- tests/e2e/testdata/basic-cluster-with-flownode/cluster.yaml (1 hunks)
- tests/e2e/testdata/basic-cluster/cluster.yaml (1 hunks)
- tests/e2e/testdata/basic-standalone/standalone.yaml (1 hunks)
- tests/e2e/testdata/cluster-with-remote-wal/cluster.yaml (1 hunks)
Files skipped from review due to trivial changes (9)
- apis/v1alpha1/greptimedbcluster_types.go
- apis/v1alpha1/testdata/greptimedbcluster/test00/expect.yaml
- apis/v1alpha1/testdata/greptimedbcluster/test00/input.yaml
- apis/v1alpha1/testdata/greptimedbcluster/test01/expect.yaml
- apis/v1alpha1/testdata/greptimedbcluster/test02/input.yaml
- config/crd/resources/greptime.io_greptimedbstandalones.yaml
- tests/e2e/testdata/basic-cluster-with-flownode/cluster.yaml
- tests/e2e/testdata/basic-cluster/cluster.yaml
- tests/e2e/testdata/cluster-with-remote-wal/cluster.yaml
Files skipped from review as they are similar to previous changes (12)
- apis/v1alpha1/defaulting.go
- apis/v1alpha1/greptimedbstandalone_types.go
- apis/v1alpha1/testdata/greptimedbcluster/test01/input.yaml
- controllers/greptimedbcluster/deployers/datanode.go
- controllers/greptimedbcluster/deployers/meta.go
- controllers/greptimedbstandalone/deployer.go
- go.mod
- manifests/greptimedb-operator-crd.yaml
- manifests/greptimedb-operator-deployment.yaml
- tests/e2e/greptimedbcluster_test.go
- tests/e2e/greptimedbstandalone_test.go
- tests/e2e/testdata/basic-standalone/standalone.yaml
Additional context used
yamllint
apis/v1alpha1/testdata/greptimedbcluster/test02/expect.yaml
[error] 62-62: no new line character at the end of file
(new-line-at-end-of-file)
Additional comments not posted (13)
apis/v1alpha1/defaulting_test.go (1)
28-77: LGTM! Verify the presence of test data files.The refactored test function enhances maintainability by using YAML files for test data.
Ensure that all necessary test data files are present in the
testdata/greptimedbclusterdirectory.Run the following script to list all test data directories and files:
controllers/greptimedbcluster/deployers/flownode.go (4)
240-240: LGTM! Consistent use of RPCPort.The change to use
RPCPortformetasrv-addrsis consistent with the new configuration strategy.
275-275: LGTM! Correct use of RPCPort in initializer.The change to use
RPCPortfor the--rpc-portargument in the initializer is appropriate.
351-353: LGTM! Updated service port configuration.The service port name change to "rpc" and use of
RPCPortis consistent with the updated configuration.
363-363: LGTM! Consistent container port configuration.The use of
RPCPortfor the container port configuration is consistent with the new setup.config/crd/resources/greptime.io_greptimedbclusters.yaml (8)
2695-2697: Addition ofhttpPortis consistent with objectives.The
httpPortproperty has been correctly added with the appropriate format and type.
2702-2704: Addition ofrpcPortis consistent with objectives.The
rpcPortproperty has been correctly added with the appropriate format and type.
5374-5376: Addition ofrpcPortis consistent with objectives.The
rpcPortproperty has been correctly added with the appropriate format and type.
10701-10703: Addition ofhttpPortis consistent with objectives.The
httpPortproperty has been correctly added with the appropriate format and type.
10708-10710: Addition ofrpcPortis consistent with objectives.The
rpcPortproperty has been correctly added with the appropriate format and type.
10681-10683: Addition ofhttpPortis consistent with objectives.The
httpPortproperty has been correctly added with the appropriate format and type.
13351-13353: Renaming tomysqlPortenhances clarity.The property has been correctly renamed with the appropriate format and type.
13400-13402: Renaming topostgreSQLPortenhances clarity.The property has been correctly renamed with the appropriate format and type.
zyy17
force-pushed
the
refactor/api
branch
2 times, most recently
from
6283522
to
5937a5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
apis/v1alpha1/testdata/greptimedbcluster/test02/expect.yaml (1)
66-66: Add a newline at the end of the file.The file should end with a newline character to adhere to POSIX standards.
Tools
yamllint
[error] 66-66: no new line character at the end of file
(new-line-at-end-of-file)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (26)
- apis/v1alpha1/defaulting.go (6 hunks)
- apis/v1alpha1/defaulting_test.go (1 hunks)
- apis/v1alpha1/greptimedbcluster_types.go (4 hunks)
- apis/v1alpha1/greptimedbstandalone_types.go (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test00/expect.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test00/input.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test01/expect.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test01/input.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test02/expect.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test02/input.yaml (1 hunks)
- config/crd/resources/greptime.io_greptimedbclusters.yaml (7 hunks)
- config/crd/resources/greptime.io_greptimedbstandalones.yaml (4 hunks)
- controllers/greptimedbcluster/deployers/datanode.go (4 hunks)
- controllers/greptimedbcluster/deployers/flownode.go (4 hunks)
- controllers/greptimedbcluster/deployers/frontend.go (2 hunks)
- controllers/greptimedbcluster/deployers/meta.go (2 hunks)
- controllers/greptimedbstandalone/deployer.go (5 hunks)
- go.mod (4 hunks)
- manifests/greptimedb-operator-crd.yaml (11 hunks)
- manifests/greptimedb-operator-deployment.yaml (11 hunks)
- tests/e2e/greptimedbcluster_test.go (3 hunks)
- tests/e2e/greptimedbstandalone_test.go (1 hunks)
- tests/e2e/testdata/basic-cluster-with-flownode/cluster.yaml (1 hunks)
- tests/e2e/testdata/basic-cluster/cluster.yaml (1 hunks)
- tests/e2e/testdata/basic-standalone/standalone.yaml (1 hunks)
- tests/e2e/testdata/cluster-with-remote-wal/cluster.yaml (1 hunks)
Files skipped from review due to trivial changes (6)
- apis/v1alpha1/testdata/greptimedbcluster/test00/expect.yaml
- apis/v1alpha1/testdata/greptimedbcluster/test00/input.yaml
- apis/v1alpha1/testdata/greptimedbcluster/test02/input.yaml
- config/crd/resources/greptime.io_greptimedbstandalones.yaml
- tests/e2e/testdata/basic-cluster/cluster.yaml
- tests/e2e/testdata/cluster-with-remote-wal/cluster.yaml
Files skipped from review as they are similar to previous changes (14)
- apis/v1alpha1/defaulting.go
- apis/v1alpha1/greptimedbstandalone_types.go
- apis/v1alpha1/testdata/greptimedbcluster/test01/expect.yaml
- apis/v1alpha1/testdata/greptimedbcluster/test01/input.yaml
- controllers/greptimedbcluster/deployers/datanode.go
- controllers/greptimedbcluster/deployers/meta.go
- controllers/greptimedbstandalone/deployer.go
- go.mod
- manifests/greptimedb-operator-crd.yaml
- manifests/greptimedb-operator-deployment.yaml
- tests/e2e/greptimedbcluster_test.go
- tests/e2e/greptimedbstandalone_test.go
- tests/e2e/testdata/basic-cluster-with-flownode/cluster.yaml
- tests/e2e/testdata/basic-standalone/standalone.yaml
Additional context used
yamllint
apis/v1alpha1/testdata/greptimedbcluster/test02/expect.yaml
[error] 66-66: no new line character at the end of file
(new-line-at-end-of-file)
Additional comments not posted (18)
apis/v1alpha1/defaulting_test.go (1)
27-83: Great use of data-driven testing!The refactor to use YAML files for test inputs and expected outputs enhances maintainability and scalability. The use of
reflect.DeepEqualanddiffmatchpatchfor comparison and diff visualization is a robust choice.apis/v1alpha1/greptimedbcluster_types.go (4)
41-47: Improved configurability forMetaSpec.The addition of
RPCPortandHTTPPortfields enhances the configurability and clarity of theMetaSpec.
81-87: Enhanced configurability forDatanodeSpec.The inclusion of
RPCPortandHTTPPortfields improves the configurability of theDatanodeSpec.
98-100: AddedRPCPorttoFlownodeSpec.The addition of the
RPCPortfield enhances the configurability of theFlownodeSpec.
132-141: Standardized naming conventions inGreptimeDBClusterSpec.The renaming of service port fields to
HTTPPort,RPCPort,MySQLPort, andPostgreSQLPortstandardizes naming conventions and clarifies their purpose.controllers/greptimedbcluster/deployers/frontend.go (3)
245-250: LGTM: Updated port references enhance clarity.The changes to use
RPCPort,HTTPPort,MySQLPort, andPostgreSQLPortimprove naming consistency.
311-328: LGTM: Updated service port names enhance clarity.The changes to use "rpc" and "pg" for service port names improve naming consistency.
336-353: LGTM: Updated container port names enhance clarity.The changes to use "rpc" and "pg" for container port names improve naming consistency.
controllers/greptimedbcluster/deployers/flownode.go (4)
240-240: LGTM: Updated address construction enhances clarity.The change to use
RPCPortfor constructing the metadata service address is consistent with the new naming conventions.
275-275: LGTM: Updated initializer arguments enhance clarity.The change to use
Flownode.RPCPortfor the RPC port configuration is consistent with the new naming conventions.
351-353: LGTM: Updated service port name enhances clarity.The change to use "rpc" for the service port name is consistent with the new naming conventions.
363-363: LGTM: Updated container port name enhances clarity.The change to use
Flownode.RPCPortfor the container port configuration is consistent with the new naming conventions.config/crd/resources/greptime.io_greptimedbclusters.yaml (6)
2695-2697: Addition ofhttpPortis correct.The
httpPortproperty is correctly added withformat: int32andtype: integer.
2702-2704: Addition ofrpcPortis correct.The
rpcPortproperty is correctly added withformat: int32andtype: integer.
5374-5376: Addition ofrpcPortis correct.The
rpcPortproperty is correctly added withformat: int32andtype: integer.
10701-10703: Addition ofhttpPortis correct.The
httpPortproperty is correctly added withformat: int32andtype: integer.
10708-10710: Addition ofrpcPortis correct.The
rpcPortproperty is correctly added withformat: int32andtype: integer.
Line range hint
13351-13426: Addition ofmysqlPort,postgreSQLPort, andrpcPortis correct.The properties are correctly added with
format: int32andtype: integer.
1. Refactor 'SetDefaults()' of GreptimeDBCluster to fix probe port setting issue; 2. Use livenessProbe instead of readinessProbe; 3. Refine the naming of `*port` filed in GreptimeDBCluster and GreptimeDBStandalone; 4. Refine the unit test structure of `defaulting_test.go`; Signed-off-by: zyy17 <zyylsxm@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (7)
apis/v1alpha1/defaulting_test.go (3)
18-24: Consider organizing imports.The imports could be organized for better readability, separating standard library imports from third-party ones.
27-37: Ensure test directory exists.Consider adding a check to ensure that the
testDirexists before attempting to read its contents, to provide a clearer error message if the directory is missing.entries, err := os.ReadDir(testDir) if err != nil { + t.Fatalf("test directory %s does not exist: %v", testDir, err) t.Fatal(err) }
39-83: Improve error messages for YAML processing.The error messages for YAML unmarshalling and file reading could include more context about the specific file being processed to aid in debugging.
if err := yaml.Unmarshal(inputData, &input); err != nil { - t.Fatalf("failed to unmarshal %s: %v", inputFile, err) + t.Fatalf("failed to unmarshal input file %s: %v", inputFile, err) } if err := yaml.Unmarshal(expectData, &expect); err != nil { - t.Fatalf("failed to unmarshal %s: %v", expectFile, err) + t.Fatalf("failed to unmarshal expected file %s: %v", expectFile, err) }controllers/greptimedbcluster/deployers/frontend.go (3)
245-250: Update Port Naming Conventions Across CodebaseThe codebase still contains references to the old port naming convention
HTTPServicePort. Please update these references to align with the new naming conventions.
controllers/greptimedbstandalone/deployer.go: Lines withHTTPServicePortapis/v1alpha1/greptimedbstandalone_types.go: Line withHTTPServicePortapis/v1alpha1/defaulting.go: Line withHTTPServicePortAnalysis chain
Clarify port configuration logic.
The changes to port configuration align with the new naming conventions. Ensure that all references to these ports are updated throughout the codebase.
Run the following script to verify the usage of these port variables:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the updated port variables. # Test: Search for the old port names. Expect: No occurrences of old port names. rg --type go 'GRPCServicePort|HTTPServicePort|MySQLServicePort|PostgresServicePort'Length of output: 566
336-353: Review and Update Container Port NamesThe container port names "grpc" and "postgres" are still present in several files. Please review and update these instances to ensure consistency across the codebase:
pkg/dbconfig/flownode_config.gocmd/initializer/internal/config_generator_test.gocontrollers/greptimedbstandalone/deployer.gocontrollers/greptimedbcluster/deployers/meta.gocontrollers/greptimedbcluster/deployers/frontend.gocontrollers/greptimedbcluster/deployers/flownode.goAnalysis chain
Ensure container port changes are reflected elsewhere.
The container port names have been updated. Verify that these changes are consistently applied across the codebase, particularly in deployment configurations.
Run the following script to verify the container port updates:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the updated container port names. # Test: Search for the old container port names. Expect: No occurrences of old container port names. rg --type go 'grpc|postgres'Length of output: 1180
311-328: Inconsistent Service Port Name Updates DetectedThe service port names have not been consistently updated across the codebase. The old service port names 'grpc' and 'postgres' are still present in several files, including deployment configurations.
pkg/dbconfig/flownode_config.gocontrollers/greptimedbcluster/deployers/frontend.gocontrollers/greptimedbcluster/deployers/flownode.gocontrollers/greptimedbstandalone/deployer.gocontrollers/greptimedbcluster/deployers/meta.gocmd/initializer/internal/config_generator_test.goPlease ensure these references are updated to reflect the new service port names.
Analysis chain
Ensure service port changes are reflected elsewhere.
The service port names have been updated. Verify that these changes are consistently applied across the codebase, particularly in deployment configurations.
Run the following script to verify the service port updates:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the updated service port names. # Test: Search for the old service port names. Expect: No occurrences of old service port names. rg --type go 'grpc|postgres'Length of output: 1180
controllers/greptimedbcluster/deployers/flownode.go (1)
351-353: Ensure consistent service port name updates across the codebase.The service port name "rpc" has been applied in many places, but there are still occurrences of the old name "grpc" in the following files. Please verify and update these occurrences to ensure consistency:
pkg/dbconfig/flownode_config.gocontrollers/greptimedbstandalone/deployer.gocontrollers/greptimedbcluster/deployers/flownode.goAnalysis chain
Ensure service port changes are reflected elsewhere.
The service port name has been updated. Verify that these changes are consistently applied across the codebase, particularly in deployment configurations.
Run the following script to verify the service port updates:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the updated service port names. # Test: Search for the old service port names. Expect: No occurrences of old service port names. rg --type go 'grpc'Length of output: 766
Script:
#!/bin/bash # Description: Verify all references to the new service port name "rpc". # Test: Search for occurrences of the new service port name "rpc". rg --type go 'rpc'Length of output: 4047
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (26)
- apis/v1alpha1/defaulting.go (6 hunks)
- apis/v1alpha1/defaulting_test.go (1 hunks)
- apis/v1alpha1/greptimedbcluster_types.go (4 hunks)
- apis/v1alpha1/greptimedbstandalone_types.go (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test00/expect.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test00/input.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test01/expect.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test01/input.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test02/expect.yaml (1 hunks)
- apis/v1alpha1/testdata/greptimedbcluster/test02/input.yaml (1 hunks)
- config/crd/resources/greptime.io_greptimedbclusters.yaml (7 hunks)
- config/crd/resources/greptime.io_greptimedbstandalones.yaml (4 hunks)
- controllers/greptimedbcluster/deployers/datanode.go (4 hunks)
- controllers/greptimedbcluster/deployers/flownode.go (4 hunks)
- controllers/greptimedbcluster/deployers/frontend.go (2 hunks)
- controllers/greptimedbcluster/deployers/meta.go (2 hunks)
- controllers/greptimedbstandalone/deployer.go (5 hunks)
- go.mod (4 hunks)
- manifests/greptimedb-operator-crd.yaml (11 hunks)
- manifests/greptimedb-operator-deployment.yaml (11 hunks)
- tests/e2e/greptimedbcluster_test.go (3 hunks)
- tests/e2e/greptimedbstandalone_test.go (1 hunks)
- tests/e2e/testdata/basic-cluster-with-flownode/cluster.yaml (1 hunks)
- tests/e2e/testdata/basic-cluster/cluster.yaml (1 hunks)
- tests/e2e/testdata/basic-standalone/standalone.yaml (1 hunks)
- tests/e2e/testdata/cluster-with-remote-wal/cluster.yaml (1 hunks)
Files skipped from review due to trivial changes (7)
- apis/v1alpha1/testdata/greptimedbcluster/test00/expect.yaml
- apis/v1alpha1/testdata/greptimedbcluster/test00/input.yaml
- apis/v1alpha1/testdata/greptimedbcluster/test01/expect.yaml
- apis/v1alpha1/testdata/greptimedbcluster/test02/input.yaml
- config/crd/resources/greptime.io_greptimedbstandalones.yaml
- tests/e2e/testdata/basic-cluster/cluster.yaml
- tests/e2e/testdata/cluster-with-remote-wal/cluster.yaml
Files skipped from review as they are similar to previous changes (15)
- apis/v1alpha1/defaulting.go
- apis/v1alpha1/greptimedbcluster_types.go
- apis/v1alpha1/greptimedbstandalone_types.go
- apis/v1alpha1/testdata/greptimedbcluster/test01/input.yaml
- apis/v1alpha1/testdata/greptimedbcluster/test02/expect.yaml
- config/crd/resources/greptime.io_greptimedbclusters.yaml
- controllers/greptimedbcluster/deployers/datanode.go
- controllers/greptimedbcluster/deployers/meta.go
- controllers/greptimedbstandalone/deployer.go
- go.mod
- manifests/greptimedb-operator-deployment.yaml
- tests/e2e/greptimedbcluster_test.go
- tests/e2e/greptimedbstandalone_test.go
- tests/e2e/testdata/basic-cluster-with-flownode/cluster.yaml
- tests/e2e/testdata/basic-standalone/standalone.yaml
Additional comments not posted (15)
controllers/greptimedbcluster/deployers/flownode.go (3)
275-275: Ensure initializer configuration changes are reflected elsewhere.The initializer configuration now uses
Flownode.RPCPort. Verify that these changes are consistently applied across the codebase.Run the following script to verify the initializer configuration updates:
Verification successful
Initializer Configuration Changes Verified
The initializer configuration change to use
Flownode.RPCPortis consistently applied across the codebase. No occurrences of the old configuration formatGRPCServicePortwere found.
Flownode.RPCPortis used in:
controllers/greptimedbcluster/deployers/flownode.goat multiple locations.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the initializer configuration. # Test: Search for the old initializer configuration. Expect: No occurrences of old configuration format. rg --type go 'GRPCServicePort'Length of output: 30
Script:
#!/bin/bash # Search for all occurrences of Flownode.RPCPort to verify its consistent application. rg --type go 'Flownode.RPCPort'Length of output: 353
363-363: Ensure container port changes are reflected elsewhere.The container port name has been updated. Verify that these changes are consistently applied across the codebase, particularly in deployment configurations.
Run the following script to verify the container port updates:
Verification successful
Container port changes are consistently applied.
The new container port name "RPCPort" is used consistently across the codebase, including in deployment configurations and other relevant files. The changes appear to be correctly reflected.
- The new container port name is used in multiple files, confirming consistent updates.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the updated container port names. # Test: Search for the old container port names. Expect: No occurrences of old container port names. rg --type go 'grpc'Length of output: 766
Script:
#!/bin/bash # Search for the new container port name in the codebase to verify its usage. rg --type go 'RPCPort'Length of output: 4866
240-240: Clarify metadata service address logic.The change to use
RPCPortfor the metadata service address aligns with the new configuration strategy. Ensure that all references to this address are updated throughout the codebase.Run the following script to verify the usage of the metadata service address:
Verification successful
Metadata service address logic is consistent.
The transition to using
RPCPortfor the metadata service address is complete and consistent across the codebase. No occurrences of the oldGRPCServicePortformat were found.
- The new
RPCPortformat is used in multiple files, includingcontrollers/greptimedbcluster/deployers/flownode.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the metadata service address. # Test: Search for the old metadata service address. Expect: No occurrences of old address format. rg --type go 'GRPCServicePort'Length of output: 30
Script:
#!/bin/bash # Description: Verify all references to the new metadata service address format. # Test: Search for the new metadata service address format. Expect: Occurrences of RPCPort. rg --type go 'RPCPort'Length of output: 4866
manifests/greptimedb-operator-crd.yaml (12)
2694-2696: Addition ofhttpPortproperty is appropriate.The introduction of the
httpPortproperty withint32format and integer type is consistent with the schema's naming conventions and type definitions.
2701-2703: Addition ofrpcPortproperty is appropriate.The introduction of the
rpcPortproperty withint32format and integer type is consistent with the schema's naming conventions and type definitions.
5373-5375: Addition ofrpcPortproperty is appropriate.The introduction of the
rpcPortproperty withint32format and integer type is consistent with the schema's naming conventions and type definitions.
10680-10682: Addition ofhttpPortproperty is appropriate.The introduction of the
httpPortproperty withint32format and integer type is consistent with the schema's naming conventions and type definitions.
10700-10702: Addition ofhttpPortproperty is appropriate.The introduction of the
httpPortproperty withint32format and integer type is consistent with the schema's naming conventions and type definitions.
10707-10709: Addition ofrpcPortproperty is appropriate.The introduction of the
rpcPortproperty withint32format and integer type is consistent with the schema's naming conventions and type definitions.
13350-13352: Addition ofmysqlPortproperty is appropriate.The introduction of the
mysqlPortproperty withint32format and integer type is consistent with the schema's naming conventions and type definitions.
13399-13401: Addition ofpostgreSQLPortproperty is appropriate.The introduction of the
postgreSQLPortproperty withint32format and integer type is consistent with the schema's naming conventions and type definitions.
13423-13425: Addition ofrpcPortproperty is appropriate.The introduction of the
rpcPortproperty withint32format and integer type is consistent with the schema's naming conventions and type definitions.
16200-16202: Addition ofhttpPortproperty is appropriate.The introduction of the
httpPortproperty withint32format and integer type is consistent with the schema's naming conventions and type definitions.
16229-16231: Addition ofmysqlPortproperty is appropriate.The introduction of the
mysqlPortproperty withint32format and integer type is consistent with the schema's naming conventions and type definitions.
16278-16280: Addition ofpostgreSQLPortproperty is appropriate.The introduction of the
postgreSQLPortproperty withint32format and integer type is consistent with the schema's naming conventions and type definitions.
What's changed
Refactor
SetDefaults()of GreptimeDBCluster to fix probe port setting issueFix The bug with the probe's port is always
4000even thehttpSericePortis not4000;Use livenessProbe instead of readinessProbe
The
/healthin GreptimeDB is more like a liveness probe.Refine the naming of
*portfiled in GreptimeDBCluster and GreptimeDBStandalone;Refine the unit test structure of
defaulting_test.go;Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores