Skip to content

Conversation

@knanao
Copy link
Member

@knanao knanao commented Jun 20, 2022

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

@knanao knanao marked this pull request as ready for review June 20, 2022 22:09
@knanao knanao enabled auto-merge (squash) June 20, 2022 22:17
matcher = cfg.Matcher
handler = cfg.Handler
)
notHandledEvents := w.eventLister.ListNotHandled(matcher.Name, matcher.Labels, milestone+1, numToMakeOutdated)
Copy link
Member

Choose a reason for hiding this comment

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

I start feeling that this RPC is being called quite frequently.
In the future, maybe we need to add a new RPC to check multiple events simultaneously.

Copy link
Member Author

@knanao knanao Jun 21, 2022

Choose a reason for hiding this comment

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

Exactly, but it's not so frequently in the current spec because the count of eventCfgs is equal to the count of apps and the count of e.Configs is almost to be one except for edge cases.
So Let's add a new RPC when we add a new event type.

Copy link
Member Author

Choose a reason for hiding this comment

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

But if the project has a fairly large number of applications, the number of the RPC calls is likely to be high.🤔

Copy link
Member

Choose a reason for hiding this comment

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

I see. It would be nice if the number of calls did not depend on the number of applications.
But it could be an improvement point.

Copy link
Member

@nghialv nghialv left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@khanhtc1202 khanhtc1202 left a comment

Choose a reason for hiding this comment

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

Good to go 🙌

@knanao knanao merged commit 2f92d0c into master Jun 21, 2022
@knanao knanao deleted the fix-eventwatcher branch June 21, 2022 03:00
@github-actions github-actions bot mentioned this pull request Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants