You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "redlsz (via GitHub)" <gi...@apache.org> on 2024/02/02 06:39:50 UTC
[PR] [ISSUE #679] Fix judgment of topic route equality and optimize loadBalancer [rocketmq-clients]
redlsz opened a new pull request, #680:
URL: https://github.com/apache/rocketmq-clients/pull/680
<!-- Please make sure the target branch is right. In most case, the target branch should be `master`. -->
### Which Issue(s) This PR Fixes
<!-- Please ensure that the related issue has already been created, and [link this pull request to that issue using keywords](<https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword>) to ensure automatic closure. -->
Fixes #679
### Brief Description
<!-- Write a brief description for your pull request to help the maintainer understand the reasons behind your changes. -->
1. correct judgment of topic route equality
2. do not reset index when update loadBalancer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #679] Fix judgment of topic route equality and optimize loadBalancer [rocketmq-clients]
Posted by "lizhanhui (via GitHub)" <gi...@apache.org>.
lizhanhui merged PR #680:
URL: https://github.com/apache/rocketmq-clients/pull/680
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #679] Fix judgment of topic route equality and optimize loadBalancer [rocketmq-clients]
Posted by "lizhanhui (via GitHub)" <gi...@apache.org>.
lizhanhui commented on code in PR #680:
URL: https://github.com/apache/rocketmq-clients/pull/680#discussion_r1475667349
##########
golang/client.go:
##########
@@ -513,18 +513,39 @@ func (cli *defaultClient) startUp() error {
cli.log.Info("newRoute is nil, but oldRoute is not. do not update")
return true
}
- if !reflect.DeepEqual(newRoute, oldRoute) {
+ routeEqual := func(old, new []*v2.MessageQueue) bool {
Review Comment:
In C++ implementation, I used [MessageDifferencer](https://protobuf.dev/reference/cpp/api-docs/google.protobuf.util.message_differencer/) to achieve this; Golang has [protocmp](https://pkg.go.dev/google.golang.org/protobuf/testing/protocmp). Can we use protocmp to compare generically instead of manual work, which is error-prone over time?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #679] Fix judgment of topic route equality and optimize loadBalancer [rocketmq-clients]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #680:
URL: https://github.com/apache/rocketmq-clients/pull/680#issuecomment-3050321747
## [Codecov](https://app.codecov.io/gh/apache/rocketmq-clients/pull/680?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 47.14%. Comparing base [(`0f38c07`)](https://app.codecov.io/gh/apache/rocketmq-clients/commit/0f38c07a704225e0e32a3895b17d38f01e2b01ee?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`83300ff`)](https://app.codecov.io/gh/apache/rocketmq-clients/commit/83300ff67266c1988899340be51452dd80162bbc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 87 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (0f38c07) and HEAD (83300ff). Click for more details.
>
> <details><summary>HEAD has 4 uploads less than BASE</summary>
>
>| Flag | BASE (0f38c07) | HEAD (83300ff) |
>|------|------|------|
>|nodejs|4|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #680 +/- ##
============================================
- Coverage 55.89% 47.14% -8.76%
Complexity 668 668
============================================
Files 259 207 -52
Lines 15653 12182 -3471
Branches 3891 3539 -352
============================================
- Hits 8750 5743 -3007
+ Misses 6650 6186 -464
Partials 253 253
```
| [Flag](https://app.codecov.io/gh/apache/rocketmq-clients/pull/680/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [golang](https://app.codecov.io/gh/apache/rocketmq-clients/pull/680/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `∅ <ø> (∅)` | |
| [nodejs](https://app.codecov.io/gh/apache/rocketmq-clients/pull/680/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/rocketmq-clients/pull/680?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #679] Fix judgment of topic route equality and optimize loadBalancer [rocketmq-clients]
Posted by "leizhiyuan (via GitHub)" <gi...@apache.org>.
leizhiyuan commented on PR #680:
URL: https://github.com/apache/rocketmq-clients/pull/680#issuecomment-1925295801
add some ref
[protocmp](https://protobuf.dev/reference/go/faq/#deepequal)
it seems only [proto.Equal](https://pkg.go.dev/google.golang.org/protobuf/proto#Equal) can be used in production code
where protocmp can be used in test (for best practice)
> Why does reflect.DeepEqual behave unexpectedly with protobuf messages?
> Generated protocol buffer message types include internal state which can vary even between equivalent messages.
>
> In addition, the reflect.DeepEqual function is not aware of the semantics of protocol buffer messages, and can report differences where none exist. For example, a map field containing a nil map and one containing a zero-length, non-nil map are semantically equivalent, but will be reported as unequal by reflect.DeepEqual.
>
> Use the [proto.Equal](https://pkg.go.dev/google.golang.org/protobuf/proto#Equal) function to compare message values.
>
> In tests, you can also use the ["github.com/google/go-cmp/cmp"](https://pkg.go.dev/github.com/google/go-cmp/cmp?tab=doc) package with the [protocmp.Transform()](https://pkg.go.dev/google.golang.org/protobuf/testing/protocmp#Transform) option. The cmp package can compare arbitrary data structures, and [cmp.Diff](https://pkg.go.dev/github.com/google/go-cmp/cmp#Diff) produces human-readable reports of the differences between values.
>
> if diff := cmp.Diff(a, b, protocmp.Transform()); diff != "" {
> t.Errorf("unexpected difference:\n%v", diff)
> }
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #679] Fix judgment of topic route equality and optimize loadBalancer [rocketmq-clients]
Posted by "redlsz (via GitHub)" <gi...@apache.org>.
redlsz commented on PR #680:
URL: https://github.com/apache/rocketmq-clients/pull/680#issuecomment-1955792483
> A few follow-up issues to resolve: 1, Test cases are required to verify this change, which also ensures future changes will not break; 2, CI is broken
1. done
2. It seems to be not related to this issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #679] Fix judgment of topic route equality and optimize loadBalancer [rocketmq-clients]
Posted by "lizhanhui (via GitHub)" <gi...@apache.org>.
lizhanhui commented on code in PR #680:
URL: https://github.com/apache/rocketmq-clients/pull/680#discussion_r1477164004
##########
golang/client.go:
##########
@@ -513,18 +513,39 @@ func (cli *defaultClient) startUp() error {
cli.log.Info("newRoute is nil, but oldRoute is not. do not update")
return true
}
- if !reflect.DeepEqual(newRoute, oldRoute) {
+ routeEqual := func(old, new []*v2.MessageQueue) bool {
Review Comment:
All right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #679] Fix judgment of topic route equality and optimize loadBalancer [rocketmq-clients]
Posted by "redlsz (via GitHub)" <gi...@apache.org>.
redlsz commented on code in PR #680:
URL: https://github.com/apache/rocketmq-clients/pull/680#discussion_r1475787902
##########
golang/client.go:
##########
@@ -513,18 +513,39 @@ func (cli *defaultClient) startUp() error {
cli.log.Info("newRoute is nil, but oldRoute is not. do not update")
return true
}
- if !reflect.DeepEqual(newRoute, oldRoute) {
+ routeEqual := func(old, new []*v2.MessageQueue) bool {
Review Comment:
> In C++ implementation, I used [MessageDifferencer](https://protobuf.dev/reference/cpp/api-docs/google.protobuf.util.message_differencer/) to achieve this; Golang has [protocmp](https://pkg.go.dev/google.golang.org/protobuf/testing/protocmp). Can we use protocmp to compare generically instead of manual work, which is error-prone over time?
The [go-cmp](https://pkg.go.dev/github.com/google/go-cmp/cmp) and [protocmp](https://pkg.go.dev/google.golang.org/protobuf/testing/protocmp) seem being for testing. And manual comparison here is actually based on [proto.Equal](https://github.com/protocolbuffers/protobuf-go/blob/v1.32.0/proto/equal.go#L41)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org