Support for lazily serialized values in Metadata.#6466
Merged
ejona86 merged 16 commits intogrpc:masterfrom Dec 12, 2019
markb74:master
Merged
Support for lazily serialized values in Metadata.#6466ejona86 merged 16 commits intogrpc:masterfrom markb74:master
ejona86 merged 16 commits intogrpc:masterfrom
markb74:master
Conversation
First add a new a Metadata.BinaryStreamMarshaller interface which serializes to/from instances of InputStream, and a corresponding Key.of() factory method. Values set with this type of key will be kept unserialized internally, alongside a reference to the Marshaller.x A new method InternalMetadata.serializePartial(), returns values which are either byte[] or InputStream, and allows transport-specific handling of lazily-serialized values. For the regular serialize() method, stream-marshalled values will be converted to byte[] via an InputStreams.
First add a new a Metadata.BinaryStreamMarshaller interface which serializes to/from instances of InputStream, and a corresponding Key.of() factory method. Values set with this type of key will be kept unserialized internally, alongside a reference to the Marshaller.x A new method InternalMetadata.serializePartial(), returns values which are either byte[] or InputStream, and allows transport-specific handling of lazily-serialized values. For the regular serialize() method, stream-marshalled values will be converted to byte[] via an InputStreams.
ejona86
added
the
kokoro:run
grpc-kokoro
removed
the
kokoro:run
markb74
commented
Nov 26, 2019
ejona86
reviewed
Nov 27, 2019
Member
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
Looks pretty good. One important detail to nail down.
Relax test guarantees for streamed values, and return Object from InternalMetadata.parsedValue
dapengzhang0
reviewed
Dec 2, 2019
dapengzhang0
reviewed
Dec 3, 2019
dapengzhang0
reviewed
Dec 4, 2019
Contributor
dapengzhang0
left a comment
dapengzhang0
left a comment
There was a problem hiding this comment.
Mostly LGTM, just need some more look to be more confident.
| * @return the marshaller object for this key, or null. | ||
| */ | ||
| @Nullable | ||
| final <M> M getMarshaller(Class<M> marshallerClass) { |
Contributor
There was a problem hiding this comment.
Seems this method is only for binary stream marshaller, why not just
@Nullable
@SuppressWarnings("unchecked")
final BinaryStreamMarshaller<T> getBinaryStreamMarshaller() {
if (marshaller instanceof BinaryStreamMarshaller) {
return (BinaryStreamMarshaller<T>) marshaller;
}
return null;
}
Contributor
Author
There was a problem hiding this comment.
I wanted to avoid having the Key class directly depend on a single Marshaller type really. This approach let me avoid that, while supporting other marshaller types as well. If figured this approach was worth it since we'd discussed possibly adding a TextStreamMarshaller later on.
I'm happy either way though, so I'll defer to your preference.
Member
There was a problem hiding this comment.
I saw the method and thought it was a bit odd, but it didn't see an obvious simple alternative that everyone would agree is superior. It is a bit weirder to be BinaryStreamMarshaller-specific as long as it is on the Key class. I'd be prone to moving the method to LazyStreamBinaryKey. But it just seemed beside the point and what is here is fine. We can always change it later.
ejona86
approved these changes
Dec 11, 2019
Member
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
One key detail still, but it wouldn't involve much code churn.
ejona86
approved these changes
Dec 12, 2019
ejona86
added
the
kokoro:run
grpc-kokoro
removed
the
kokoro:run
dapengzhang0
approved these changes
Dec 12, 2019
After an offline discussion it became clear we can do this now, which we think is preferable.
ejona86
added
the
kokoro:run
grpc-kokoro
removed
the
kokoro:run
Member
|
@markb74, thank you! Thank you for working with us on the back-and-forth! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a new a Metadata.BinaryStreamMarshaller interface which serializes to/from instances of InputStream, and a corresponding Key.of() factory method.
Values set with this type of key will be kept unserialized internally, alongside a reference to the Marshaller. A new method InternalMetadata.serializePartial(), returns values which are either
byte[] or InputStream, allowing transport-specific handling of lazily-serialized values.
For the regular serialize() method, stream-marshalled values will be converted to byte[] via InputStreams.
Transport-internal code can also create metadata with pre-parsed values.