Re: [PR] feat: add pre-ingestion filtering based on Kafka headers (druid)

["FrankChen021 (via GitHub)" <gi...@apache.org>]

FrankChen021 commented on code in PR #18525:
URL: https://github.com/apache/druid/pull/18525#discussion_r3969882995


##########
extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorIOConfig.java:
##########
@@ -77,6 +78,7 @@ public KafkaSupervisorIOConfig(
       @JsonProperty("earlyMessageRejectionPeriod") Period earlyMessageRejectionPeriod,
       @JsonProperty("lateMessageRejectionStartDateTime") DateTime lateMessageRejectionStartDateTime,
       @JsonProperty("configOverrides") KafkaConfigOverrides configOverrides,
+      @JsonProperty("headerBasedFilterConfig") KafkaHeaderBasedFilterConfig headerBasedFilterConfig,

Review Comment:
   [P2] Preserve the existing public constructor
   
   Adding `headerBasedFilterConfig` to this public constructor removes the previous JVM descriptor, so callers compiled against the existing Kafka indexing-service API fail with `NoSuchMethodError` (and source callers stop compiling) even when filtering is unused. Keep the old overload delegating `null`, as this change already does for `KafkaIndexTaskIOConfig` and `OrderedPartitionableRecord`.



##########
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] Update the default logging-emitter allowlist
   
   `LoggingEmitter` drops every service metric not present in its bundled `processing/src/main/resources/loggingEmitterAllowedMetrics.json` when `shouldFilterMetrics` is enabled without a custom allowlist. This new `ingest/events/filtered` metric is not in that resource, so the documented metric silently disappears for that supported configuration even though the other emitter defaults were updated. Add it to the bundled allowlist and cover the default-filtered path.



-- 
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@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org