grpc-ecosystem / grpc-spring

Spring Boot starter module for gRPC framework.
https://grpc-ecosystem.github.io/grpc-spring/
Apache License 2.0
3.52k stars 825 forks source link

why in GrpcMetadataZookeeperConfiguration setPort(0) #587

Open winnerku opened 3 years ago

winnerku commented 3 years ago

when i use client and server in one project (if i just only use grpc-server-spring-boot-starter ,i did not get this problem) I find GrpcMetadataZookeeperConfiguration will set port 0 but I want remove this line , because i want use what port i set in config file zookeeperRegistration.setPort(0);

    public void init() {
        if (zookeeperRegistration != null) {
            final int port = grpcServerProperties.getPort();
            zookeeperRegistration.setPort(0);
            if (GrpcUtils.INTER_PROCESS_DISABLE != port) {
                zookeeperRegistration.getServiceInstance().getPayload().getMetadata()
                        .put(GrpcUtils.CLOUD_DISCOVERY_METADATA_PORT, Integer.toString(port));
            }
        }
    }

can anyone explain this line why?

ST-DDT commented 3 years ago

The support was added here: https://github.com/yidongnan/grpc-spring-boot-starter/pull/475#discussion_r554219476

Is it a bug/should we change it? If yes, can you help us with testing the fix?

winnerku commented 3 years ago

The support was added here: #475 (comment)

Is it a bug/should we change it? If yes, can you help us with testing the fix?

OK, I don't know if this is a bug,but I guess the loading order of the properties file affects the settings in this place

In the process of debugging, I found that the default value of serverbuilder depends on the port setting of this place. By default, after I load grpc.server.port, I will get the port value. The port value of Web / rest / RPC will be loaded in order in NameResovle. However, once grpc-client-spring-boot-starter is relied on, If it is set to 0 here, the port value of the whole web / rest / RPC will be loaded by default by the value of server.port. In fact, the value of grpc.server.port should be set here

The corresponding is: in zookeeper discovery client

{
    "name": "fundx-monitor-server",
    "id": "24ea9188-f091-44f6-b812-c2948880c292",
    "address": "172.20.10.2",
    "port": 6568, # this place should be grpc.server.port
    ...
}

may be I don't know how to config this when grpc-client-spring-boot-starter,grpc-server-spring-boot-starter in one project

winnerku commented 3 years ago

The support was added here: #475 (comment) Is it a bug/should we change it? If yes, can you help us with testing the fix?

OK, I don't know if this is a bug,but I guess the loading order of the properties file affects the settings in this place

In the process of debugging, I found that the default value of serverbuilder depends on the port setting of this place. By default, after I load grpc.server.port, I will get the port value. The port value of Web / rest / RPC will be loaded in order in NameResovle. However, once grpc-client-spring-boot-starter is relied on, If it is set to 0 here, the port value of the whole web / rest / RPC will be loaded by default by the value of server.port. In fact, the value of grpc.server.port should be set here

The corresponding is: in zookeeper discovery client

{
    "name": "fundx-monitor-server",
    "id": "24ea9188-f091-44f6-b812-c2948880c292",
    "address": "172.20.10.2",
    "port": 6568, # this place should be grpc.server.port
    ...
}

may be I don't know how to config this when grpc-client-spring-boot-starter,grpc-server-spring-boot-starter in one project

i know what‘s reason , zookeeperRegistration.setPort(0); this place will cause protected ServiceInstanceBuilder<ZookeeperInstance> builder; in the ServiceInstanceRegistration that the builder will build the class serverinstance always set port is 0 . so this place in GrpcMetadataZookeeperConfiguration zookeeperRegistration.setPort(0); is it right?

ST-DDT commented 3 years ago

I will try to test it with and without that line. Maybe there are ways to improve it. It might take a while though.