refactor(cohorts): isolate person_merge_events topic producer - #69529
Conversation
…/produce-isolation-hardening
…isolation-hardening
haacked
left a comment
There was a problem hiding this comment.
Nice fix! I was late to this one, but some comments for possible follow-up?
| * topic field is required so every caller must plumb it through, turning a mis-wired config into a | ||
| * build error rather than a silent self-disable. | ||
| */ | ||
| export function effectivePersonMergeEventsEnabled(config: { |
There was a problem hiding this comment.
suggestion: No test covers effectivePersonMergeEventsEnabled, the on/off gate for the whole feature. person-merge-event.test.ts only tests buildPersonMergeEventMessage, so a regression here (the feature silently disabling itself, or enabled with an unwired topic that swallows produce failures) ships uncaught.
Add the test cases below. The topic-empty case needs jest.isolateModules (or an exported reset) around warnedUnconfiguredMergeEventsTopic, since that module-level flag never resets and test order would otherwise decide whether the assertion passes.
describe('effectivePersonMergeEventsEnabled', () => {
it('returns false when the gate is off, regardless of topic', () => {
expect(effectivePersonMergeEventsEnabled({ PERSON_MERGE_EVENTS_ENABLED: false, INGESTION_OUTPUT_PERSON_MERGE_EVENTS_TOPIC: 'topic' })).toBe(false)
})
it('returns true when the gate is on and the topic is configured', () => {
expect(effectivePersonMergeEventsEnabled({ PERSON_MERGE_EVENTS_ENABLED: true, INGESTION_OUTPUT_PERSON_MERGE_EVENTS_TOPIC: 'topic' })).toBe(true)
})
it('returns false when the gate is on but the topic is unset', () => {
expect(effectivePersonMergeEventsEnabled({ PERSON_MERGE_EVENTS_ENABLED: true, INGESTION_OUTPUT_PERSON_MERGE_EVENTS_TOPIC: '' })).toBe(false)
})
})| * accepted until the delivery-guarantees milestone. The message is explicitly partitioned by | ||
| * `(team_id, P_old)` so it reaches the worker holding P_old's state — see `buildPersonMergeEventMessage`. | ||
| */ | ||
| async producePersonMergeEvent(sourcePerson: InternalPerson, targetPerson: InternalPerson): Promise<void> { |
There was a problem hiding this comment.
nit: producePersonMergeEvent now keeps the full sourcePerson/targetPerson objects alive for the entire produce, since it's detached from kafkaAck; buildPersonMergeEventMessage and the catch-block log only need the two UUIDs. During a broker backlog, produces can stay pending for a while (bounded by message timeout and the shared queue buffer), so this keeps that many full InternalPerson objects, including properties, in memory instead of just UUID strings.
const sourceUuid = sourcePerson.uuid
const targetUuid = targetPerson.uuid
try {
const { key, partition, value } = buildPersonMergeEventMessage(
this.team.id,
sourceUuid,
targetUuid,
Date.now(),
this.mergeEventsConfig.partitionCount
)
...
} catch (error) {
logger.warn('person_merge_events produce failed, dropping', {
team_id: this.team.id,
source_person_uuid: sourceUuid,
target_person_uuid: targetUuid,
error,
})
}| // Kill switch for emitting person_merge_events to the cohort-stream-processor. | ||
| // Enable ordering: (1) create the topic, (2) set INGESTION_OUTPUT_PERSON_MERGE_EVENTS_TOPIC | ||
| // (startup topic verification is then fatal by design), (3) flip this on. Flipping this on before | ||
| // the topic env is set is a no-op — see effectivePersonMergeEventsEnabled. |
There was a problem hiding this comment.
question: Before flipping this gate on, does INGESTION_OUTPUT_PERSON_MERGE_EVENTS_PRODUCER resolve to the same producer instance as the person-message producers? If it does, detaching the merge produce from kafkaAck removes the backpressure that used to slow down merges, letting uncapped merge operations compete with critical person messages for the same queue.buffering.max.messages buffer during a broker slowdown. Pointing the shadow topic at its own producer slot would keep the isolation this PR is going for.
Problem
person_merge_eventsis a shadow topic consumed only by the cohort stream processor, yet its producer was bundled into the samekafkaAckPromise.allas the critical person messages, so a failed or slow shadow produce could crash or stall analytics ingestion.Before flipping the producer gate on, producing to it must not be able to affect any service that does not consume it.
Changes
Stacked on #69514.
producePersonMergeEvent(person-context.ts) is now best effort.It wraps message building and the produce in try/catch, never throws, logs and drops on failure, and counts only broker acked produces.
The produce is detached from the ack chain (
person-merge-service.ts).kafkaAcknow carries only the person messages and the merge event is fire and forget, so a shadow produce failure or slowness can no longer reject or delay offset commits.A new
effectivePersonMergeEventsEnabledhelper (person-merge-event.ts) also requires a configured output topic, so flipping the flag on before wiring the topic env silently does nothing rather than warning on every gated merge.Delivery stays at most once. Tightening it is deferred to the actual cut over from the shadow mode.
How did you test this code?
person-context.test.tscase where a rejecting produce still resolves and does not increment the counter, pinning the never throws contract.person-state-batch.test.tscase that hangs the shadow produce and assertskafkaAckstill resolves.kafkaAckbundled back together, so a rejecting produce would not catch the regression.Docs update
No.