FrankChen021 commented on code in PR #18525:
URL: https://github.com/apache/druid/pull/18525#discussion_r4062501319
##########
processing/src/main/java/org/apache/druid/segment/incremental/RowIngestionMetersTotals.java:
##########
@@ -44,32 +45,66 @@ public RowIngestionMetersTotals(
@JsonProperty("processedWithError") long processedWithError,
@JsonProperty("thrownAway") long thrownAway,
@JsonProperty("thrownAwayByReason") @Nullable Map<String, Long>
thrownAwayByReason,
- @JsonProperty("unparseable") long unparseable
+ @JsonProperty("unparseable") long unparseable,
+ @JsonProperty("filtered") long filtered
Review Comment:
[P1] Retain the previous full totals constructor
**Finding:** The @JsonCreator is also a public constructor; adding filtered
changes the old (long, long, long, long, Map<String, Long>, long) descriptor,
while the added overloads preserve only the five-argument constructors and the
new six-argument no-map variant. Previously compiled callers using the full
totals constructor can fail with NoSuchMethodError when building reports,
independent of header filtering.
**Suggestion:** Retain a six-argument overload with thrownAwayByReason and
no filtered argument that delegates with filtered=0 while leaving the Jackson
creator on the new signature.
##########
extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/KafkaIndexTaskIOConfig.java:
##########
@@ -66,7 +68,8 @@ public KafkaIndexTaskIOConfig(
@JsonProperty("configOverrides") @Nullable KafkaConfigOverrides
configOverrides,
@JsonProperty("multiTopic") @Nullable Boolean multiTopic,
@JsonProperty("refreshRejectionPeriodsInMinutes") Long
refreshRejectionPeriodsInMinutes,
- @JsonProperty("boundedStreamConfig") @Nullable BoundedStreamConfig
boundedStreamConfig
+ @JsonProperty("boundedStreamConfig") @Nullable BoundedStreamConfig
boundedStreamConfig,
+ @JsonProperty("headerBasedFilterConfig") @Nullable
KafkaHeaderBasedFilterConfig headerBasedFilterConfig
Review Comment:
[P1] Retain the previous full IO-config constructor
**Finding:** Adding headerBasedFilterConfig to the public constructor that
previously ended at boundedStreamConfig removes the old JVM descriptor. The
overload below only preserves the separate convenience constructor; code or
extensions compiled against the prior full constructor will fail with
NoSuchMethodError at task/spec construction even when filtering is not
configured.
**Suggestion:** Add a public overload with the old full parameter list that
delegates to this constructor with a null header filter, and cover the
descriptor in an API-compatibility test.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/stats/TaskRealtimeMetricsMonitor.java:
##########
@@ -105,6 +105,9 @@ public boolean doMonitor(ServiceEmitter emitter)
emitter.emit(builder.setMetric("ingest/events/processed",
rowIngestionMetersTotals.getProcessed() -
previousRowIngestionMetersTotals.getProcessed()));
+ final long filtered = rowIngestionMetersTotals.getFiltered() -
previousRowIngestionMetersTotals.getFiltered();
+ emitter.emit(builder.setMetric("ingest/events/filtered", filtered));
Review Comment:
[P2] Clear the reason dimension for filtered metrics
**Finding:** If this monitor interval emits any thrownAway-by-reason event,
the shared ServiceMetricEvent.Builder still contains that last reason when the
new ingest/events/filtered event is emitted. ServiceMetricEvent copies all user
dimensions, so the filtered event is tagged with an unrelated reason (and the
same stale dimension leaks into subsequent metrics), which corrupts logging or
custom-emitter output and can split or mislabel metric series.
**Suggestion:** Build filtered and other non-reason metrics with a fresh
builder or explicitly clear the reason dimension after the per-reason
emissions, and add a test covering both counters in one monitor tick.
--
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]
