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

Nacos support subscribe to all with '*' #2374

Merged
merged 5 commits into from
Aug 28, 2023
Merged

Nacos support subscribe to all with '*' #2374

merged 5 commits into from
Aug 28, 2023

Conversation

FinalT
Copy link
Member

@FinalT FinalT commented Aug 8, 2023

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Aug 8, 2023

Codecov Report

Merging #2374 (28acaeb) into main (6888010) will decrease coverage by 0.12%.
The diff coverage is 0.00%.

❗ Current head 28acaeb differs from pull request most recent head d095f97. Consider uploading reports for the commit d095f97 to get more accurate results

@@            Coverage Diff             @@
##             main    #2374      +/-   ##
==========================================
- Coverage   44.22%   44.11%   -0.12%     
==========================================
  Files         305      304       -1     
  Lines       18628    18594      -34     
==========================================
- Hits         8239     8202      -37     
- Misses       9526     9531       +5     
+ Partials      863      861       -2     
Files Changed Coverage Δ
registry/nacos/listener.go 0.00% <0.00%> (ø)
registry/nacos/registry.go 33.49% <0.00%> (-9.03%) ⬇️

... and 5 files with indirect coverage changes

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

@sonarcloud
Copy link

sonarcloud bot commented Aug 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
2.7% 2.7% Duplication

logger.Warnf("event listener game over.")
return perrors.New("nacosRegistry is not available.")
}
services, err := nr.getAllSubscribeServiceNames()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a scheduled pulling task here? What if the service name list in Nacos server changes after we subscribed to '', for example, if a new service name is registered, will we still get notified by subscribing to ''?

Copy link
Contributor

Choose a reason for hiding this comment

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

This can be added later.

Copy link
Member

@binbin0325 binbin0325 left a comment

Choose a reason for hiding this comment

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

In what scenarios should subscribe to all services

Copy link
Contributor

@chickenlj chickenlj left a comment

Choose a reason for hiding this comment

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

LGTM.

logger.Warnf("event listener game over.")
return perrors.New("nacosRegistry is not available.")
}
services, err := nr.getAllSubscribeServiceNames()
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be added later.

@chickenlj chickenlj merged commit 2e4f4b1 into apache:main Aug 28, 2023
6 checks passed
@FinalT FinalT deleted the nacos branch September 3, 2023 03:56
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.

4 participants