gortiz commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2120757113
##########
pinot-common/src/main/java/org/apache/pinot/common/datablock/ZeroCopyDataBlockSerde.java:
##########
@@ -114,11 +118,36 @@ private void serializeExceptions(DataBlock dataBlock,
PinotOutputStream into)
return;
}
- into.writeInt(errCodeToExceptionMap.size());
+ // We add 3 fake error codes to the map to store stage, worker and server
ID.
+ // These fake error codes are only used in the serialized format.
+ // When deserialized, the returned datablock will not contain these fake
error codes.
+ into.writeInt(errCodeToExceptionMap.size() + 3);
for (Map.Entry<Integer, String> entry : errCodeToExceptionMap.entrySet()) {
into.writeInt(entry.getKey());
into.writeInt4String(entry.getValue());
}
+ int stage;
+ int worker;
+ String serverId;
+ if (dataBlock instanceof MetadataBlock) {
+ MetadataBlock metablock = (MetadataBlock) dataBlock;
+ stage = metablock.getStage();
+ worker = metablock.getWorker();
+ serverId = metablock.getServerId() != null ? metablock.getServerId() :
"";
+ } else {
+ stage = -1; // Default value when not initialized
+ worker = -1; // Default value when not initialized
+ serverId = ""; // Default value when not initialized
+ }
+ // Add fake error codes for stage, worker and server ID
+ into.writeInt(STAGE_ID_FAKE_ERROR_CODE);
Review Comment:
> What will happen when the old version server see the new version response?
Will the result include 3 errors with fake error code?
In this situation, the old server receiving the error will fail with an
internal error, given mailbox receiver operator will end up calling
QueryErrorCode.fromErrorCode, which fails with
`IllegalArgumentException("Invalid error code: " + errorCode);`.
> If we append the new fields after the exception map, after deserializing
the map we can tell whether there are extra info following the map based on
_exceptionsLength. For old servers, given we only use _exceptionsLength to
identify whether there is exception section, it should be able to handle new
format.
This is a bit tricky. In general cases, we cannot do what you suggest
because older code wouldn't consume these final bytes, which could produce
other errors. But this specific part of the code seems to be protected against
that because the next thing we do is to read the stats, which have their own
section in the header.
I've pushed a new code that should be similar to what you suggested. I've
also improved the serialization to store stageId and workerId as ints now that
we don't need to follow the exception format (which requires string values)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
