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

Extension kubernetes registry #1395

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

wangchengming666
Copy link
Collaborator

@wangchengming666 wangchengming666 commented Jan 26, 2024

总体设计

image

样例

apiVersion: v1
kind: Pod
metadata:
  annotations:
   com.alipay.sofa.rpc.registry.kubernetes.TestService:1.0@DEFAULT: 'bolt://192.168.1.100:12200?accepts=100000&appName=TestAppName&weight=100&language=java&pid=41517&interface=com.alipay.sofa.rpc.registry.kubernetes.TestService&timeout=0&serialization=hessian2&protocol=bolt&delay=20&dynamic=true&startTime=1711021435028&id=rpc-cfg-0&uniqueId=&rpcVer=51300'
  creationTimestamp: "2024-02-02T08:47:54Z"
  generateName: sofa-rpc-k8s-provider-86f8565dcb-
  labels:
    com.alipay.sofa.rpc.registry.kubernetes.TestService:1.0@DEFAULT: ''
    app: sofa-rpc-k8s-provider
    pod-template-hash: 86f8565dcb
  name: sofa-rpc-k8s-provider-86f8565dcb-m7r6v
  namespace: sofa-rpc-k8s

环境准备

  1. 安装docker
    https://www.docker.com/
  2. 安装minikube
    https://kubernetes.io/zh-cn/docs/tutorials/hello-minikube/

代码准备

https://github.com/wangchengming666/sofa-rpc-registry-kubernetes-demo

测试手法

Provider部分

  1. 打包provider的镜像
    docker build -t sofa-rpc-k8s-provider:1.0.0 .
  2. 加载镜像
    minikube image load sofa-rpc-k8s-provider:1.0.0
  3. apply sa
    kubectl apply -f ServiceAccount.yml
  4. apply Deployment
    kubectl apply -f Deployment.yml
  5. 查看provider的pod
    kubectl get pods -n sofa-rpc-k8s
chengming@chengmingdeMacBook-Pro k8s % kubectl get pods -n sofa-rpc-k8s                                          
NAME                                     READY   STATUS    RESTARTS   AGE
sofa-rpc-k8s-provider-86f8565dcb-9z7hw   1/1     Running   0          7s
sofa-rpc-k8s-provider-86f8565dcb-jw4wg   1/1     Running   0          7s
sofa-rpc-k8s-provider-86f8565dcb-m7r6v   1/1     Running   0          7s
  1. 查看provider的日志
    kubectl logs -f sofa-rpc-k8s-provider-86f8565dcb-m7r6v -n sofa-rpc-k8s
chengming@chengmingdeMacBook-Pro k8s % kubectl logs -f sofa-rpc-k8s-provider-86f8565dcb-m7r6v  -n sofa-rpc-k8s    

  .   ____          _            __ _ _
 /\\ / ___'_ __ _ _(_)_ __  __ _ \ \ \ \
( ( )\___ | '_ | '_| | '_ \/ _` | \ \ \ \
 \\/  ___)| |_)| | | | | || (_| |  ) ) ) )
  '  |____| .__|_| |_|_| |_\__, | / / / /
 =========|_|==============|___/=/_/_/_/
 :: Spring Boot ::               (v2.4.13)

2024-02-02 08:48:02.078  INFO 1 --- [           main] com.example.appboot.AppbootApplication   : Starting AppbootApplication v0.0.1-SNAPSHOT using Java 1.8.0_342 on sofa-rpc-k8s-provider-86f8565dcb-m7r6v with PID 1 (/app/appboot-0.0.1-SNAPSHOT.jar started by root in /app)
2024-02-02 08:48:02.098  INFO 1 --- [           main] com.example.appboot.AppbootApplication   : No active profile set, falling back to default profiles: default
2024-02-02 08:48:12.561  INFO 1 --- [           main] o.s.b.w.embedded.tomcat.TomcatWebServer  : Tomcat initialized with port(s): 8080 (http)
2024-02-02 08:48:12.753  INFO 1 --- [           main] o.apache.catalina.core.StandardService   : Starting service [Tomcat]
2024-02-02 08:48:12.757  INFO 1 --- [           main] org.apache.catalina.core.StandardEngine  : Starting Servlet engine: [Apache Tomcat/9.0.55]
2024-02-02 08:48:13.470  INFO 1 --- [           main] o.a.c.c.C.[Tomcat].[localhost].[/myapp]  : Initializing Spring embedded WebApplicationContext
2024-02-02 08:48:13.470  INFO 1 --- [           main] w.s.c.ServletWebServerApplicationContext : Root WebApplicationContext: initialization completed in 10590 ms
2024-02-02 08:48:17.359  INFO 1 --- [           main] o.s.b.w.embedded.tomcat.TomcatWebServer  : Tomcat started on port(s): 8080 (http) with context path '/myapp'
2024-02-02 08:48:17.380  INFO 1 --- [           main] com.example.appboot.AppbootApplication   : Started AppbootApplication in 18.826 seconds (JVM running for 21.29)
AppbootApplication start
AppbootApplication start success

Consumer部分

  1. 打包consumer的镜像
    docker build -t sofa-rpc-k8s-consumer:1.0.0 .
  2. 加载镜像
    minikube image load sofa-rpc-k8s-consumer:1.0.0
  3. apply Deployment
    kubectl apply -f Deployment.yml
  4. 查看consumer的pod
    kubectl get pods -n sofa-rpc-k8s
chengming@chengmingdeMacBook-Pro k8s % kubectl get pods -n sofa-rpc-k8s                                                      
NAME                                     READY   STATUS    RESTARTS   AGE
sofa-rpc-k8s-consumer-5d6cd8698d-pf7wd   1/1     Running   0          4s
  1. 查看consumer的日志
    kubectl logs -f sofa-rpc-k8s-consumer-5d6cd8698d-pf7wd -n sofa-rpc-k8s
chengming@chengmingdeMacBook-Pro k8s % kubectl logs -f sofa-rpc-k8s-consumer-5d6cd8698d-pf7wd -n sofa-rpc-k8s   
Listening for transport dt_socket at address: 5005

  .   ____          _            __ _ _
 /\\ / ___'_ __ _ _(_)_ __  __ _ \ \ \ \
( ( )\___ | '_ | '_| | '_ \/ _` | \ \ \ \
 \\/  ___)| |_)| | | | | || (_| |  ) ) ) )
  '  |____| .__|_| |_|_| |_\__, | / / / /
 =========|_|==============|___/=/_/_/_/
 :: Spring Boot ::               (v2.4.13)

2024-02-02 08:58:31.304  INFO 1 --- [           main] c.s.e.s.SofaRpcK8sConsumerApplication    : Starting SofaRpcK8sConsumerApplication v0.0.1-SNAPSHOT using Java 1.8.0_342 on sofa-rpc-k8s-consumer-5d6cd8698d-pf7wd with PID 1 (/app/sofa-rpc-k8s-consumer-0.0.1-SNAPSHOT.jar started by root in /app)
2024-02-02 08:58:31.308  INFO 1 --- [           main] c.s.e.s.SofaRpcK8sConsumerApplication    : No active profile set, falling back to default profiles: default
2024-02-02 08:58:34.300  INFO 1 --- [           main] o.s.b.w.embedded.tomcat.TomcatWebServer  : Tomcat initialized with port(s): 8082 (http)
2024-02-02 08:58:34.320  INFO 1 --- [           main] o.apache.catalina.core.StandardService   : Starting service [Tomcat]
2024-02-02 08:58:34.320  INFO 1 --- [           main] org.apache.catalina.core.StandardEngine  : Starting Servlet engine: [Apache Tomcat/9.0.55]
2024-02-02 08:58:34.590  INFO 1 --- [           main] o.a.c.c.C.[Tomcat].[localhost].[/myapp]  : Initializing Spring embedded WebApplicationContext
2024-02-02 08:58:34.590  INFO 1 --- [           main] w.s.c.ServletWebServerApplicationContext : Root WebApplicationContext: initialization completed in 3165 ms
2024-02-02 08:58:36.019  INFO 1 --- [           main] o.s.b.w.embedded.tomcat.TomcatWebServer  : Tomcat started on port(s): 8082 (http) with context path '/myapp'
2024-02-02 08:58:36.077  INFO 1 --- [           main] c.s.e.s.SofaRpcK8sConsumerApplication    : Started SofaRpcK8sConsumerApplication in 5.676 seconds (JVM running for 6.578)
  1. 发起调用,在控制台可以看到如下日志,代表调用成功
start call sofa-rpc-k8s-provider
2024-02-04 02:12:40.364  WARN 1 --- [           main] io.fury.config.FuryBuilder               : Class registration isn't forced, unknown classes can be deserialized. If the environment isn't secure, please enable class registration by `FuryBuilder#requireClassRegistration(true)` or configure ClassChecker by `ClassResolver#setClassChecker`
2024-02-04 02:12:41.593  INFO 1 --- [           main] io.fury.Fury                             : Created new fury io.fury.Fury@4d02f94e
2024-02-04 02:12:41.675  WARN 1 --- [           main] .c.h.i.AbstractStringBuilderDeserializer : coder field not found or not accessible, will skip coder check, error is coder
call provider success chengming
call provider success chengming
call provider success chengming
call provider success chengming
call provider success chengming
call provider success chengming
call provider success chengming
call provider success chengming
call provider success chengming
call provider success chengming
call provider success chengming
call provider success chengming

Summary by CodeRabbit

  • New Features
    • Introduced Kubernetes registry support for service registration and discovery.
    • Added utility classes for Kubernetes client configuration and interaction.
    • Enhanced RegistryConfig with methods to retrieve parameters with default values.
  • Tests
    • Added tests for Kubernetes registry functionality, including service registration and discovery.
  • Documentation
    • Added documentation entries for Kubernetes registry support.

Copy link

codecov bot commented Jan 26, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 72.00%. Comparing base (df2dcae) to head (d6704a9).
Report is 1 commits behind head on master.

Files Patch % Lines
...ava/com/alipay/sofa/rpc/config/RegistryConfig.java 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1395      +/-   ##
============================================
- Coverage     72.02%   72.00%   -0.03%     
  Complexity      791      791              
============================================
  Files           422      422              
  Lines         17813    17816       +3     
  Branches       2769     2769              
============================================
- Hits          12830    12828       -2     
- Misses         3568     3570       +2     
- Partials       1415     1418       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nobodyiam
Copy link
Member

Looks interesting. Would you please briefly describe the behavior of the register and unRegister? It seems the information is written to the pod's annotation and label?

@wangchengming666
Copy link
Collaborator Author

Looks interesting. Would you please briefly describe the behavior of the register and unRegister? It seems the information is written to the pod's annotation and label?

sure, I will add the doc on this feature

@wangchengming666 wangchengming666 changed the title 【WIP】Extension Sofa registry kubernetes Extension Sofa registry kubernetes Feb 6, 2024
@wangchengming666
Copy link
Collaborator Author

wangchengming666 commented Feb 20, 2024

related sofastack/sofa-boot#1289

@Lihus
Copy link

Lihus commented Feb 20, 2024 via email

@zhenjunMa zhenjunMa closed this Feb 26, 2024
@zhenjunMa zhenjunMa reopened this Feb 26, 2024
@zhenjunMa zhenjunMa closed this Feb 26, 2024
@zhenjunMa zhenjunMa reopened this Feb 26, 2024
@zhenjunMa zhenjunMa closed this Feb 26, 2024
@zhenjunMa zhenjunMa reopened this Feb 26, 2024
@zhenjunMa zhenjunMa closed this Feb 26, 2024
@zhenjunMa zhenjunMa reopened this Feb 26, 2024
@zhenjunMa zhenjunMa closed this Feb 28, 2024
@zhenjunMa zhenjunMa reopened this Feb 28, 2024
@zhenjunMa zhenjunMa closed this Feb 28, 2024
@zhenjunMa zhenjunMa reopened this Feb 28, 2024
@zhenjunMa zhenjunMa closed this Feb 29, 2024
@zhenjunMa zhenjunMa reopened this Feb 29, 2024
@wangchengming666 wangchengming666 changed the title Extension Sofa registry kubernetes Extension kubernetes registry Mar 4, 2024
Copy link
Contributor

coderabbitai bot commented Mar 18, 2024

Walkthrough

The update introduces a Kubernetes registry to the SOFA RPC framework, enhancing service discovery and registration within Kubernetes environments. It adds support for configuring Kubernetes client settings, converting Kubernetes pods to provider information, and managing service provider updates based on pod changes. Additionally, it includes testing functionality for these new features, ensuring reliability and performance in Kubernetes deployments.

Changes

Files Change Summary
core/api/src/main/java/com/alipay/sofa/rpc/config/RegistryConfig.java Added overloaded getParameter methods for retrieving parameters with default values.
registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/... Introduced Kubernetes registry support, including service registration, discovery, and provider management.
registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/constant/KubernetesClientConstants.java
registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/utils/KubernetesClientUtils.java
registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/utils/KubernetesConfigUtils.java
Added Kubernetes client configuration utilities and constants.
registry/registry-kubernetes/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.registry.Registry Registered Kubernetes registry in service loader.
registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/... Provided tests for Kubernetes registry functionality.
registry/registry-kubernetes/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker Enabled mock-maker-inline for advanced mocking capabilities.

"In the land of code and Kubernetes pods,
A rabbit hopped through, leaving digital nods.
🐰🌟 With each leap and bound,
New registries were found,
And services danced, free from their clods.
So here's to changes, both big and small,
May our code run smoothly, bugs none at all."

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>.
    • Generate unit-tests 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 tests 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 generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

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.

Review Status

Actionable comments generated: 7

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between ac2a73e and 2da24a7.
Files ignored due to path filters (6)
  • all/pom.xml is excluded by: !**/*.xml
  • bom/pom.xml is excluded by: !**/*.xml
  • registry/pom.xml is excluded by: !**/*.xml
  • registry/registry-kubernetes/pom.xml is excluded by: !**/*.xml
  • registry/registry-kubernetes/src/test/resources/log4j.xml is excluded by: !**/*.xml
  • registry/registry-kubernetes/src/test/resources/sofa-rpc/rpc-config.json is excluded by: !**/*.json
Files selected for processing (14)
  • core/api/src/main/java/com/alipay/sofa/rpc/config/RegistryConfig.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistry.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryHelper.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryProviderWatcher.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/constant/KubernetesClientConstants.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/utils/KubernetesClientUtils.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/utils/KubernetesConfigUtils.java (1 hunks)
  • registry/registry-kubernetes/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.registry.Registry (1 hunks)
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryTest.java (1 hunks)
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestService.java (1 hunks)
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestService2.java (1 hunks)
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestServiceImpl.java (1 hunks)
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestServiceImpl2.java (1 hunks)
  • registry/registry-kubernetes/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker (1 hunks)
Files skipped from review due to trivial changes (3)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/constant/KubernetesClientConstants.java
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestService.java
  • registry/registry-kubernetes/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker
Additional comments: 10
registry/registry-kubernetes/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.registry.Registry (1)
  • 1-1: The service provider configuration correctly maps the kubernetes key to the KubernetesRegistry implementation. This is essential for the SOFA RPC framework to discover and use the Kubernetes registry at runtime.
registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestService2.java (1)
  • 1-22: The TestService2 interface is correctly defined and follows Java conventions. It provides a clear contract for a test service, which is essential for testing the Kubernetes registry functionality.
registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestServiceImpl.java (1)
  • 1-25: The TestServiceImpl class is correctly implemented, adhering to the TestService interface. It provides a straightforward implementation of the sayHello method, suitable for testing purposes.
registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestServiceImpl2.java (1)
  • 1-25: The TestServiceImpl2 class is correctly implemented, adhering to the TestService2 interface. It provides a straightforward implementation of the sayHello method, suitable for testing purposes.
registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/utils/KubernetesClientUtils.java (1)
  • 1-28: The KubernetesClientUtils class provides a crucial utility method buildKubernetesClient for creating Kubernetes clients with the provided configuration. This is essential for the Kubernetes registry functionality within the SOFA RPC framework.
registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryProviderWatcher.java (1)
  • 1-78: The KubernetesRegistryProviderWatcher class is well-implemented, providing functionality to manage provider listeners and update providers based on Kubernetes pod information. This is crucial for the dynamic nature of service discovery in Kubernetes environments.
registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryHelper.java (1)
  • 1-113: The KubernetesRegistryHelper class is well-implemented, offering essential functionality for converting Kubernetes pod information into ProviderInfo instances for service discovery. This plays a critical role in integrating the SOFA RPC framework with Kubernetes environments.
registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/utils/KubernetesConfigUtils.java (1)
  • 1-104: The KubernetesConfigUtils class provides essential functionality for creating Kubernetes configurations based on registry settings. This is crucial for the seamless integration of the SOFA RPC framework with Kubernetes environments.
core/api/src/main/java/com/alipay/sofa/rpc/config/RegistryConfig.java (1)
  • 427-428: The boolean getParameter method is correctly implemented and follows best practices by directly parsing the boolean value from the parameters map. This method is straightforward and does not require further refinement.
registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryTest.java (1)
  • 319-351: The testUpdatePodAnnotations test case is well-structured and covers the functionality of updating pod annotations. It correctly creates a pod, updates its annotations, and asserts the changes. This test case follows best practices for unit testing and effectively validates the intended behavior.

Comment on lines +84 to +91
// init kubernetes config
Config config = KubernetesConfigUtils.buildKubernetesConfig(registryConfig);
// init kubernetes client
if (kubernetesClient == null) {
this.kubernetesClient = KubernetesClientUtils.buildKubernetesClient(config);
}
// init Watcher
if (kubernetesRegistryProviderWatcher == null) {
kubernetesRegistryProviderWatcher = new KubernetesRegistryProviderWatcher();
}
this.currentHostname = System.getenv("HOSTNAME");
this.namespace = config.getNamespace();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Initialization of Config and KubernetesClient in the init method does not handle potential exceptions that might be thrown during the client's creation or configuration setup. It's important to ensure that any issues during initialization are properly logged or handled to avoid runtime failures.

Consider wrapping the client and config initialization code in a try-catch block to handle potential exceptions, logging any errors encountered during the process.

Comment on lines 104 to 129
public void register(ProviderConfig config) {
String appName = config.getAppName();
if (!registryConfig.isRegister()) {
if (LOGGER.isInfoEnabled(appName)) {
LOGGER.infoWithApp(appName, LogCodes.getLog(LogCodes.INFO_REGISTRY_IGNORE));
}
return;
}

if (config.isRegister()) {
PodResource podResource = kubernetesClient.pods()
.inNamespace(namespace)
.withName(currentHostname);

List<ServerConfig> serverConfigs = config.getServer();

if (CommonUtils.isNotEmpty(serverConfigs)) {
for (ServerConfig serverConfig : serverConfigs) {
String dataId = KubernetesRegistryHelper.buildDataId(config, serverConfig.getProtocol());
providerInstances.put(dataId, config);

podResource.edit(pod -> new PodBuilder(pod).editOrNewMetadata()
// 将ProviderConfig存在Annotations上
.addToAnnotations(dataId, JSON.toJSONString(config, true))
.endMetadata().build());
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The register method directly modifies pod annotations to store ProviderConfig information. This approach tightly couples the service registration logic with the Kubernetes pod annotations, which might not be ideal for all deployment scenarios. Additionally, there's no error handling for the Kubernetes client operations, which could lead to silent failures.

Consider abstracting the storage mechanism for ProviderConfig and adding error handling for Kubernetes client operations to ensure robustness and flexibility.

+ try {
+     podResource.edit(pod -> new PodBuilder(pod).editOrNewMetadata()
+             .addToAnnotations(dataId, JSON.toJSONString(config, true))
+             .endMetadata().build());
+ } catch (Exception e) {
+     LOGGER.error("Error registering provider config", e);
+ }

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.

Suggested change
public void register(ProviderConfig config) {
String appName = config.getAppName();
if (!registryConfig.isRegister()) {
if (LOGGER.isInfoEnabled(appName)) {
LOGGER.infoWithApp(appName, LogCodes.getLog(LogCodes.INFO_REGISTRY_IGNORE));
}
return;
}
if (config.isRegister()) {
PodResource podResource = kubernetesClient.pods()
.inNamespace(namespace)
.withName(currentHostname);
List<ServerConfig> serverConfigs = config.getServer();
if (CommonUtils.isNotEmpty(serverConfigs)) {
for (ServerConfig serverConfig : serverConfigs) {
String dataId = KubernetesRegistryHelper.buildDataId(config, serverConfig.getProtocol());
providerInstances.put(dataId, config);
podResource.edit(pod -> new PodBuilder(pod).editOrNewMetadata()
// 将ProviderConfig存在Annotations上
.addToAnnotations(dataId, JSON.toJSONString(config, true))
.endMetadata().build());
}
}
}
public void register(ProviderConfig config) {
String appName = config.getAppName();
if (!registryConfig.isRegister()) {
if (LOGGER.isInfoEnabled(appName)) {
LOGGER.infoWithApp(appName, LogCodes.getLog(LogCodes.INFO_REGISTRY_IGNORE));
}
return;
}
if (config.isRegister()) {
PodResource podResource = kubernetesClient.pods()
.inNamespace(namespace)
.withName(currentHostname);
List<ServerConfig> serverConfigs = config.getServer();
if (CommonUtils.isNotEmpty(serverConfigs)) {
for (ServerConfig serverConfig : serverConfigs) {
String dataId = KubernetesRegistryHelper.buildDataId(config, serverConfig.getProtocol());
providerInstances.put(dataId, config);
try {
podResource.edit(pod -> new PodBuilder(pod).editOrNewMetadata()
// 将ProviderConfig存在Annotations上
.addToAnnotations(dataId, JSON.toJSONString(config, true))
.endMetadata().build());
} catch (Exception e) {
LOGGER.error("Error registering provider config", e);
}
}
}
}

Comment on lines 135 to 160
public void unRegister(ProviderConfig config) {
String appName = config.getAppName();
if (!registryConfig.isRegister()) {
if (LOGGER.isInfoEnabled(appName)) {
LOGGER.infoWithApp(appName, LogCodes.getLog(LogCodes.INFO_REGISTRY_IGNORE));
}
return;
}

if (config.isRegister()) {
PodResource podResource = kubernetesClient.pods()
.inNamespace(namespace)
.withName(currentHostname);

List<ServerConfig> serverConfigs = config.getServer();
if (CommonUtils.isNotEmpty(serverConfigs)) {
for (ServerConfig serverConfig : serverConfigs) {
String dataId = KubernetesRegistryHelper.buildDataId(config, serverConfig.getProtocol());
providerInstances.remove(dataId);

podResource.edit(pod -> new PodBuilder(pod).editOrNewMetadata()
.removeFromAnnotations(dataId)
.endMetadata()
.build());
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the register method, unRegister also directly interacts with pod annotations without error handling. This could lead to issues if the Kubernetes client operations fail.

Implement error handling for Kubernetes client operations in the unRegister method to ensure reliability.

+ try {
+     podResource.edit(pod -> new PodBuilder(pod).editOrNewMetadata()
+             .removeFromAnnotations(dataId)
+             .endMetadata()
+             .build());
+ } catch (Exception e) {
+     LOGGER.error("Error unregistering provider config", e);
+ }

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.

Suggested change
public void unRegister(ProviderConfig config) {
String appName = config.getAppName();
if (!registryConfig.isRegister()) {
if (LOGGER.isInfoEnabled(appName)) {
LOGGER.infoWithApp(appName, LogCodes.getLog(LogCodes.INFO_REGISTRY_IGNORE));
}
return;
}
if (config.isRegister()) {
PodResource podResource = kubernetesClient.pods()
.inNamespace(namespace)
.withName(currentHostname);
List<ServerConfig> serverConfigs = config.getServer();
if (CommonUtils.isNotEmpty(serverConfigs)) {
for (ServerConfig serverConfig : serverConfigs) {
String dataId = KubernetesRegistryHelper.buildDataId(config, serverConfig.getProtocol());
providerInstances.remove(dataId);
podResource.edit(pod -> new PodBuilder(pod).editOrNewMetadata()
.removeFromAnnotations(dataId)
.endMetadata()
.build());
}
}
}
}
public void unRegister(ProviderConfig config) {
String appName = config.getAppName();
if (!registryConfig.isRegister()) {
if (LOGGER.isInfoEnabled(appName)) {
LOGGER.infoWithApp(appName, LogCodes.getLog(LogCodes.INFO_REGISTRY_IGNORE));
}
return;
}
if (config.isRegister()) {
PodResource podResource = kubernetesClient.pods()
.inNamespace(namespace)
.withName(currentHostname);
List<ServerConfig> serverConfigs = config.getServer();
if (CommonUtils.isNotEmpty(serverConfigs)) {
for (ServerConfig serverConfig : serverConfigs) {
String dataId = KubernetesRegistryHelper.buildDataId(config, serverConfig.getProtocol());
providerInstances.remove(dataId);
try {
podResource.edit(pod -> new PodBuilder(pod).editOrNewMetadata()
.removeFromAnnotations(dataId)
.endMetadata()
.build());
} catch (Exception e) {
LOGGER.error("Error unregistering provider config", e);
}
}
}
}
}

Comment on lines 177 to 220
public List<ProviderGroup> subscribe(ConsumerConfig config) {
String appName = config.getAppName();
if (!registryConfig.isSubscribe()) {
// registry ignored
if (LOGGER.isInfoEnabled(appName)) {
LOGGER.infoWithApp(appName, LogCodes.getLog(LogCodes.INFO_REGISTRY_IGNORE));
}
return null;
}

if (config.isSubscribe()) {

ProviderInfoListener providerInfoListener = config.getProviderInfoListener();
kubernetesRegistryProviderWatcher.addProviderListener(config, providerInfoListener);

FilterWatchListDeletable<Pod, PodList, PodResource> podPodListPodResourceFilterWatchListDeletable =
kubernetesClient.pods()
.inNamespace(namespace);

SharedIndexInformer<Pod> inform = podPodListPodResourceFilterWatchListDeletable.inform(new ResourceEventHandler<Pod>() {
@Override
public void onAdd(Pod pod) {
kubernetesRegistryProviderWatcher.updateProviders(config, getPods());
}

@Override
public void onUpdate(Pod pod, Pod t1) {
kubernetesRegistryProviderWatcher.updateProviders(config, getPods());
}

@Override
public void onDelete(Pod pod, boolean b) {
kubernetesRegistryProviderWatcher.updateProviders(config, getPods());
}
});

consumerListeners.put(config, inform);

inform.start();

List<Pod> pods = podPodListPodResourceFilterWatchListDeletable.list().getItems();
List<ProviderInfo> providerInfos = KubernetesRegistryHelper.convertPodsToProviders(pods, config);
List<ProviderInfo> matchProviders = RegistryUtils.matchProviderInfos(config, providerInfos);

return Collections.singletonList(new ProviderGroup().addAll(matchProviders));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The subscribe method lacks error handling for Kubernetes client operations and does not validate the returned pod list before converting it to provider infos. This could lead to unexpected behavior if the pod list is null or the Kubernetes client operations fail.

Add error handling and validation for the pod list in the subscribe method to ensure stability and correctness.

+ try {
+     List<Pod> pods = podPodListPodResourceFilterWatchListDeletable.list().getItems();
+     if (pods != null) {
+         List<ProviderInfo> providerInfos = KubernetesRegistryHelper.convertPodsToProviders(pods, config);
+         List<ProviderInfo> matchProviders = RegistryUtils.matchProviderInfos(config, providerInfos);
+         return Collections.singletonList(new ProviderGroup().addAll(matchProviders));
+     } else {
+         LOGGER.warn("No pods found for subscription");
+         return Collections.emptyList();
+     }
+ } catch (Exception e) {
+     LOGGER.error("Error subscribing to provider infos", e);
+     return Collections.emptyList();
+ }

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.

Suggested change
public List<ProviderGroup> subscribe(ConsumerConfig config) {
String appName = config.getAppName();
if (!registryConfig.isSubscribe()) {
// registry ignored
if (LOGGER.isInfoEnabled(appName)) {
LOGGER.infoWithApp(appName, LogCodes.getLog(LogCodes.INFO_REGISTRY_IGNORE));
}
return null;
}
if (config.isSubscribe()) {
ProviderInfoListener providerInfoListener = config.getProviderInfoListener();
kubernetesRegistryProviderWatcher.addProviderListener(config, providerInfoListener);
FilterWatchListDeletable<Pod, PodList, PodResource> podPodListPodResourceFilterWatchListDeletable =
kubernetesClient.pods()
.inNamespace(namespace);
SharedIndexInformer<Pod> inform = podPodListPodResourceFilterWatchListDeletable.inform(new ResourceEventHandler<Pod>() {
@Override
public void onAdd(Pod pod) {
kubernetesRegistryProviderWatcher.updateProviders(config, getPods());
}
@Override
public void onUpdate(Pod pod, Pod t1) {
kubernetesRegistryProviderWatcher.updateProviders(config, getPods());
}
@Override
public void onDelete(Pod pod, boolean b) {
kubernetesRegistryProviderWatcher.updateProviders(config, getPods());
}
});
consumerListeners.put(config, inform);
inform.start();
List<Pod> pods = podPodListPodResourceFilterWatchListDeletable.list().getItems();
List<ProviderInfo> providerInfos = KubernetesRegistryHelper.convertPodsToProviders(pods, config);
List<ProviderInfo> matchProviders = RegistryUtils.matchProviderInfos(config, providerInfos);
return Collections.singletonList(new ProviderGroup().addAll(matchProviders));
}
public List<ProviderGroup> subscribe(ConsumerConfig config) {
String appName = config.getAppName();
if (!registryConfig.isSubscribe()) {
// registry ignored
if (LOGGER.isInfoEnabled(appName)) {
LOGGER.infoWithApp(appName, LogCodes.getLog(LogCodes.INFO_REGISTRY_IGNORE));
}
return null;
}
if (config.isSubscribe()) {
ProviderInfoListener providerInfoListener = config.getProviderInfoListener();
kubernetesRegistryProviderWatcher.addProviderListener(config, providerInfoListener);
FilterWatchListDeletable<Pod, PodList, PodResource> podPodListPodResourceFilterWatchListDeletable =
kubernetesClient.pods()
.inNamespace(namespace);
SharedIndexInformer<Pod> inform = podPodListPodResourceFilterWatchListDeletable.inform(new ResourceEventHandler<Pod>() {
@Override
public void onAdd(Pod pod) {
kubernetesRegistryProviderWatcher.updateProviders(config, getPods());
}
@Override
public void onUpdate(Pod pod, Pod t1) {
kubernetesRegistryProviderWatcher.updateProviders(config, getPods());
}
@Override
public void onDelete(Pod pod, boolean b) {
kubernetesRegistryProviderWatcher.updateProviders(config, getPods());
}
});
consumerListeners.put(config, inform);
inform.start();
try {
List<Pod> pods = podPodListPodResourceFilterWatchListDeletable.list().getItems();
if (pods != null) {
List<ProviderInfo> providerInfos = KubernetesRegistryHelper.convertPodsToProviders(pods, config);
List<ProviderInfo> matchProviders = RegistryUtils.matchProviderInfos(config, providerInfos);
return Collections.singletonList(new ProviderGroup().addAll(matchProviders));
} else {
LOGGER.warn("No pods found for subscription");
return Collections.emptyList();
}
} catch (Exception e) {
LOGGER.error("Error subscribing to provider infos", e);
return Collections.emptyList();
}
}

Comment on lines +228 to +232
public void unSubscribe(ConsumerConfig config) {
if (config.isSubscribe()) {
SharedIndexInformer<Pod> informer = consumerListeners.remove(config);
if (null != informer) {
informer.stop();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The unSubscribe method does not log or handle potential exceptions that might occur during the informer's stop operation. This could lead to silent failures in unsubscribing from provider updates.

Add error handling for the informer's stop operation in the unSubscribe method to ensure clean resource management.

+ try {
+     if (null != informer) {
+         informer.stop();
+     }
+ } catch (Exception e) {
+     LOGGER.error("Error stopping informer for consumer config", e);
+ }

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.

Suggested change
public void unSubscribe(ConsumerConfig config) {
if (config.isSubscribe()) {
SharedIndexInformer<Pod> informer = consumerListeners.remove(config);
if (null != informer) {
informer.stop();
}
}
public void unSubscribe(ConsumerConfig config) {
if (config.isSubscribe()) {
SharedIndexInformer<Pod> informer = consumerListeners.remove(config);
try {
if (null != informer) {
informer.stop();
}
} catch (Exception e) {
LOGGER.error("Error stopping informer for consumer config", e);
}
}

Comment on lines +401 to +429
/**
* Gets parameter or default.
*
* @param key the key
* @return the value
*/
public String getParameter(String key, String defaultValue) {
return getParameter(key) == null ? defaultValue : getParameter(key);
}

/**
* Gets parameter or default.
*
* @param key the key
* @return the value
*/
public int getParameter(String key, int defaultValue) {
return getParameter(key) == null ? defaultValue : Integer.parseInt(parameters.get(key));
}

/**
* Gets parameter or default.
*
* @param key the key
* @return the value
*/
public boolean getParameter(String key, boolean defaultValue) {
return getParameter(key) == null ? defaultValue : Boolean.parseBoolean(parameters.get(key));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The newly added getParameter overloaded methods provide a convenient way to retrieve parameters with default values. However, there's a potential performance issue with the string-based getParameter method due to calling getParameter(key) twice.

Optimize the string-based getParameter method to avoid redundant map lookups.

- return getParameter(key) == null ? defaultValue : getParameter(key);
+ String value = getParameter(key);
+ return value == null ? defaultValue : value;

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.

Suggested change
/**
* Gets parameter or default.
*
* @param key the key
* @return the value
*/
public String getParameter(String key, String defaultValue) {
return getParameter(key) == null ? defaultValue : getParameter(key);
}
/**
* Gets parameter or default.
*
* @param key the key
* @return the value
*/
public int getParameter(String key, int defaultValue) {
return getParameter(key) == null ? defaultValue : Integer.parseInt(parameters.get(key));
}
/**
* Gets parameter or default.
*
* @param key the key
* @return the value
*/
public boolean getParameter(String key, boolean defaultValue) {
return getParameter(key) == null ? defaultValue : Boolean.parseBoolean(parameters.get(key));
}
/**
* Gets parameter or default.
*
* @param key the key
* @return the value
*/
public String getParameter(String key, String defaultValue) {
String value = getParameter(key);
return value == null ? defaultValue : value;
}
/**
* Gets parameter or default.
*
* @param key the key
* @return the value
*/
public int getParameter(String key, int defaultValue) {
return getParameter(key) == null ? defaultValue : Integer.parseInt(parameters.get(key));
}
/**
* Gets parameter or default.
*
* @param key the key
* @return the value
*/
public boolean getParameter(String key, boolean defaultValue) {
return getParameter(key) == null ? defaultValue : Boolean.parseBoolean(parameters.get(key));
}

Comment on lines +417 to +418
public int getParameter(String key, int defaultValue) {
return getParameter(key) == null ? defaultValue : Integer.parseInt(parameters.get(key));
Copy link
Contributor

Choose a reason for hiding this comment

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

In the getParameter method that returns an int, there's a potential NumberFormatException if the parameter value is not a valid integer. This could lead to runtime exceptions when invalid data is present.

Add error handling for parsing integers to prevent NumberFormatException.

+ try {
+     return getParameter(key) == null ? defaultValue : Integer.parseInt(parameters.get(key));
+ } catch (NumberFormatException e) {
+     LOGGER.warn("Invalid integer parameter value for key: " + key, e);
+     return defaultValue;
+ }

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.

Suggested change
public int getParameter(String key, int defaultValue) {
return getParameter(key) == null ? defaultValue : Integer.parseInt(parameters.get(key));
public int getParameter(String key, int defaultValue) {
try {
return getParameter(key) == null ? defaultValue : Integer.parseInt(parameters.get(key));
} catch (NumberFormatException e) {
LOGGER.warn("Invalid integer parameter value for key: " + key, e);
return defaultValue;
}

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 2da24a7 and 3feb714.
Files selected for processing (1)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistry.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistry.java

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 3feb714 and cc38025.
Files selected for processing (3)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistry.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryHelper.java (1 hunks)
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryTest.java (1 hunks)
Files skipped from review as they are similar to previous changes (3)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistry.java
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryHelper.java
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryTest.java

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between cc38025 and 1db382c.
Files selected for processing (2)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistry.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryProviderWatcher.java (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistry.java
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryProviderWatcher.java

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between ac2a73e and 49349b2.
Files ignored due to path filters (7)
  • all/pom.xml is excluded by: !**/*.xml
  • bom/pom.xml is excluded by: !**/*.xml
  • pom.xml is excluded by: !**/*.xml
  • registry/pom.xml is excluded by: !**/*.xml
  • registry/registry-kubernetes/pom.xml is excluded by: !**/*.xml
  • registry/registry-kubernetes/src/test/resources/log4j.xml is excluded by: !**/*.xml
  • registry/registry-kubernetes/src/test/resources/sofa-rpc/rpc-config.json is excluded by: !**/*.json
Files selected for processing (18)
  • core/api/src/main/java/com/alipay/sofa/rpc/common/Version.java (1 hunks)
  • core/api/src/main/java/com/alipay/sofa/rpc/config/RegistryConfig.java (1 hunks)
  • core/api/src/main/java/com/alipay/sofa/rpc/config/UserThreadPoolManager.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistry.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryHelper.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryProviderWatcher.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/constant/KubernetesClientConstants.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/utils/KubernetesClientUtils.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/utils/KubernetesConfigUtils.java (1 hunks)
  • registry/registry-kubernetes/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.registry.Registry (1 hunks)
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryTest.java (1 hunks)
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestService.java (1 hunks)
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestService2.java (1 hunks)
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestServiceImpl.java (1 hunks)
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestServiceImpl2.java (1 hunks)
  • registry/registry-kubernetes/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker (1 hunks)
  • registry/registry-nacos/src/main/java/com/alipay/sofa/rpc/registry/nacos/NacosRegistry.java (4 hunks)
  • registry/registry-nacos/src/test/java/com/alipay/sofa/rpc/registry/nacos/NacosRegistryTest.java (8 hunks)
Files skipped from review due to trivial changes (1)
  • core/api/src/main/java/com/alipay/sofa/rpc/common/Version.java
Files skipped from review as they are similar to previous changes (14)
  • core/api/src/main/java/com/alipay/sofa/rpc/config/RegistryConfig.java
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistry.java
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryHelper.java
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryProviderWatcher.java
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/constant/KubernetesClientConstants.java
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/utils/KubernetesClientUtils.java
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/utils/KubernetesConfigUtils.java
  • registry/registry-kubernetes/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.registry.Registry
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryTest.java
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestService.java
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestService2.java
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestServiceImpl.java
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestServiceImpl2.java
  • registry/registry-kubernetes/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker
Additional comments: 9
core/api/src/main/java/com/alipay/sofa/rpc/config/UserThreadPoolManager.java (1)
  • 84-84: The optimization in getUserThreadPoolSet by using addAll directly on the HashSet is a good improvement for efficiency. Well done on enhancing the performance of this method.
registry/registry-nacos/src/main/java/com/alipay/sofa/rpc/registry/nacos/NacosRegistry.java (4)
  • 28-28: The addition of the VisibleForTesting annotation import is appropriate for indicating the visibility scope of methods intended for testing purposes.
  • 282-291: Refactoring in the subscribe method to filter out disabled instances before updating providers is a robust improvement. It ensures that the system reacts only to relevant changes, enhancing the overall stability.
  • 149-151: Initializing the providerObserver in the init method if it's null is a good defensive programming practice. It ensures the component's availability and prevents potential null pointer exceptions.
  • 365-368: The addition of the getProviderObserver method, annotated with VisibleForTesting, is a clear and effective way to facilitate testing by exposing internal components. This approach enhances testability while maintaining code clarity.
registry/registry-nacos/src/test/java/com/alipay/sofa/rpc/registry/nacos/NacosRegistryTest.java (4)
  • 37-48: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [21-45]

The addition of imports for ConcurrentHashSet, Set, ExecutorService, and others supports the new test functionalities and modifications in existing test methods. These imports are appropriate and necessary for the test context.

  • 82-82: The removal of registry.init() and Assert.assertTrue(registry.start()) from the setUp() method increases flexibility in testing by allowing individual test methods to control the initialization process. This change is particularly relevant for testing specific initialization behaviors, such as in the testMuiltInit() method.
  • 84-105: The addition of the testMuiltInit() method is a valuable test case that verifies the idempotency of the init() method in NacosRegistry. It ensures that multiple initializations do not create multiple instances of NacosRegistryProviderObserver, which is crucial for the stability and correctness of the initialization logic.
  • 257-257: Adding serverConfig.destroy() calls at the end of test methods is a good practice for ensuring proper cleanup of resources. This prevents potential resource leaks and ensures that each test runs in a clean environment, reducing the risk of interference between tests.

Also applies to: 330-330

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between ac2a73e and d6704a9.
Files ignored due to path filters (6)
  • all/pom.xml is excluded by: !**/*.xml
  • bom/pom.xml is excluded by: !**/*.xml
  • registry/pom.xml is excluded by: !**/*.xml
  • registry/registry-kubernetes/pom.xml is excluded by: !**/*.xml
  • registry/registry-kubernetes/src/test/resources/log4j.xml is excluded by: !**/*.xml
  • registry/registry-kubernetes/src/test/resources/sofa-rpc/rpc-config.json is excluded by: !**/*.json
Files selected for processing (14)
  • core/api/src/main/java/com/alipay/sofa/rpc/config/RegistryConfig.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistry.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryHelper.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryProviderWatcher.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/constant/KubernetesClientConstants.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/utils/KubernetesClientUtils.java (1 hunks)
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/utils/KubernetesConfigUtils.java (1 hunks)
  • registry/registry-kubernetes/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.registry.Registry (1 hunks)
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryTest.java (1 hunks)
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestService.java (1 hunks)
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestService2.java (1 hunks)
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestServiceImpl.java (1 hunks)
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestServiceImpl2.java (1 hunks)
  • registry/registry-kubernetes/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker (1 hunks)
Files skipped from review as they are similar to previous changes (14)
  • core/api/src/main/java/com/alipay/sofa/rpc/config/RegistryConfig.java
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistry.java
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryHelper.java
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryProviderWatcher.java
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/constant/KubernetesClientConstants.java
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/utils/KubernetesClientUtils.java
  • registry/registry-kubernetes/src/main/java/com/alipay/sofa/rpc/registry/kubernetes/utils/KubernetesConfigUtils.java
  • registry/registry-kubernetes/src/main/resources/META-INF/services/sofa-rpc/com.alipay.sofa.rpc.registry.Registry
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/KubernetesRegistryTest.java
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestService.java
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestService2.java
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestServiceImpl.java
  • registry/registry-kubernetes/src/test/java/com/alipay/sofa/rpc/registry/kubernetes/TestServiceImpl2.java
  • registry/registry-kubernetes/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker

Copy link
Collaborator

@EvenLjj EvenLjj left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

LGTM

@EvenLjj EvenLjj merged commit 6cf6f00 into sofastack:master Mar 25, 2024
4 checks passed
@wangchengming666 wangchengming666 deleted the sofa-registry-kubernetes branch March 28, 2024 02:54
@EvenLjj EvenLjj added this to the 5.13.0 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants