suneet-s commented on a change in pull request #10336:
URL: https://github.com/apache/druid/pull/10336#discussion_r482204639
##########
File path:
server/src/main/java/org/apache/druid/segment/realtime/appenderator/PeonAppenderatorsManager.java
##########
@@ -129,7 +137,9 @@ public Appenderator createOfflineAppenderatorForTask(
dataSegmentPusher,
objectMapper,
indexIO,
- indexMerger
+ indexMerger,
+ rowIngestionMeters,
+ parseExceptionHandler
Review comment:
General comment here for all the new functions that accept
`parseExceptionHandler`. `parseExceptionHandler` tends to be lazily initialized
throughout the codebase, but I don't see safeguards against using an
un-initialized parseExceptionHandler. Perhaps more use of `@Nullable`
annotation will help surface potential places where it may be used before it's
initialized
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/common/task/BatchAppenderators.java
##########
@@ -59,7 +65,9 @@ public static Appenderator newAppenderator(
TaskToolbox toolbox,
DataSchema dataSchema,
AppenderatorConfig appenderatorConfig,
- DataSegmentPusher segmentPusher
+ DataSegmentPusher segmentPusher,
+ RowIngestionMeters rowIngestionMeters,
+ ParseExceptionHandler parseExceptionHandler
Review comment:
Is there ever a case where the `rowIngestionMeters` is different than
the one used in `parseExceptionHandler`? If not, I think we should just pass in
the parseExceptionHandler.
Similar comment on line 45
##########
File path:
processing/src/main/java/org/apache/druid/segment/incremental/NoopRowIngestionMeters.java
##########
@@ -0,0 +1,88 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.segment.incremental;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class NoopRowIngestionMeters implements RowIngestionMeters
Review comment:
Javadocs please.
Should this just be a singleton?
##########
File path:
processing/src/main/java/org/apache/druid/segment/incremental/OnheapIncrementalIndex.java
##########
@@ -186,7 +185,7 @@ protected AddToFactsResult addToFacts(
} else {
// We lost a race
aggs = concurrentGet(prev);
- parseExceptionMessages = doAggregate(metrics, aggs, rowContainer, row);
+ doAggregate(metrics, aggs, rowContainer, row, parseExceptionMessages);
Review comment:
This is a change in behavior. parseExceptionMessages will add to the
parseExceptionMessages that were found on line 165. Previously it was re-set.
Was this intentional?
##########
File path:
processing/src/main/java/org/apache/druid/segment/incremental/OffheapIncrementalIndex.java
##########
@@ -233,16 +232,13 @@ protected AddToFactsResult addToFacts(
}
catch (ParseException e) {
// "aggregate" can throw ParseExceptions if a selector expects
something but gets something else.
- if (getReportParseExceptions()) {
- throw new ParseException(e, "Encountered parse error for
aggregator[%s]", getMetricAggs()[i].getName());
- } else {
- log.debug(e, "Encountered parse error, skipping aggregator[%s].",
getMetricAggs()[i].getName());
- }
+ log.debug(e, "Encountered parse error, skipping aggregator[%s].",
getMetricAggs()[i].getName());
+ parseExceptionMessages.add(e.getMessage());
Review comment:
This is a change in behavior. Previously the aggs would fail fast, but
now it tries to parse all of them before bubbling up all the parse exceptions.
Was this change intentional? I see a similar change in at least the Onheap
implementation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
