Adding SegmentMetadataEvent and publishing them via KafkaEmitter

[harinirajendran]

Details

Adding the new SegmentMetadataEvent and publishing these segment-related metadata events into Kafka by enhancing the KafkaEmitter

Description

In this PR, we are enhancing KafkaEmitter, to emit metadata about published segments (SegmentMetadataEvent) into a Kafka topic. This segment metadata information that gets published into Kafka, can be used by any other downstream services to query Druid intelligently based on the segments published. The segment metadata gets published into kafka topic in json string format similar to other events.

Old behavior of Kafka Emitter

Kafka Emitter always emits metrics and alerts and would emit requests if the config request.topic is configured.
Configs metric.topic and alert.topic are always mandatory and cannot be null.

Current behavior of Kafka Emitter [with backwards compatibility]

We introduced a new config named event.types which dictates the types of events we want the KafkaEmitter to emit. This config takes in a list of strings and can have one or more from [alerts, metrics, requests and segmentMetadata]. And based on this config, alert.topic, request.topic, metric.topic and segmentMetadata.topic should be configured and not left empty.
If no event.types is set, then by default, the kafka emitter would emit metrics and alerts. And in that case, to maintain backwards compatibility, decision to send out requests are based on if request.topic is empty or set.

This PR has:

• been self-reviewed.
  • using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
• added documentation for new or modified features or behaviors.
• a release note entry in the PR description.
• added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
• added or updated version, license, or notice information in licenses.yaml
• added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
• added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
• added integration tests.
• been tested in a test Druid cluster.

[harinirajendran]

@nishantmonu51 : I have added a new event type in this PR that would cause the following exception to be thrown in AmbariMetricsEmitter and DropWizardEmitter

throw new ISE("unknown event type [%s]", event.getClass());

Should I explicitly filter this new event type from these emitters?

[gianm]

Should I explicitly filter this new event type from these emitters?

IMO it would make more sense to edit those emitters to ignore unknown event types.

Btw, this may be OK for your use case, but, I wanted to point out that there is a race here: it's possible for the segments to be committed and for the server to crash before it emits this event. So, some might get missed.

[harinirajendran]

IMO it would make more sense to edit those emitters to ignore unknown event types.

That makes sense @gianm. I'll update those emitters to ignore the new event type.

[harinirajendran]

Btw, this may be OK for your use case, but, I wanted to point out that there is a race here: it's possible for the segments to be committed and for the server to crash before it emits this event. So, some might get missed.

That's a great point @gianm. Do you have a recommendation for a better place to emit this segment metadata event instead of this place to prevent this?

[gianm]

That's a great point @gianm. Do you have a recommendation for a better place to emit this segment metadata event instead of this place to prevent this?

I think for it to be "perfect" the best way to do it would be to emit in the place you emit here, but also have some other process that detects missed emits somehow and fixes them up by redoing the missed emits. This would be a lot more complex of an implementation, however. So I'd only recommend doing that if it seems worth it.

To figure that out, I would consider the requirements here. What kind of things are likely to consume the emitted payloads? Could they tolerate either of the following conditions?

• missed emits (segments that are published, but never emitted)
• bogus emits (segments that are never published, but were emitted anyway)

If one or both of these can be tolerated, the implementation becomes a lot simpler.

If I understand correctly— the one you have in this PR is the "missed emit" scenario. It won't generate bogus emits, but it can potentially miss some.

[harinirajendran]

I think for it to be "perfect" the best way to do it would be to emit in the place you emit here, but also have some other process that detects missed emits somehow and fixes them up by redoing the missed emits. This would be a lot more complex of an implementation, however. So I'd only recommend doing that if it seems worth it.

I think for now we can just go with this implement and enhance it to emit missed segments metadata in the future.

[ektravel]

Docs: left some questions/suggestions.

[harinirajendran]

@harinirajendran can we update the PR description to reflect the current state of the code. It still references the protobuf implementation which is no longer part of this.

Updated it @xvrl . will address other comments now

[harinirajendran]

Addressed review comments.

[harinirajendran]

I was following the same strategy that was followed for request events earlier

          if (config.getRequestTopic() == null || !requestQueue.offer(objectContainer)) {
            requestLost.incrementAndGet();
          }

[harinirajendran]

Reference: https://github.com/apache/druid/blob/24.0.2/extensions-contrib/kafka-emitter/src/main/java/org/apache/druid/emitter/kafka/KafkaEmitter.java#L195

[harinirajendran]

@xvrl @abhishekagarwal87 I ran org.apache.druid.java.util.emitter.core.HttpEmitterConfigTest.testDefaultsLegacy test locally in my laptop and it passed. But it failed twice when we trigger it as a part of this PR. What do we do in such cases?

[abhishekagarwal87]

@harinirajendran - We will have to fix it. When you run locally, are you running just one test? I will suggest running the same maven command that github action is running. I think that you will run into the same error then.

[harinirajendran]

@harinirajendran - We will have to fix it. When you run locally, are you running just one test? I will suggest running the same maven command that github action is running. I think that you will run into the same error then.

Yeah I was just running that one test. let me try using the same command and see if I can reproduce it. Thanks @abhishekagarwal87

[abhishekagarwal87]

@harinirajendran - You should avoid force-push in the future as we can't see the diff from the last commit anymore. can you describe the most recent change?

[harinirajendran]

@harinirajendran - You should avoid force-push in the future as we can't see the diff from the last commit anymore. can you describe the most recent change?

Ahh okay! Wasn't aware of that. Will keep that in mind for future PRs. I did not make any code changes. Just rebased the code on top of the latest master and pushed it again.