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

fix: bug with repeated alias for service [NET-434] #1536

Merged
merged 8 commits into from
Mar 31, 2023

Conversation

justprosh
Copy link
Member

@justprosh justprosh commented Mar 28, 2023

+ a little bit of refactoring

@justprosh justprosh requested review from folex and kmd-fl March 28, 2023 07:48
@linear
Copy link

linear bot commented Mar 28, 2023

NET-434 Fix aliases duplication

[2023-03-27T16:20:40.193915Z WARN  particle_services::app_services] Alias `worker-spell` is the same for ca449fcc-aa74-402f-a8d6-a5936a85e946 and ca449fcc-aa74-402f-a8d6-a5936a85e946
[2023-03-27T16:20:40.193936Z WARN  particle_services::app_services] Alias `worker-spell` is the same for ca449fcc-aa74-402f-a8d6-a5936a85e946 and ca449fcc-aa74-402f-a8d6-a5936a85e946
[2023-03-27T16:20:40.193939Z INFO  particle_services::app_services] Persisted service ca449fcc-aa74-402f-a8d6-a5936a85e946 created in 2s 230ms 101us 99ns, aliases: ["worker-spell", "worker-spell", "worker-spell"]

@@ -73,7 +73,9 @@ pub struct Service {

impl Service {
pub fn remove_alias(&mut self, alias: &str) {
self.aliases.retain(|a| a.ne(alias));
if let Some(pos) = self.aliases.iter().position(|x| *x == alias) {
self.aliases.remove(pos);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can make self.aliases a set?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think about that, but we must decide which alias we can use in metrics. Now it's just the first element of vec

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, good point.

Also, we should decide for which services with aliases to collect metrics. In the current model, where we create aliases for every deal, it's just too many services, and an uncontrollable amount of services for metrics collection in prometheus.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we can collect metrics per-worker?

Copy link
Contributor

@kmd-fl kmd-fl Mar 28, 2023

Choose a reason for hiding this comment

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

Difficult question. Guess we can discuss it later when we start reworking metrics for spells/workers/so on.

Copy link
Member

Choose a reason for hiding this comment

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

is there any reason to allow several aliases per service?

Copy link
Member Author

Choose a reason for hiding this comment

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

I really don't know

particle-services/src/app_services.rs Outdated Show resolved Hide resolved
@justprosh justprosh requested a review from kmd-fl March 28, 2023 12:52
@@ -73,7 +73,9 @@ pub struct Service {

impl Service {
pub fn remove_alias(&mut self, alias: &str) {
self.aliases.retain(|a| a.ne(alias));
Copy link
Member

Choose a reason for hiding this comment

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

Why retain wasn't enough? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

It is more than enough, I did add_alias and remove_alias symmetrically. Now add_alias adds 1 alias, and remove_alias removes exactly one alias

@justprosh justprosh requested review from folex and kmd-fl March 30, 2023 15:57
@justprosh justprosh added the e2e Run e2e workflow label Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run e2e workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants