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

VAULT-14733: Refactor processClientRecord in activity log #19933

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

miagilepner
Copy link
Contributor

@miagilepner miagilepner commented Apr 3, 2023

This makes it easier to split up the precomputed query worker and unit test its components.

@miagilepner miagilepner added this to the 1.14 milestone Apr 3, 2023
@miagilepner miagilepner requested review from mpalmi and a team April 3, 2023 11:56
@miagilepner miagilepner marked this pull request as ready for review April 3, 2023 11:58
@hghaf099
Copy link
Contributor

hghaf099 commented Apr 3, 2023

Do we need to backport to 1.9 or 1.10?

}
}

func (p *processCounts) contains(client *activity.EntityRecord) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this used? I couldn't find a reference to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually not used here - it's used in testing in a later change that's on top of this one. However, since it relates to the code here, I included it. Let me know if you would prefer that I remove it and add it to a later PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, cool! Thanks for the context!

Copy link
Contributor

@raskchanky raskchanky left a comment

Choose a reason for hiding this comment

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

Dramatic improvement

@miagilepner
Copy link
Contributor Author

Do we need to backport to 1.9 or 1.10?

I labeled it as a backport to older versions so that this code will be available when we need to test upgrade scenarios for client counts. However, it may make more sense to get the implementation working in supported versions, then backport later. I'll remove the labels here and make a separate ticket in the epic for earlier version backports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants