cryptoe commented on code in PR #13198:
URL: https://github.com/apache/druid/pull/13198#discussion_r997258744
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/error/MSQWarningReportLimiterPublisher.java:
##########
@@ -35,35 +37,46 @@
public class MSQWarningReportLimiterPublisher implements
MSQWarningReportPublisher
{
- final MSQWarningReportPublisher delegate;
- final long totalLimit;
- final Map<String, Long> errorCodeToLimit;
- final ConcurrentHashMap<String, Long> errorCodeToCurrentCount = new
ConcurrentHashMap<>();
+ private final MSQWarningReportPublisher delegate;
+ private final long totalLimit;
+ private final Map<String, Long> errorCodeToLimit;
+ private final Set<String> criticalWarningCodes;
+ private final ConcurrentHashMap<String, Long> errorCodeToCurrentCount = new
ConcurrentHashMap<>();
+ private final ControllerClient controllerClient;
+ private final String workerId;
+
+ @Nullable
+ private final String host;
long totalCount = 0L;
final Object lock = new Object();
- public MSQWarningReportLimiterPublisher(MSQWarningReportPublisher delegate)
- {
- this(
- delegate,
- Limits.MAX_VERBOSE_WARNINGS,
- ImmutableMap.of(
- CannotParseExternalDataFault.CODE,
Limits.MAX_VERBOSE_PARSE_EXCEPTIONS
- )
- );
- }
-
+ /**
+ * Creates a publisher which publishes the warnings to the controller if
they have not yet exceeded the allowed limit.
+ * Moreover, if a warning is disallowed, i.e. it's limit is set to 0, then
the publisher directly reports the warning
Review Comment:
Nit: It would be much better if we could explain each variable. We really do
not need to mention the tooManyWarningsFault piece as it's just confusing.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/error/MSQWarningReportLimiterPublisher.java:
##########
@@ -74,12 +78,22 @@ public void publishException(int stageNumber, Throwable e)
totalCount = totalCount + 1;
errorCodeToCurrentCount.compute(errorCode, (ignored, count) -> count ==
null ? 1L : count + 1);
+ // Send the warning as an error if it is disallowed altogether
+ if (disallowedWarningCode.contains(errorCode)) {
+ try {
+ controllerClient.postWorkerError(workerId,
MSQErrorReport.fromException(workerId, host, stageNumber, e));
+ }
+ catch (IOException e2) {
Review Comment:
Nit: rename from e2 to postException?
--
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]
