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

feat: use notifyAll insteadOf notify for listener events notify #2043

Merged
merged 1 commit into from
Sep 10, 2022

Conversation

thehackercat
Copy link
Contributor

@thehackercat thehackercat commented Sep 6, 2022

Signed-off-by: xiaoxu [email protected]

What this PR does:
image

Notify one after one might have performance issues, this PR makes serviceListeners notify events concurrently and do not wait for notification complated.

I am wondering if it's needed to add a sync.WaitGroup after NotifyAll to wait notification completed or not.

Which issue(s) this PR fixes:
Fixes #2030

You should pay attention to items below to ensure your pr passes our ci test
We do not merge pr with ci tests failed

  • All ut passed (run 'go test ./...' in project root)
  • After go-fmt ed , run 'go fmt project' using goland.
  • Golangci-lint passed, run 'sudo golangci-lint run' in project root.
  • After import formatted, (using imports-formatter to run 'imports-formatter .' in project root, to format your import blocks, mentioned in CONTRIBUTING.md above)
  • Your new-created file needs to have apache license at the top, like other existed file does.
  • All integration test passed. You can run integration test locally (with docker env). Clone our dubbo-go-samples project and replace the go.mod to your dubbo-go, and run 'sudo sh start_integration_test.sh' at root of samples project root. (M1 Slice is not Support)

@thehackercat
Copy link
Contributor Author

@chickenlj PTAL~

@chickenlj
Copy link
Contributor

Please take a look at the build and test failures.

@thehackercat
Copy link
Contributor Author

thehackercat commented Sep 8, 2022

Please take a look at the build and test failures.

@chickenlj Hi I've passed make test , go fmt and import-formatted locally, it seems the ci failure is not introduced by this PR.

image
image

Do you have any idea how to fix this?

@justxuewei
Copy link
Member

justxuewei commented Sep 8, 2022

Do you have any idea how to fix this?

IMO, please try to checkout 3.0 branch and pull the latest commits, then rebase the 3.0 branch to thehackercat:thehackercat-patch-2.

UPDATE 1: After rebased, run go mod tidy to keep everything up-to-date.

@thehackercat thehackercat force-pushed the thehackercat-patch-2 branch 3 times, most recently from 0bb9886 to 16faa53 Compare September 8, 2022 06:44
@chickenlj
Copy link
Contributor

Probably because there are failures on the 3.0 base branch. We need to fix failures there first.

@thehackercat
Copy link
Contributor Author

golangci-lint passed after rebased this PR #2047, will check the ut failure later

@thehackercat thehackercat force-pushed the thehackercat-patch-2 branch 2 times, most recently from df3f2e6 to 137a190 Compare September 9, 2022 03:52
@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2022

Codecov Report

Merging #2043 (eea7a03) into 3.0 (9f86f36) will increase coverage by 0.08%.
The diff coverage is 20.00%.

@@            Coverage Diff             @@
##              3.0    #2043      +/-   ##
==========================================
+ Coverage   44.59%   44.68%   +0.08%     
==========================================
  Files         283      283              
  Lines       17106    17108       +2     
==========================================
+ Hits         7629     7645      +16     
+ Misses       8671     8655      -16     
- Partials      806      808       +2     
Impacted Files Coverage Δ
...y/event/service_instances_changed_listener_impl.go 0.00% <0.00%> (ø)
registry/directory/directory.go 74.18% <50.00%> (ø)
metrics/prometheus/reporter.go 31.79% <0.00%> (+1.53%) ⬆️
metadata/report/delegate/delegate_report.go 35.09% <0.00%> (+8.60%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@justxuewei justxuewei merged commit 34934d1 into apache:3.0 Sep 10, 2022
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.

6 participants