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

GO-3913 separate getting identities into multiple calls to identity #1790

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Copy link

github-actions bot commented Nov 6, 2024

New Coverage 50.0% of statements
Patch Coverage 94.1% of changed statements (16/17)

Coverage provided by https://github.com/seriousben/go-patch-cover-action

} else {
requestedIdentities = identities[j:]
}
repoIdentities, err = s.identityRepoClient.IdentityRepoGet(ctx, requestedIdentities, []string{identityRepoDataKind})
Copy link
Member

Choose a reason for hiding this comment

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

maybe send requests in parallel?

} else {
resultIdentities = append(resultIdentities, repoIdentities...)
}
j += identityBatch
Copy link
Member

Choose a reason for hiding this comment

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

maybe move it to the loop expression?

for j := 0; j < len(identities); j+= identityBatch {

}
repoIdentities, err = s.identityRepoClient.IdentityRepoGet(ctx, requestedIdentities, []string{identityRepoDataKind})
if err != nil {
log.Error("error getting identity repo data", zap.Error(err))
Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned about the number of log messages. Is it possible that we will log unnecessary errors?

@@ -280,6 +278,32 @@ func (s *service) getIdentitiesDataFromRepo(ctx context.Context, identities []st
return res, nil
}

func (s *service) batchIdentities(ctx context.Context, identities []string) ([]*identityrepoproto.DataWithIdentity, []string) {
resultIdentities := make([]*identityrepoproto.DataWithIdentity, 0, len(identities))
Copy link
Contributor

Choose a reason for hiding this comment

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

Just summing all the comments, there should be one abstract function, which splits the []T to several batches of size <= N (take N as 200 pls), this function should be in package slice or whatever and tested

and then call in parallel some named func for every batch which returns results using waitgroup, gather everything and return.

This func should either get stuff from remote or if nothing in remote get stuff from local.

I would do it like this

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.

3 participants