Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(#5608 && #10223): When the custom instance id is empty, the id will be automatically generated. #10812

Merged
merged 1 commit into from
Aug 1, 2023

Conversation

Bo-Qiu
Copy link
Contributor

@Bo-Qiu Bo-Qiu commented Jul 17, 2023

What is the purpose of the change

feat(#5608 && #10223): When the custom instance id is empty, the id will be automatically generated.

@Bo-Qiu
Copy link
Contributor Author

Bo-Qiu commented Jul 17, 2023

Based on our discussions, I re-wrote the code and submitted a new PR, please review it again, thx!
@KomachiSion

@codecov-commenter
Copy link

codecov-commenter commented Jul 17, 2023

Codecov Report

Merging #10812 (63f1bd7) into develop (f986916) will decrease coverage by 0.12%.
Report is 13 commits behind head on develop.
The diff coverage is 83.33%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             develop   #10812      +/-   ##
=============================================
- Coverage      53.44%   53.32%   -0.12%     
+ Complexity      5626     5625       -1     
=============================================
  Files            923      927       +4     
  Lines          29458    29567     +109     
  Branches        3244     3250       +6     
=============================================
+ Hits           15743    15768      +25     
- Misses         12339    12413      +74     
- Partials        1376     1386      +10     
Files Changed Coverage Δ
...a/com/alibaba/nacos/naming/utils/InstanceUtil.java 73.46% <80.00%> (+1.67%) ⬆️
...emote/rpc/handler/BatchInstanceRequestHandler.java 100.00% <100.00%> (ø)
...ing/remote/rpc/handler/InstanceRequestHandler.java 100.00% <100.00%> (ø)

... and 35 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f986916...63f1bd7. Read the comment docs.

Copy link
Collaborator

@KomachiSion KomachiSion left a comment

Choose a reason for hiding this comment

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

Please use DefaultInstanceIdGenerator to generate default Id like http register

@KomachiSion
Copy link
Collaborator

com.alibaba.nacos.naming.pojo.instance.HttpRequestInstanceBuilder#setInstanceId

@Bo-Qiu
Copy link
Contributor Author

Bo-Qiu commented Jul 19, 2023

Please use DefaultInstanceIdGenerator to generate default Id like http register

In fact, my implementation is consistent with it. could you provide more information? thanks a lot.

image

@Bo-Qiu
Copy link
Contributor Author

Bo-Qiu commented Jul 20, 2023

Please use DefaultInstanceIdGenerator to generate default Id like http register

Is your suggestion to directly set the ID using it regardless of whether the customID is empty or not? Code as follow:

private void setInstanceId(Instance instance) {
        DefaultInstanceIdGenerator idGenerator = new DefaultInstanceIdGenerator(instance.getServiceName(),
                instance.getClusterName(), instance.getIp(), instance.getPort());
        instance.setInstanceId(idGenerator.generateInstanceId());
    }

I think there are some problems with using it directly. In GRPC mode, the filed serviceName of instance could be empty when registering an instance. During the process of registering an instance via Grpc, we are directly using the value of the service's name field.

By the way, whether it's necessary to maintain the logic of custom instance IDs is also a topic worth discussing.

Copy link
Collaborator

@KomachiSion KomachiSion left a comment

Choose a reason for hiding this comment

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

You can see com.alibaba.nacos.naming.pojo.instance.HttpRequestInstanceBuilder#setAttributesToBuilder

the instance service name in from the request.
so grpc also should use service name in request.

service name in request should not be null acutally.

@Bo-Qiu
Copy link
Contributor Author

Bo-Qiu commented Jul 25, 2023

Thanks for your information, I've changed it.

@Bo-Qiu Bo-Qiu closed this Jul 25, 2023
@Bo-Qiu Bo-Qiu reopened this Jul 25, 2023
@Bo-Qiu Bo-Qiu closed this Jul 25, 2023
@Bo-Qiu Bo-Qiu reopened this Jul 25, 2023
@@ -51,6 +52,7 @@ public InstanceRequestHandler(EphemeralClientOperationServiceImpl clientOperatio
public InstanceResponse handle(InstanceRequest request, RequestMeta meta) throws NacosException {
Service service = Service
.newService(request.getNamespace(), request.getGroupName(), request.getServiceName(), true);
InstanceUtil.setInstanceIdIfEmpty(request.getInstance(), request.getServiceName());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this will lose the group name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your reminder, I have already fixed this issue, please review it again.

…mpty, the id will be automatically generated.
@Bo-Qiu Bo-Qiu closed this Jul 28, 2023
@Bo-Qiu Bo-Qiu reopened this Jul 28, 2023
@Bo-Qiu Bo-Qiu closed this Jul 28, 2023
@Bo-Qiu Bo-Qiu reopened this Jul 28, 2023
@KomachiSion KomachiSion merged commit 8370124 into alibaba:develop Aug 1, 2023
14 of 19 checks passed
@KomachiSion KomachiSion added the kind/feature type/feature label Aug 1, 2023
@KomachiSion KomachiSion added this to the 2.3.0 milestone Aug 1, 2023
realJackSun added a commit that referenced this pull request Aug 10, 2023
…lop branch (#10942)

* [ISSUE #10734] Implement http request param check filter and http param extractors (#10758)

* For #10734,Implement grpc server interceptor and grpc param extractors

* For #10734,add unit test for grpc server interceptor and grpc param extractors

* For #10734,alter the test case

* For #10734,delete the ConnectionSetupRequestParamExtractor

* For #10734,add the naming http request param check filter and implement the naming http request param extractors

* For #10734,add unit test for naming http request param extractors

* For #10734,Implement grpc server interceptor and grpc param extractors

* For #10734,add unit test for grpc server interceptor and grpc param extractors

* For #10734,delete the ConnectionSetupRequestParamExtractor

* For #10734,add the naming http request param check filter and implement the naming http request param extractors

* For #10734,add unit test for naming http request param extractors

* For #10734,add the config http request param check filter and implement the config http request param extractors and unit test

* For #10734,add the console http request param check filter and implement the console http request param extractors and unit test

* For #10734,fix code style

* For #10734,alter the logic of exception handle in filter

* For #10734,fix code style

* dump change check task submit (#10755)

* dump change check task submit

* delete config nid convert error fix

* fix test case

* checkstyle

* Refactor grpc tls (#10759)

* Move Tls negotiator to GrpcSdkServer.

* use protocol negotiator builder replace directly create.

* use SPI load negotiator and set tls as default negotiator.

* Remove tlsconfig in BaseRpcServer.

* Add some ut.

* For checkstyle.

* fix word spelling in `AuthenticationManagerDelegator` (#10777)

* fix(#10427): When the execution of handleServerRequest() encounters an exception, record the log and throw an exception, then quickly response to the server errResponse (#10770)

* fix(#10585): selectInstances and selectOneHealthyInstance methods, if the parameter subscribe is true. Subscription is required when clientProxy.isSubscribe() is false. (#10805)

* disable check port input when use registered port (#10799)

* Add the handle to overload connection (#10783)

* add the handle to overload connection

* fast return

* [ISSUE #10662]Prometheus-sd add namespace and service api (#10663)

* add apis

* add tests

* fix checkstyle

* param namespace to namespaceId

* namespace to namespaceId

* fix test case

* fix test case

* Service instance should display related color when healthy or unhealthy (#10811)

* fix a couple of invaild props (#10810)

* feat(#10539): When the operation configuration fails, log. (#10804)

* [ISSUE #10744]feat:Add HealthControllerV2 and HealthControllerV2Test (#10786)

* [ISSUE #10744]feat:Add HealthControllerV2 and HealthControllerV2Test

* fix:V2 api return Result

* [ISSUE #10734] Refactor the ParamChecker and ParamExtractor (#10775)

* For #10734,refactor the paramextractor and ParamChecker

* For #10734,alter the rules of ParamCheck

* For #10734,alter the rules of ParamCheck

* For #10734,fix bug

* For #10734,fix bug and alter the ParamCheckRules.java

* For #10734,fix code style

* For #10734,fix the param check rules

* For #10734,implement the server param check config

* For #10734,optimize the logic

* For #10734,optimize the logic

* For #10734,optimize the logic

* Refactor Prometheus Module (#10827)

* Refactor Prometheus Module

* Complete Test Case

* format

* format

* For #10734,fix the param check rule (#10826)

* [ISSUE #10792]When nacos client use endpoint, the registration center should support configuring context-path and cluster-name like the configuration center (#10793)

* Reactor code in datasource-plugin (#10791)

* Reactor code in datasource-plugin

* Fix Abstract Mapper Test Case

* Add Empty Check

* Fix Checkstyle

* fix checkstyle

* fix check style

* fix check style

* Fix CheckStyle

* Fix SQL Blank

* bugfix for PersistentClientOperationServiceImpl log (#10825)

* For #10734,fix the param check rule (#10858)

* fix(#10831): When using the deregisterInstance method to remove one of multiple instances registered by batchRegisterInstance, all instances registered by batchRegisterInstance will be removed. (#10836)

* UnsupportedFeatureError (#10860)

* fix(distro): fix issue#10880. (#10881)

* feat(#5608 && #10223): When the custom instance id is empty, the id will be automatically generated. (#10812)

* fix: test-code branch (#10904)

* add nacos ci

* delete client version of nacos ci

* fix: test-code branch

* console-ui 新增 toml 语言支持,修复之前 properties 主题未生效的问题 (#10896)

* console-ui 新增 toml 语言支持,修复之前 properties 主题未生效的问题

* console-ui 新增 toml 语言支持,修复之前 properties 主题未生效的问题

* 老规矩,要编译一波

* feat(10891): Provide a configuration item for the maximum number of push retries, instead of directly hardcoding it to 50 times in the code. (#10895)

* [ISSUE #10824] Remove udp port param for v1-client (#10914)

* Remove UDP Param

* Fix gRPC client

* fix test case

* fix test case

* fix test case

* fix test case

* Fix login failed when close auth.

---------

Co-authored-by: Sunrisea <49605583+Sunrisea@users.noreply.github.com>
Co-authored-by: nov.lzf <liuzunfei@gmail.com>
Co-authored-by: 杨翊 SionYang <xiweng.yy@alibaba-inc.com>
Co-authored-by: ZhangShenao <15201440436@163.com>
Co-authored-by: blake.qiu <46370663+Bo-Qiu@users.noreply.github.com>
Co-authored-by: Joey777210 <53630996+Joey777210@users.noreply.github.com>
Co-authored-by: chenyiqin <83362909+Daydreamer-ia@users.noreply.github.com>
Co-authored-by: maoling <11016631+maoling@users.noreply.github.com>
Co-authored-by: DiligenceLai <96920907+DiligenceLai@users.noreply.github.com>
Co-authored-by: huhongjie2014 <139250842+huhongjie2014@users.noreply.github.com>
Co-authored-by: lu-xiaoshuang <121755080+lu-xiaoshuang@users.noreply.github.com>
Co-authored-by: zt9788 <zt9788@users.noreply.github.com>
Co-authored-by: 阿魁 <670569467@qq.com>
Co-authored-by: wuyfee <30968107+Wuyunfan-BUPT@users.noreply.github.com>
Co-authored-by: Darren Luo <jn296@qq.com>
Co-authored-by: xxc <xchaos8@126.com>
@Bo-Qiu Bo-Qiu deleted the feat/instanceId branch November 24, 2023 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature type/feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants