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

standard JSON dont compat with go client #4471

Closed
yuz10 opened this issue Jun 16, 2022 · 7 comments · Fixed by #4473
Closed

standard JSON dont compat with go client #4471

yuz10 opened this issue Jun 16, 2022 · 7 comments · Fixed by #4473
Milestone

Comments

@yuz10
Copy link
Member

yuz10 commented Jun 16, 2022

apache/rocketmq-client-go#845

BUG REPORT

in #4432 , the response of GET_ROUTEINFO_BY_TOPIC is changed, for example, the new format is:
{"brokerDatas":[{"brokerAddrs":{"0":"127.0.0.1:10911"}]}
the old is:
{"brokerDatas":[{"brokerAddrs":{0:"127.0.0.1:10911"}]}

the following go code in https://github.com/apache/rocketmq-client-go/blob/master/internal/route.go#L561

func (routeData *TopicRouteData) decode(data string) error {
	res := gjson.Parse(data)
	err := jsoniter.Unmarshal([]byte(res.Get("queueDatas").String()), &routeData.QueueDataList)

	if err != nil {
		return err
	}

	bds := res.Get("brokerDatas").Array()
	routeData.BrokerDataList = make([]*BrokerData, len(bds))
	for idx, v := range bds {
		bd := &BrokerData{
			BrokerName:      v.Get("brokerName").String(),
			Cluster:         v.Get("cluster").String(),
			BrokerAddresses: make(map[int64]string, 0),
		}
		addrs := v.Get("brokerAddrs").String()
		strs := strings.Split(addrs[1:len(addrs)-1], ",")
		if strs != nil {
			for _, str := range strs {
				i := strings.Index(str, ":")
				if i < 0 {
					continue
				}
				id, _ := strconv.ParseInt(str[0:i], 10, 64)
				bd.BrokerAddresses[id] = strings.Replace(str[i+1:], "\"", "", -1)
			}
		}
		routeData.BrokerDataList[idx] = bd
	}
	return nil
}

in id, _ := strconv.ParseInt(str[0:i], 10, 64) , if broker id is 1, then str[0:i] is "\"1\"", and strconv.ParseInt("\"1\"") will always be 0
so the producer will send message to slave.

I have submit a pull request in rocketmq-client-go, apache/rocketmq-client-go#846, but I think we should also be compatable with the old versions of go client.

  1. Please tell us about your environment:

  2. Other information (e.g. detailed explanation, logs, related issues, suggestions on how to fix, etc):

@yuz10 yuz10 changed the title Go client fail to connect to current version of develop branch standard JSON dont compat with go client Jun 16, 2022
@duhenglucky
Copy link
Contributor

duhenglucky commented Jun 17, 2022

@yuz10 good catch, would you like to submit a PR to temporarily change back to the previous way to ensure compatibility with old clients? Then start a discussion to provide standard JSON under the premise of ensuring compatibility, such as adding new interfaces or judging according to the protocol version

@yuz10
Copy link
Member Author

yuz10 commented Jun 17, 2022

@yuz10 good catch, would you like to submit a PR to temporarily change back to the previous way to ensure compatibility with old clients? Then start a discussion to provide standard JSON under the premise of ensuring compatibility, such as adding new interfaces or judging according to the protocol version

sure

@lizhanhui
Copy link
Contributor

@yuz10 Should create an issue to the go-sdk, making it compatible with standard JSON. For the ops, upgrade client first then the server. Let name server generate invalid JSON would cause C++ client coredump from time to time.

@lizhanhui
Copy link
Contributor

The go code posted here is manually parsing strings bit by bit, which itself is error-prone. This is not the supposed way of deserializating JSON

@yuz10
Copy link
Member Author

yuz10 commented Jun 17, 2022

@yuz10 Should create an issue to the go-sdk, making it compatible with standard JSON. For the ops, upgrade client first then the server. Let name server generate invalid JSON would cause C++ client coredump from time to time.

I have submit an pull request in go sdk, but there is a compatability problem

@lizhanhui
Copy link
Contributor

@yuz10 Let's make things work this way
For clients prior to V4_9_3, generate JSON as before unless its request header explicitly accepts standard JSON only;
For clients released in the future, aka, version >= V4_9_3, respond with standard JSON

@duhenglucky duhenglucky added this to the 4.9.4 milestone Jun 17, 2022
@dongeforever
Copy link
Member

@yuz10 It is suggested to use the explicit header field "acceptStandardJsonOnly" instead of relying on the version code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants