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

[ISSUE #679] Fix judgment of topic route equality and optimize loadBalancer #680

Merged
merged 3 commits into from
Feb 26, 2024

Conversation

redlsz
Copy link
Contributor

@redlsz redlsz commented Feb 2, 2024

Which Issue(s) This PR Fixes

Fixes #679

Brief Description

  1. correct judgment of topic route equality
  2. do not reset index when update loadBalancer

@lizhanhui lizhanhui self-requested a review February 2, 2024 07:17
golang/client.go Outdated Show resolved Hide resolved
@leizhiyuan
Copy link
Contributor

add some ref

protocmp

it seems only 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 function to compare message values.

In tests, you can also use the "github.com/google/go-cmp/cmp" package with the protocmp.Transform() option. The cmp package can compare arbitrary data structures, and 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)
}

Copy link
Contributor

@lizhanhui lizhanhui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@redlsz
Copy link
Contributor Author

redlsz commented Feb 21, 2024

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.

@redlsz redlsz requested a review from lizhanhui February 21, 2024 04:00
Copy link
Contributor

@lizhanhui lizhanhui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work

@lizhanhui lizhanhui merged commit 80d9c07 into apache:master Feb 26, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Go client can't receive certain broker's messages
3 participants