[ISSUE#5863]add nacos client batchRegitserInstance request #8675

chenhao26-nineteen · 2022-07-03T10:23:01Z

Please do not create a Pull Request without creating an issue first.

What is the purpose of the change

for #5863 nacos client support batchRegister request

Brief changelog

XX

Verifying this change

XXXX

Follow this checklist to help us incorporate your contribution quickly and easily:

Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
Run mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=true to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.

markguy · 2022-07-03T10:23:28Z

received

codecov-commenter · 2022-07-03T10:34:02Z

Codecov Report

Merging #8675 (1288888) into develop (6fe84f4) will increase coverage by 0.00%.
The diff coverage is 42.55%.

@@            Coverage Diff             @@
##             develop    #8675   +/-   ##
==========================================
  Coverage      41.59%   41.59%           
- Complexity      4480     4489    +9     
==========================================
  Files            884      885    +1     
  Lines          32245    32291   +46     
  Branches        3733     3738    +5     
==========================================
+ Hits           13411    13433   +22     
- Misses         17479    17503   +24     
  Partials        1355     1355

Impacted Files	Coverage Δ
...om/alibaba/nacos/api/naming/utils/NamingUtils.java	`20.00% <0.00%> (-5.72%)`	⬇️
...libaba/nacos/client/naming/NacosNamingService.java	`86.09% <0.00%> (-1.75%)`	⬇️
...lient/naming/remote/NamingClientProxyDelegate.java	`84.12% <0.00%> (-10.52%)`	⬇️
...ent/naming/remote/gprc/redo/RedoScheduledTask.java	`73.13% <0.00%> (-4.65%)`	⬇️
...naming/remote/gprc/redo/data/InstanceRedoData.java	`100.00% <ø> (ø)`
...ient/naming/remote/http/NamingHttpClientProxy.java	`80.00% <0.00%> (-0.37%)`	⬇️
...vice/impl/EphemeralClientOperationServiceImpl.java	`68.42% <0.00%> (ø)`
...g/remote/gprc/redo/data/BatchInstanceRedoData.java	`87.50% <87.50%> (ø)`
...ient/naming/remote/gprc/NamingGrpcClientProxy.java	`82.35% <100.00%> (+1.30%)`	⬆️
...naming/remote/gprc/redo/NamingGrpcRedoService.java	`94.00% <100.00%> (+0.38%)`	⬆️
... and 5 more

Continue to review full report at Codecov.

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

KomachiSion · 2022-07-04T05:49:34Z

api/src/main/java/com/alibaba/nacos/api/naming/utils/NamingUtils.java

+        for (int i = 0; i < newInstanceList.size(); i++) {
+            Instance instance = newInstanceList.get(i);
+            checkInstanceIsLegal(instance);
+            for (int j = i + 1; j < newInstanceList.size(); j++) {


Why not use set?

Why not use set?

get

KomachiSion · 2022-07-04T05:54:48Z

api/src/main/java/com/alibaba/nacos/api/naming/utils/NamingUtils.java

+        CopyOnWriteArrayList<Instance> newInstanceList = new CopyOnWriteArrayList<>(instances);
+        for (int i = 0; i < newInstanceList.size(); i++) {
+            Instance instance = newInstanceList.get(i);
+            checkInstanceIsLegal(instance);


should we check all instance is same ephemeral?

should we check all instance is same ephemeral?

Yes, I adjusted it

KomachiSion · 2022-07-04T05:56:55Z

client/src/main/java/com/alibaba/nacos/client/naming/remote/NamingClientProxyDelegate.java

+            NAMING_LOGGER.warn("batchRegisterInstance instances is Empty:{}", instances);
+            grpcClientProxy.batchRegisterService(serviceName, groupName, instances);
+        }
+        getExecuteClientProxy(instances.get(0)).batchRegisterService(serviceName, groupName, instances);


ArrayIndexOutOf if instances is empty.

I can't get the logic for double call batchRegisterService for different proxy.
The batch feature only add for grpc I think.

I can't get the logic for double call batchRegisterService for different proxy. The batch feature only add for grpc I think.

In order to adapt to the Nacos call style, I have modified it to grpc call

KomachiSion · 2022-07-04T06:00:04Z

client/src/main/java/com/alibaba/nacos/client/naming/remote/gprc/redo/RedoScheduledTask.java

@@ -75,6 +76,12 @@ private void redoForInstance(InstanceRedoData redoData) throws NacosException {
                if (isClientDisabled()) {
                    return;
                }
+                if (redoData instanceof BatchInstanceRedoData) {


abstract line from 79 to 85 into other method.

abstract line from 79 to 85 into other method.

i abstract it

KomachiSion · 2022-07-05T09:49:00Z

api/src/main/java/com/alibaba/nacos/api/naming/utils/NamingUtils.java

+    public static void checkInstanceIsEphemeral(Instance instance) throws NacosException {
+        if (!instance.isEphemeral()) {
+            throw new NacosException(NacosException.INVALID_PARAM,
+                    String.format("Batch registration does not allow non-temporary instance registration , Instance：%s", instance));


non-temporary -> non-ephemeral or persistent

persistent

rigorous

KomachiSion · 2022-07-05T09:50:01Z

client/src/main/java/com/alibaba/nacos/client/naming/remote/http/NamingHttpClientProxy.java

@@ -487,7 +492,7 @@ public String callServer(String api, Map<String, String> params, Map<String, Str
            HttpRestResult<String> restResult = nacosRestTemplate
                    .exchangeForm(url, header, Query.newInstance().initParams(params), body, method, String.class);
            end = System.currentTimeMillis();
-            
+


Why remove indent?

Why remove indent?

I have revised it

add nacos client batchRegitserInstance request

1288888

KomachiSion requested changes Jul 4, 2022

View reviewed changes

adjustment code

8f99f87

KomachiSion requested changes Jul 5, 2022

View reviewed changes

fix word and do not remove indent

2c32509

KomachiSion approved these changes Jul 7, 2022

View reviewed changes

KomachiSion added the kind/feature type/feature label Jul 7, 2022

KomachiSion added this to the 2.1.1 milestone Jul 7, 2022

KomachiSion merged commit 073f9e0 into alibaba:develop Jul 7, 2022

mahaitao617 mentioned this pull request Oct 18, 2022

[BUG] <nacos register exist an phenomenon that last registered instance will override previous instance> apache/shenyu#4104

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE#5863]add nacos client batchRegitserInstance request #8675

[ISSUE#5863]add nacos client batchRegitserInstance request #8675

chenhao26-nineteen commented Jul 3, 2022

markguy commented Jul 3, 2022 via email

codecov-commenter commented Jul 3, 2022

KomachiSion Jul 4, 2022

chenhao26-nineteen Jul 4, 2022

KomachiSion Jul 4, 2022

chenhao26-nineteen Jul 4, 2022

KomachiSion Jul 4, 2022

KomachiSion Jul 4, 2022

chenhao26-nineteen Jul 4, 2022

KomachiSion Jul 4, 2022

chenhao26-nineteen Jul 4, 2022

KomachiSion Jul 5, 2022

chenhao26-nineteen Jul 5, 2022

KomachiSion Jul 5, 2022

chenhao26-nineteen Jul 6, 2022

[ISSUE#5863]add nacos client batchRegitserInstance request #8675

[ISSUE#5863]add nacos client batchRegitserInstance request #8675

Conversation

chenhao26-nineteen commented Jul 3, 2022

What is the purpose of the change

Brief changelog

Verifying this change

markguy commented Jul 3, 2022 via email

codecov-commenter commented Jul 3, 2022

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment