cronet: allow multiple annotation objects attached to stream #3695
Conversation
| */ | ||
| public static CallOptions withAnnotation(CallOptions callOptions, Object annotation) { | ||
| Set<Object> annotations = callOptions.getOption(CRONET_ANNOTATIONS_KEY); | ||
| if (annotations != null) { |
There was a problem hiding this comment.
if (annotations == null) {
annotations = new HashSet<Object>();
}
annotations.add(annotation);However, since options are mostly immutable, it may be strange this modifies the set. Notice that the withOption at the end isn't necessary if the key was already present.
There was a problem hiding this comment.
Switched to Collection backed by ArrayList (also matches how Cronet handles the annotations) and recreate a new ArrayList on each call. I think this better respects the immutability of call options; is this type of change what you had in mind?
There was a problem hiding this comment.
Yeah, that's what I was thinking.
| if (annotations != null) { | ||
| annotations.add(annotation); | ||
| } else { | ||
| annotations = new HashSet<Object>(); |
There was a problem hiding this comment.
What happens if you don't use a set? (say, a list instead?)
| } | ||
| return callOptions.withOption(CronetCallOptions.CRONET_ANNOTATIONS_KEY, annotations); | ||
| newAnnotations.add(annotation); | ||
| return callOptions.withOption(CronetCallOptions.CRONET_ANNOTATIONS_KEY, newAnnotations); |
There was a problem hiding this comment.
You may consider wrapping with unmodifiableList.
| */ | ||
| public static CallOptions withAnnotation(CallOptions callOptions, Object annotation) { | ||
| Set<Object> annotations = callOptions.getOption(CRONET_ANNOTATIONS_KEY); | ||
| if (annotations != null) { |
There was a problem hiding this comment.
Yeah, that's what I was thinking.
3 participants
Cronet allows attaching multiple annotation objects to a bidirectional stream (javadoc). This PR deprecates the
CRONET_ANNOTATION_KEY, which allowed at most one annotation object, and instead adds aCronetCallOptions#withAnnotationwhich lets users add annotation objects to a set of annotations attached to the Cronet stream, more closely matching the semantics exposed by Cronet.This (or some alternative) API change is helpful to accommodate existing users of
CRONET_ANNOTATION_KEYalongside a mechanism for exporting gRPC-specific metrics to Cronet'sRequestFinishedInfo.Listenerimplementations using the same annotation mechanism, which seems to require multiple annotations attached to the Cronet stream.