Add inline descriptor Protobuf bytes decoder #13192
Conversation
| { | ||
| DynamicSchema dynamicSchema; | ||
| try { | ||
| byte[] decodedDesc = StringUtils.decodeBase64String(descriptorString); |
There was a problem hiding this comment.
In case of decode failure, IllegalArgumentException is thrown. We should catch it and turn it into a ParseException
There was a problem hiding this comment.
If the descriptor can't be decoded, then we can't proceed, since we won't be able to parse any records, so I don't think this should be a parse exception
There was a problem hiding this comment.
But other exceptions caught by the try-catch here are turned into ParseException.
My concern is that, if the descriptorString is not a valid base64 string, the thrown IllegalArgumentException is not a user-face exception that maybe it's not intuitive for user to realize it's due to the wrong input instead they might think there's bug at the Druid server side.
There was a problem hiding this comment.
I agree that it is not a kind of exception that we can just move over. Though @FrankChen021 makes a good point of surfacing a nicer user-friendly error and throwing an IAE isn't a good way of getting that done.
@jon-wei - What error does the user get with this change if the descriptor string is not valid / can't be decoded?
There was a problem hiding this comment.
Updated to catch IllegalArgumentException and throw a new exception indicating Base64 encoding was not valid.
For valid Base64 and invalid descriptor the existing block
catch (Descriptors.DescriptorValidationException e) {
throw new ParseException(null, e, "Invalid descriptor string: " + descriptorString);
}
handles that
| import java.util.Objects; | ||
| import java.util.Set; | ||
|
|
||
| public class InlineDescriptorProtobufBytesDecoder implements ProtobufBytesDecoder |
There was a problem hiding this comment.
This implementation is very similar to FileBasedProtobufBytesDecoder. I'm wondering if we can abstract some code so that these two classes can share them?
There was a problem hiding this comment.
Refactored to share a base calss
| @JsonProperty("protoMessageType") String protoMessageType | ||
| ) | ||
| { | ||
| this.descriptorString = descriptorString; |
There was a problem hiding this comment.
Better to check if it's null
...src/main/java/org/apache/druid/data/input/protobuf/InlineDescriptorProtobufBytesDecoder.java
Outdated
Show resolved
Hide resolved
…ruid/data/input/protobuf/InlineDescriptorProtobufBytesDecoder.java Co-authored-by: Frank Chen <frankchen@apache.org>
This PR adds a new ProtobufBytesDecoder implementation that allows the user to pass inline the contents of a Protobuf descriptor file, encoded as a Base64 string.
This can be useful when the user does not have the ability/convenience of placing files on the filesystems of the Druid cluster nodes.
This PR has: