You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2025/05/28 08:57:25 UTC
[PR] MSE error management improvements [pinot]
gortiz opened a new pull request, #15919:
URL: https://github.com/apache/pinot/pull/15919
Several improvements on the error management in MSE.
1. Include all exception chain messages on errors that don't extend QueryException
2. Include the server and stage that fail in the error message
3. Include the error code description in Pinot UI.
Use this image as an example of the changes:
Here the query fails because the cast is illegal. As a difference, we can see that the UI now prints the `(QueryExecutionError)` close to `Error Code: 200`. Also, the error message now starts with:
> Received 1 error from stage 2 on server Server_192.168.1.42_7052
Where `Server_192.168.1.42_7052` is the name of the server in quickstart, but should be something like `pinot-pinot-server-2` in a deployment with k8s.
The line changes are significant because, in order to send the stage and server id without modifying all possible error messages, I had to introduce the service id in QueryThreadContext and also include these fields in the error blocks. In order to serialize this new information without breaking binary serialization compatibility, I'm using the error map in DataBlock. Specifically, I'm using negative error codes, which weren't used before. I've also added javadocs to make this restriction explicit.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2245145403
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MailboxSendOperator.java:
##########
@@ -204,11 +204,7 @@ protected MseBlock getNextBlock() {
try {
MseBlock block = _input.nextBlock();
if (block.isEos()) {
- if (_context.isSendStats()) {
- sendEos((MseBlock.Eos) block);
- } else {
- sendEos(SuccessMseBlock.INSTANCE);
- }
Review Comment:
We also sent the stats, even if the predicate indicates otherwise
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2113604487
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MailboxSendOperator.java:
##########
@@ -204,11 +204,7 @@ protected MseBlock getNextBlock() {
try {
MseBlock block = _input.nextBlock();
if (block.isEos()) {
- if (_context.isSendStats()) {
- sendEos((MseBlock.Eos) block);
- } else {
- sendEos(SuccessMseBlock.INSTANCE);
- }
Review Comment:
This was erroneous. In case the block was an error and isSendStats was false, we sent a successful EOS.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2132208909
##########
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:
> Is there scenario where stageId and workerId are unavailable?
Yes, MetadataBlock is used to serialize any EOS, including those that succeed and those that don't. But stageId and workerId are only used on error blocks. We could include them in successful blocks as well, but that won't be very useful and would make SuccessMseBlock stateful, which is not great given they can be used as singletons now.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2120477668
##########
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:
I think the text is correct. Readers cannot assume the only values that will be received are the ones found in QueryErrorCode because a more modern server may have more error codes and therefore send an error the reader doesn't recognize. What to do in that case (ignore the error, map it to unknown, keep it as it is) is the reader responsibility.
Right now code in master fails in case new error codes are received. The consequence is that in case we add a new error code (we didn't in a while), if that new error happens during an upgrade, the receiver operator of a non-upgraded server will fail with an internal error instead of the correct one. We should change the code to be more resilient, but given that it only happens when errors are thrown during an upgrade, it doesn't to be a blocking issue, but at least I wanted to register that in the comment
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2113594483
##########
pinot-controller/src/main/resources/app/pages/Query.tsx:
##########
@@ -635,9 +675,9 @@ const QueryPage = () => {
>
{
resultData.columns.length > 0 ? (
- <Typography variant='body2'>Partial results due to exceptions. Please toggle the switch to view details.</Typography>
Review Comment:
Now the message is printed by default, no need to switch to view details. Instead I've changed the text to say that stats may be partial
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2121035156
##########
pinot-spi/src/main/java/org/apache/pinot/spi/query/QueryThreadContext.java:
##########
@@ -415,6 +429,18 @@ public static void setQueryEngine(String queryEngine) {
get().setQueryEngine(queryEngine);
}
+ /**
+ * Returns the serviceId of the query.
+ *
+ * This is usually the id that identifies the server, broker, controller, etc.
+ *
+ * The default value of {@code null} means the serviceId is not set.
+ * @throws IllegalStateException if the {@link QueryThreadContext} is not initialized
+ */
+ public static String getServiceId() {
+ return get().getServiceId();
+ }
Review Comment:
Fixed
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2113590167
##########
pinot-common/src/main/java/org/apache/pinot/common/datablock/DataBlockUtils.java:
##########
@@ -102,19 +97,6 @@ private DataBlockUtils() {
static final int VERSION_TYPE_SHIFT = 5;
- public static MetadataBlock getErrorDataBlock(Exception e) {
- if (e instanceof ProcessingException) {
- int errorCodeId = ((ProcessingException) e).getErrorCode();
- return getErrorDataBlock(Collections.singletonMap(errorCodeId, extractErrorMsg(e)));
- } else if (e instanceof QueryException) {
- int errorCodeId = ((QueryException) e).getErrorCode().getId();
- return getErrorDataBlock(Collections.singletonMap(errorCodeId, extractErrorMsg(e)));
- } else {
- // TODO: Pass in meaningful error code.
- return getErrorDataBlock(Map.of(QueryErrorCode.UNKNOWN.getId(), extractErrorMsg(e)));
- }
- }
Review Comment:
This and `getErrorDataBlock(Map<Integer, String> exceptions)` are not used anymore
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2113596184
##########
pinot-core/pom.xml:
##########
@@ -165,6 +165,10 @@
<artifactId>annotations</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.assertj</groupId>
+ <artifactId>assertj-core</artifactId>
+ </dependency>
Review Comment:
Used in pinot-core/src/test/java/org/apache/pinot/queries/HistogramQueriesTest.java to improve error catching, test legibility and error message
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #15919:
URL: https://github.com/apache/pinot/pull/15919
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2111322777
##########
pinot-common/src/main/java/org/apache/pinot/common/datablock/ZeroCopyDataBlockSerde.java:
##########
@@ -46,6 +46,10 @@
*/
public class ZeroCopyDataBlockSerde implements DataBlockSerde {
+ private static final int STAGE_ID_FAKE_ERROR_CODE = -1;
Review Comment:
Ensure that the selected fake error codes (-1, -2, -3) cannot conflict with real error code values in any future changes. Consider documenting this design decision more explicitly to aid future maintainers.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/BaseJoinOperator.java:
##########
@@ -327,13 +327,15 @@ protected boolean isMaxRowsLimitReached(int numJoinedRows) {
protected static void throwForJoinRowLimitExceeded(String reason) {
throw QueryErrorCode.SERVER_RESOURCE_LIMIT_EXCEEDED.asException(
reason
- + ". Consider increasing the limit for the maximum number of rows in a join either via the query option '"
- + QueryOptionKey.MAX_ROWS_IN_JOIN + "' or the '" + JoinHintOptions.MAX_ROWS_IN_JOIN + "' hint in the '"
- + PinotHintOptions.JOIN_HINT_OPTIONS
- + "'. Alternatively, if partial results are acceptable, the join overflow mode can be set to '"
- + JoinOverFlowMode.BREAK.name() + "' either via the query option '" + QueryOptionKey.JOIN_OVERFLOW_MODE
- + "' or the '" + JoinHintOptions.JOIN_OVERFLOW_MODE + "' hint in the '" + PinotHintOptions.JOIN_HINT_OPTIONS
- + "'.");
+ + ".\nConsider increasing the limit for the maximum number of rows in a join either via:\n"
Review Comment:
[nitpick] Consider extracting and centralizing this multi-line error message formatting into a constant or helper method to improve consistency and maintainability across the codebase.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2113602711
##########
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;
+ private final String _serverId;
private final EnumMap<QueryErrorCode, String> _errorMessages;
+ /// Kept for backward compatibility.
+ ///
+ /// @deprecated Use [#fromMap(Map)] instead
+ @Deprecated
public ErrorMseBlock(Map<QueryErrorCode, String> errorMessages) {
+ this(-1, -1, null, errorMessages);
+ }
+
+ public ErrorMseBlock(int stage, int worker, String serverId, Map<QueryErrorCode, String> errorMessages) {
+ _stage = stage;
+ _worker = worker;
+ _serverId = serverId;
Preconditions.checkArgument(!errorMessages.isEmpty(), "Error messages cannot be empty");
_errorMessages = new EnumMap<>(errorMessages);
}
+ public static ErrorMseBlock fromMap(Map<QueryErrorCode, String> errorMessages) {
+ int stage;
+ int worker;
+ String server;
+ if (MseWorkerThreadContext.isInitialized()) {
+ stage = MseWorkerThreadContext.getStageId();
+ worker = MseWorkerThreadContext.getWorkerId();
+ server = QueryThreadContext.getServiceId();
+ } else {
+ stage = -1; // Default value when not initialized
+ worker = -1; // Default value when not initialized
+ server = null; // Default value when not initialized
+ }
+ return new ErrorMseBlock(stage, worker, server, errorMessages);
+ }
+
public static ErrorMseBlock fromException(Exception e) {
QueryErrorCode errorCode;
+ boolean extractTrace;
if (e instanceof QueryException) {
errorCode = ((QueryException) e).getErrorCode();
+ extractTrace = false;
} else if (e instanceof ProcessingException) {
errorCode = QueryErrorCode.fromErrorCode(((ProcessingException) e).getErrorCode());
+ extractTrace = true;
} else {
errorCode = QueryErrorCode.UNKNOWN;
+ extractTrace = true;
}
- String errorMessage = shouldIncludeStackTrace(errorCode) ? DataBlockUtils.extractErrorMsg(e) : e.getMessage();
- return new ErrorMseBlock(Collections.singletonMap(errorCode, errorMessage));
+ String errorMessage = extractTrace ? ExceptionUtils.consolidateExceptionMessages(e) : e.getMessage();
+ return fromMap(Collections.singletonMap(errorCode, errorMessage));
Review Comment:
New semantic:
1. If the exception was a QueryException, we assume the message is the one we want to send to the user. No extra processing is done, which means if there is a cause, we don't include its message.
2. On any other case, we iterate over the causes and print the message (but not the stack trace) of the whole exception chain.
Previously, we only did that for exceptions whose error code was UNKNOWN.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #15919:
URL: https://github.com/apache/pinot/pull/15919#issuecomment-2940156794
Now that other smaller o11y improvements have been moved to dedicated branches, this PR is a little bit easier to review and focused on how to include the originator stages and servers on error. Can you take a new look?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2137677116
##########
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:
I'm using "".
I think it is better (may avoid NPEs) and simplifies the serialized code, as it is simpler to write an empty string than a null value. We can then transform this empty string into null when converting it to a block if needed
##########
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:
We cannot use EMPTY, as exceptions could be non-empty.
##########
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:
> Is there scenario where stageId and workerId are unavailable?
Yes, MetadataBlock is used to serialize any EOS, including those that succeed and those that don't. But stageId and workerId are only used on error blocks. We could include them in successful blocks as well, but that won't be very useful and would make SuccessMseBlock stateful, which is not great given they can be used as singletons now.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2121033175
##########
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:
Fixed
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2121033671
##########
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:
Fixed
##########
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:
Fixed
##########
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:
Fixed
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #15919:
URL: https://github.com/apache/pinot/pull/15919#issuecomment-2918716437
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/15919?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `58.74126%` with `59 lines` in your changes missing coverage. Please review.
> Project coverage is 56.39%. Comparing base [(`1a476de`)](https://app.codecov.io/gh/apache/pinot/commit/1a476dea72ebf1c15e14ded28ae60c385f71b2c3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`b123164`)](https://app.codecov.io/gh/apache/pinot/commit/b123164f258cc795268f7390f72d3426bb20e598?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 156 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/15919?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...apache/pinot/common/datablock/DataBlockEquals.java](https://app.codecov.io/gh/apache/pinot/pull/15919?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fdatablock%2FDataBlockEquals.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZGF0YWJsb2NrL0RhdGFCbG9ja0VxdWFscy5qYXZh) | 11.76% | [12 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15919?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...org/apache/pinot/spi/query/QueryThreadContext.java](https://app.codecov.io/gh/apache/pinot/pull/15919?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Fquery%2FQueryThreadContext.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvcXVlcnkvUXVlcnlUaHJlYWRDb250ZXh0LmphdmE=) | 25.00% | [12 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15919?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...apache/pinot/query/service/server/QueryServer.java](https://app.codecov.io/gh/apache/pinot/pull/15919?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fservice%2Fserver%2FQueryServer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvc2VydmljZS9zZXJ2ZXIvUXVlcnlTZXJ2ZXIuamF2YQ==) | 53.33% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15919?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...pinot/common/datablock/ZeroCopyDataBlockSerde.java](https://app.codecov.io/gh/apache/pinot/pull/15919?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fdatablock%2FZeroCopyDataBlockSerde.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZGF0YWJsb2NrL1plcm9Db3B5RGF0YUJsb2NrU2VyZGUuamF2YQ==) | 72.72% | [3 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15919?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...che/pinot/core/transport/grpc/GrpcQueryServer.java](https://app.codecov.io/gh/apache/pinot/pull/15919?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Ftransport%2Fgrpc%2FGrpcQueryServer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvZ3JwYy9HcnBjUXVlcnlTZXJ2ZXIuamF2YQ==) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15919?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ot/query/runtime/operator/MailboxSendOperator.java](https://app.codecov.io/gh/apache/pinot/pull/15919?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Foperator%2FMailboxSendOperator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9NYWlsYm94U2VuZE9wZXJhdG9yLmphdmE=) | 50.00% | [3 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15919?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../pinot/query/service/dispatch/QueryDispatcher.java](https://app.codecov.io/gh/apache/pinot/pull/15919?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fservice%2Fdispatch%2FQueryDispatcher.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvc2VydmljZS9kaXNwYXRjaC9RdWVyeURpc3BhdGNoZXIuamF2YQ==) | 55.55% | [2 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15919?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ache/pinot/query/runtime/blocks/ErrorMseBlock.java](https://app.codecov.io/gh/apache/pinot/pull/15919?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Fblocks%2FErrorMseBlock.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9ibG9ja3MvRXJyb3JNc2VCbG9jay5qYXZh) | 86.95% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15919?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...org/apache/pinot/spi/exception/QueryErrorCode.java](https://app.codecov.io/gh/apache/pinot/pull/15919?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Fexception%2FQueryErrorCode.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZXhjZXB0aW9uL1F1ZXJ5RXJyb3JDb2RlLmphdmE=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15919?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (1a476de) and HEAD (b123164). Click for more details.
>
> <details><summary>HEAD has 68 uploads less than BASE</summary>
>
>| Flag | BASE (1a476de) | HEAD (b123164) |
>|------|------|------|
>|java-21|10|1|
>|unittests|6|1|
>|unittests1|3|1|
>|skip-bytebuffers-true|4|0|
>|temurin|15|1|
>|unittests2|3|0|
>|skip-bytebuffers-false|8|0|
>|java-11|5|0|
>|integration|9|0|
>|integration2|3|0|
>|integration1|3|0|
>|custom-integration1|3|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #15919 +/- ##
============================================
- Coverage 62.90% 56.39% -6.51%
+ Complexity 1386 690 -696
============================================
Files 2867 2272 -595
Lines 163354 124067 -39287
Branches 24952 19783 -5169
============================================
- Hits 102755 69970 -32785
+ Misses 52847 48310 -4537
+ Partials 7752 5787 -1965
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/15919/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/15919/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/15919/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/15919/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/15919/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/15919/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/15919/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.39% <58.74%> (-6.43%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/15919/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/15919/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/15919/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.39% <58.74%> (-6.51%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/15919/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.39% <58.74%> (-6.51%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/15919/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.39% <58.74%> (+0.57%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/15919/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/15919?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2113593286
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -120,7 +120,7 @@ public static SqlNodeAndOptions compileToSqlNodeAndOptions(String sql)
sqlNodeAndOptions.setParseTimeNs(System.nanoTime() - parseStartTimeNs);
return sqlNodeAndOptions;
} catch (Throwable e) {
- throw new SqlCompilationException("Caught exception while parsing query: " + sql, e);
+ throw new SqlCompilationException("Caught exception while parsing query: " + sql + ": " + e.getMessage(), e);
Review Comment:
This is mandatory, otherwise the message doesn't include the actual compilation issue
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2116509753
##########
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.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2113599332
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageWithoutStatsIntegrationTest.java:
##########
@@ -152,8 +152,7 @@ public void testIntersection()
assertEquals(children.size(), 1);
JsonNode child = children.get(0);
- assertEquals(child.get("type").asText(), "MAILBOX_SEND");
+ assertEquals(child.get("type").asText(), "EMPTY_MAILBOX_SEND");
Review Comment:
There was an error in MailboxSendOperator when send stats was false. As a result, a successful EOS instead of an error was sent. That had the side effect of adding an empty stats block, which means the node was printed as MAILBOX_SEND instead of the correct EMPTY_MAILBOX_SEND
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2113591666
##########
pinot-common/src/main/java/org/apache/pinot/common/datablock/ZeroCopyDataBlockSerde.java:
##########
@@ -46,6 +46,10 @@
*/
public class ZeroCopyDataBlockSerde implements DataBlockSerde {
+ private static final int STAGE_ID_FAKE_ERROR_CODE = -1;
Review Comment:
Already done
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #15919:
URL: https://github.com/apache/pinot/pull/15919#discussion_r2120111385
##########
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:
+1 on trying to avoid hacking the exception map
##########
pinot-spi/src/main/java/org/apache/pinot/spi/query/QueryThreadContext.java:
##########
@@ -415,6 +429,18 @@ public static void setQueryEngine(String queryEngine) {
get().setQueryEngine(queryEngine);
}
+ /**
+ * Returns the serviceId of the query.
+ *
+ * This is usually the id that identifies the server, broker, controller, etc.
+ *
+ * The default value of {@code null} means the serviceId is not set.
+ * @throws IllegalStateException if the {@link QueryThreadContext} is not initialized
+ */
+ public static String getServiceId() {
+ return get().getServiceId();
+ }
Review Comment:
Why not use the term `instanceId` directly instead? `serviceId` is confusing because it's not a term used elsewhere in the codebase.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] MSE error management improvements [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org