Jackie-Jiang commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2116473372
##########
pinot-common/src/main/java/org/apache/pinot/common/datablock/BaseDataBlock.java:
##########
@@ -85,7 +85,9 @@
@SuppressWarnings("DuplicatedCode")
public abstract class BaseDataBlock implements DataBlock {
protected static final int HEADER_SIZE = Integer.BYTES * 13;
- // _errCodeToExceptionMap stores exceptions as a map of
errorCode->errorMessage
+ /// _errCodeToExceptionMap stores exceptions as a map of
errorCode->errorMessage
+ /// Only values from QueryErrorCode are allowed as keys, but readers should
not assume that given newer components
Review Comment:
`should assume` or `should not assume`?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -125,6 +126,13 @@ public class PinotClientRequest {
@Inject
private HttpClientConnectionManager _httpConnMgr;
+ private String _instanceId;
+
+ @PostConstruct
+ public void init() {
+ _instanceId =
_brokerConf.getProperty(CommonConstants.Broker.CONFIG_OF_BROKER_ID,
"unknownBroker");
Review Comment:
`_instanceId` is already bind to `brokerInstanceId` in
`BrokerAdminApiApplication`. You should be able to direct use `@Inject` to get
it
##########
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?
Going through the code, I feel there is a better way to handle this without
hacking them into exception map. With the existing format, we know the length
of the exceptions section (`_exceptionsLength). 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.
##########
pinot-common/src/main/java/org/apache/pinot/common/datablock/MetadataBlock.java:
##########
@@ -48,16 +49,17 @@ 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, String serverId,
Map<Integer, String> exceptions) {
Review Comment:
(minor)
```suggestion
public static MetadataBlock newError(int stage, int worker, @Nullable
String serverId, Map<Integer, String> exceptions) {
```
##########
pinot-common/src/main/java/org/apache/pinot/common/datablock/MetadataBlock.java:
##########
@@ -69,6 +71,17 @@ public static MetadataBlock newEosWithStats(List<DataBuffer>
statsByStage) {
public MetadataBlock(List<DataBuffer> statsByStage) {
Review Comment:
Do we want to remove this constructor eventually? Is there scenario where
`stageId` and `workerId` are unavailable?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/blocks/ErrorMseBlock.java:
##########
@@ -24,43 +24,75 @@
import java.util.Collections;
import java.util.EnumMap;
import java.util.Map;
-import javax.validation.constraints.NotNull;
-import org.apache.pinot.common.datablock.DataBlockUtils;
+import javax.annotation.Nullable;
import org.apache.pinot.common.response.ProcessingException;
+import org.apache.pinot.common.utils.ExceptionUtils;
+import org.apache.pinot.query.MseWorkerThreadContext;
import org.apache.pinot.spi.exception.QueryErrorCode;
import org.apache.pinot.spi.exception.QueryException;
+import org.apache.pinot.spi.query.QueryThreadContext;
import org.apache.pinot.spi.utils.JsonUtils;
/// A block that represents a failed execution.
///
public class ErrorMseBlock implements MseBlock.Eos {
+ private final int _stage;
+ private final int _worker;
Review Comment:
(minor) we usually call them `stageId` and `workerId`
##########
pinot-common/src/main/java/org/apache/pinot/common/datablock/MetadataBlock.java:
##########
@@ -35,10 +34,12 @@
public class MetadataBlock extends BaseDataBlock {
private static final Logger LOGGER =
LoggerFactory.getLogger(MetadataBlock.class);
- @VisibleForTesting
- static final int VERSION = 2;
@Nullable
private List<DataBuffer> _statsByStage;
+ private final int _stage;
+ private final int _worker;
Review Comment:
(minor) we usually call them `stageId` and `workerId`
--
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
