kitex-contrib / registry-etcd

Apache License 2.0
21 stars 25 forks source link

fix resolve collision bug when serviceNames have the same prefix #12

Closed succulentxb closed 1 year ago

succulentxb commented 1 year ago

kitex-contrib/registry-etcd in current version facing at the bug that services will be resolved chaotically when services have names with the same prefix. For example, Service A named "demo-service" Service B named "demo-service-B" the resolver will return both Service A and Service B registry info when resolve for "demo-service".

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

bytedance-oss-robot[bot] commented 1 year ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: succulentxb

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/kitex-contrib/registry-etcd/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
lizhemingi commented 1 year ago

Nice catch.

But it seems still wrongly for demo-service and demo-service/b. How about adding a check to ensure character / not in service name when register ?

GuangmingLuo commented 1 year ago

@succulentxb hi, will you continue on this PR ?

succulentxb commented 1 year ago

@succulentxb hi, will you continue on this PR ?

Sorry for omission, I've push a new commit for checking char '/' in service name.

GuangmingLuo commented 1 year ago

@succulentxb please also fix this

image
succulentxb commented 1 year ago

@succulentxb please also fix this image

done

lizhemingi commented 1 year ago

LGTM

lizhemingi commented 1 year ago

@succulentxb Could you also help to fix the CI failure? Just add a check for the return value of pem.Encode in etcd_resolver_test.go

succulentxb commented 1 year ago

@lizhemingi Is it okay that I assign return error to '_' in order to fix ci lint?

lizhemingi commented 1 year ago

/LGTM

lizhemingi commented 1 year ago

@succulentxb Thanks.