agavra commented on code in PR #9711: URL: https://github.com/apache/pinot/pull/9711#discussion_r1014429188
########## pinot-common/src/main/java/org/apache/pinot/common/datablock/MetadataBlock.java: ########## @@ -18,28 +18,118 @@ */ package org.apache.pinot.common.datablock; +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.annotations.VisibleForTesting; import java.io.IOException; import java.nio.ByteBuffer; -import org.apache.pinot.common.utils.DataSchema; /** - * Wrapper for row-wise data table. It stores data in row-major format. + * A block type to indicate some metadata about the current processing state. + * For the different types of metadata blocks see {@link MetadataBlockType}. */ public class MetadataBlock extends BaseDataBlock { - private static final int VERSION = 1; - public MetadataBlock() { - super(0, null, new String[0], new byte[]{0}, new byte[]{0}); + private static final ObjectMapper JSON = new ObjectMapper(); + + @VisibleForTesting + static final int VERSION = 1; + + public enum MetadataBlockType { + /** + * Indicates that this block is the final block to be sent + * (End Of Stream) as part of an operator chain computation. + */ + EOS, + + /** + * An {@code ERROR} metadata block indicates that there was + * some error during computation. To retrieve the error that + * occurred, use {@link MetadataBlock#getExceptions()} + */ + ERROR, + + /** + * A {@code NOOP} metadata block can be sent at any point to + * and should be ignored by downstream - it is often used to + * indicate that the operator chain either has nothing to process + * or has processed data but is not yet ready to emit a result + * block. + */ + NOOP; + + MetadataBlockType() { + } } - public MetadataBlock(DataSchema dataSchema) { - super(0, dataSchema, new String[0], new byte[]{0}, new byte[]{0}); + /** + * Used to serialize the contents of the metadata block conveniently and in + * a backwards compatible way. Use JSON because the performance of metadata block + * SerDe should not be a bottleneck. + */ + @JsonIgnoreProperties(ignoreUnknown = true) + @VisibleForTesting + static class Contents { + + private String _type; + + @JsonCreator + public Contents(@JsonProperty("type") String type) { + _type = type; + } + + @JsonCreator + public Contents() { + _type = null; + } + + public String getType() { + return _type; + } + + public void setType(String type) { + _type = type; + } + } + + private final Contents _contents; + + public MetadataBlock(MetadataBlockType type) { + super(0, null, new String[0], new byte[]{0}, toContents(new Contents(type.name()))); + _contents = new Contents(type.name()); + } + + private static byte[] toContents(Contents type) { + try { + return JSON.writeValueAsBytes(type); + } catch (JsonProcessingException e) { + throw new RuntimeException(e); + } } public MetadataBlock(ByteBuffer byteBuffer) throws IOException { super(byteBuffer); + if (_variableSizeDataBytes != null) { + _contents = JSON.readValue(_variableSizeDataBytes, Contents.class); + } else { + _contents = new Contents(); + } + } + + public MetadataBlockType getType() { + String type = _contents.getType(); + + // if type is null, then we're reading a legacy block where we didn't encode any + // data. assume that it is an EOS block if there's no exceptions and an ERROR block + // otherwise + return type == null + ? (getExceptions().isEmpty() ? MetadataBlockType.EOS : MetadataBlockType.ERROR) + : MetadataBlockType.valueOf(type); Review Comment: > if the metadata block transferred over the wire is of previous version. then using the current version of the code it cannot reconstruct a metadata block back from the byteBuffer that's not correct - it can decode the byteBuffer, the only difference is that it will read it with an empty `_variableBytesData`, which will mean the JSON contents will be empty. (see the `MetadataBlockTest`) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org