Jackie-Jiang commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2132749029
##########
pinot-common/src/main/java/org/apache/pinot/common/datablock/ZeroCopyDataBlockSerde.java:
##########
@@ -254,21 +265,37 @@ private long calculateEndOffset(DataBuffer buffer, Header
header) {
return currentOffset;
}
+ /// Deserializes the exceptions and metadata from the stream.
@VisibleForTesting
- static Map<Integer, String> deserializeExceptions(PinotInputStream stream,
Header header)
+ static ErrorsAndMetadata deserializeExceptions(PinotInputStream stream,
Header header)
throws IOException {
if (header._exceptionsLength == 0) {
- return new HashMap<>();
+ return ErrorsAndMetadata.EMPTY;
}
+ long currentOffset = header.getExceptionsStart();
+
stream.seek(header.getExceptionsStart());
int numExceptions = stream.readInt();
- Map<Integer, String> exceptions = new
HashMap<>(HashUtil.getHashMapCapacity(numExceptions));
+ // We reserve extra space for the fake error codes storing stageId,
workerId and serverId
+ Map<Integer, String> exceptions = new
HashMap<>(HashUtil.getHashMapCapacity(numExceptions + 3));
for (int i = 0; i < numExceptions; i++) {
int errCode = stream.readInt();
String errMessage = stream.readInt4UTF();
exceptions.put(errCode, errMessage);
}
- return exceptions;
+
+ long readOffset = stream.getCurrentOffset() - currentOffset;
+ if (readOffset >= header._exceptionsLength) {
+ return new ErrorsAndMetadata(exceptions, -1, -1, "");
Review Comment:
Use `EMPTY`? Same for line 293
##########
pinot-common/src/main/java/org/apache/pinot/common/datablock/ZeroCopyDataBlockSerde.java:
##########
@@ -254,21 +265,37 @@ private long calculateEndOffset(DataBuffer buffer, Header
header) {
return currentOffset;
}
+ /// Deserializes the exceptions and metadata from the stream.
@VisibleForTesting
- static Map<Integer, String> deserializeExceptions(PinotInputStream stream,
Header header)
+ static ErrorsAndMetadata deserializeExceptions(PinotInputStream stream,
Header header)
throws IOException {
if (header._exceptionsLength == 0) {
- return new HashMap<>();
+ return ErrorsAndMetadata.EMPTY;
}
+ long currentOffset = header.getExceptionsStart();
+
stream.seek(header.getExceptionsStart());
int numExceptions = stream.readInt();
- Map<Integer, String> exceptions = new
HashMap<>(HashUtil.getHashMapCapacity(numExceptions));
+ // We reserve extra space for the fake error codes storing stageId,
workerId and serverId
+ Map<Integer, String> exceptions = new
HashMap<>(HashUtil.getHashMapCapacity(numExceptions + 3));
for (int i = 0; i < numExceptions; i++) {
int errCode = stream.readInt();
String errMessage = stream.readInt4UTF();
exceptions.put(errCode, errMessage);
}
- return exceptions;
+
+ long readOffset = stream.getCurrentOffset() - currentOffset;
+ if (readOffset >= header._exceptionsLength) {
+ return new ErrorsAndMetadata(exceptions, -1, -1, "");
+ }
+ int errorMetadataVersion = stream.readInt();
+ if (errorMetadataVersion != ERROR_METADATA_VERSION) {
+ return new ErrorsAndMetadata(exceptions, -1, -1, "");
+ }
+ int stageId = stream.readInt();
+ int workerId = stream.readInt();
+ String serverId = stream.readInt4UTF();
Review Comment:
Do we want it to be `""` or `null`?
##########
pinot-common/src/main/java/org/apache/pinot/common/datablock/MetadataBlock.java:
##########
@@ -48,16 +49,18 @@ public static MetadataBlock newEos() {
return new MetadataBlock();
}
- public static MetadataBlock newError(Map<Integer, String> exceptions) {
- MetadataBlock errorBlock = new MetadataBlock();
+ public static MetadataBlock newError(int stage, int worker, @Nullable String
serverId,
Review Comment:
(nit) Same for other places
```suggestion
public static MetadataBlock newError(int stageId, int workerId, @Nullable
String serverId,
```
##########
pinot-common/src/main/java/org/apache/pinot/common/datablock/ZeroCopyDataBlockSerde.java:
##########
@@ -465,4 +492,29 @@ public int getMetadataStart() {
return _metadataStart;
}
}
+
+ private static class ErrorsAndMetadata {
+ public static final ErrorsAndMetadata EMPTY =
+ new ErrorsAndMetadata(new HashMap<>(), -1, -1, "");
Review Comment:
Should we use `""` as `serverId` or `null`?
--
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
