-
Notifications
You must be signed in to change notification settings - Fork 8
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
Support persistent session in IntrospectToken method of GRPC client, PLTFRM-72444 #33
Conversation
fc3abda
to
ca77c0a
Compare
51053f9
to
e7d52f8
Compare
c4a838c
to
634c416
Compare
m.LastSessionMeta = "" | ||
} | ||
var requestedResponseCode codes.Code | ||
if mdVal := metadata.ValueFromIncomingContext(ctx, TestMetaRequestedRespCode); len(mdVal) != 0 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- AppendToOutgoingContext method is called here either one or (with requestIDProvider) two times - not event tens or hundreds of times, optimization is not obvious.
- 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.
There was a problem hiding this comment.
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)
634c416
to
7dd7fed
Compare
…partly PLTFRM-72444
7dd7fed
to
8321429
Compare
Support persistent session in IntrospectToken method of GRPC client.
Partly PLTFRM-72444