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

Support persistent session in IntrospectToken method of GRPC client, PLTFRM-72444 #33

Conversation

Semerokozlyat
Copy link

Support persistent session in IntrospectToken method of GRPC client.

Partly PLTFRM-72444

@Semerokozlyat Semerokozlyat force-pushed the feature/PLTFRM-72444-caching-idp-client-session-support branch 2 times, most recently from fc3abda to ca77c0a Compare January 29, 2025 17:43
@Semerokozlyat Semerokozlyat force-pushed the feature/PLTFRM-72444-caching-idp-client-session-support branch 3 times, most recently from 51053f9 to e7d52f8 Compare January 30, 2025 15:47
@Semerokozlyat Semerokozlyat force-pushed the feature/PLTFRM-72444-caching-idp-client-session-support branch 8 times, most recently from c4a838c to 634c416 Compare February 6, 2025 08:51
m.LastSessionMeta = ""
}
var requestedResponseCode codes.Code
if mdVal := metadata.ValueFromIncomingContext(ctx, TestMetaRequestedRespCode); len(mdVal) != 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks better to use the metadata.MD wrapper here in case of multiple calls to ValueFromIncomingContext(), otherwise, we have redundant deserialization work.

Copy link
Author

Choose a reason for hiding this comment

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

Reworked, but this code is a part of Token Introspector mock and amount of allocation is not so critical here.

ctx = metadata.AppendToOutgoingContext(ctx, grpcMetaSessionID, sessID)
} else {
ctx = metadata.AppendToOutgoingContext(ctx, grpcMetaAuthorization, makeBearerToken(accessToken))
}
if c.requestIDProvider != nil {
ctx = metadata.AppendToOutgoingContext(ctx, grpcMetaRequestID, c.requestIDProvider(ctx))
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's better to use the metadata.MD wrapper here when making multiple calls to AppendToOutgoingContext() or ValueFromIncomingContext(), as otherwise, redundant (de)serialization work occurs. Additionally, with AppendToOutgoingContext(), you will have +N redundant memory allocations, where N is the number of AppendToOutgoingContext() calls.

Copy link
Author

Choose a reason for hiding this comment

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

No, I cannot agree with this proposal:

  1. AppendToOutgoingContext method is called here either one or (with requestIDProvider) two times - not event tens or hundreds of times, optimization is not obvious.
  2. AppendToOutgoingContext does additional data validation, keys lowercase conversion and so on - with "NewIncomingContext(ctx context.Context, md MD)" method we must do these manually which is easy to forget.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Using more than one call to AppendToOutgoingContext() is inefficient—it causes redundant memory allocations and unnecessary CPU usage. If you need to set several context keys, consider doing something like this:
md := make(metadata.MD)
md.Append("key1", "value1")
md.Append("key2", "value2")
ctx = metadata.NewOutgoingContext(ctx, md)

@Semerokozlyat Semerokozlyat force-pushed the feature/PLTFRM-72444-caching-idp-client-session-support branch from 634c416 to 7dd7fed Compare February 10, 2025 11:08
@Semerokozlyat Semerokozlyat force-pushed the feature/PLTFRM-72444-caching-idp-client-session-support branch from 7dd7fed to 8321429 Compare February 10, 2025 11:18
@vasayxtx vasayxtx merged commit 4a6a2c2 into acronis:main Feb 10, 2025
5 checks passed
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.

5 participants