rzepinskip commented on PR #11857: URL: https://github.com/apache/nifi/pull/11857#issuecomment-6465722588
@kotwal-itpro Thanks for your input. Let me try to answer your points: > 1. **Verification counts tombstones as parsed records.** In `verifyCanParse`, `recordIndex++` runs before the tombstone check, so a sample of one tombstone reports "Successfully parsed 1 records (skipped 1 tombstones)" (`testVerifySucceedsWhenSampleContainsOnlyTombstones` asserts this). Nothing was parsed in that case. Counting parsed records separately would give "parsed 0", and an all-tombstone sample could return SKIPPED like an empty sample does. `recordIndex` can stay as the position for the "Failed to parse Record number N" message. Fixed, thanks for pointing this out! > 2. **FLOW_FILE and DEMARCATOR still mark a non-null empty value as a tombstone.** `testFlowFile` expects `kafka.tombstone=true` on the empty value at offset 0, and `testDemarcatorEmpty` does the same. `FlowFileStreamKafkaMessageConverter` isn't changed, so it still infers a tombstone from `value.length == 0`. Now that `ByteRecord.isTombstone()` exists, these strategies could use it, which is what the issue asks for. If keeping the attribute for empty values is deliberate, for backward compatibility, it would help to say so in the `@WritesAttribute` description, since the two cases are now distinguishable. This ticket and PR focuses on RECORD strategy. For FLOW_FILE I filed a bug ticket: https://issues.apache.org/jira/browse/NIFI-16517. Should be a straightforward fix after this PR gets merged. For DEMARCATOR strategy I think we need more complex solution - added it as part of https://issues.apache.org/jira/browse/NIFI-16521. > 3. **USE_WRAPPER drops tombstones.** With RECORD + USE_WRAPPER, a tombstone is now skipped rather than emitted as a wrapper record with a null value. PublishKafka already treats a null wrapper value as a tombstone, so a ConsumeKafka → PublishKafka flow (for example mirroring a compacted topic) would lose the deletes. Skipping fixes the parse failures, but a maintainer may prefer the null-value wrapper record for USE_WRAPPER. Could be a follow-up. 1. Preserving tombstones in RECORD processing strategy would change current behavior for success relationship. 2. The solution to preserve tombstones will require complex changes and should be consistent with other strategies hence I see it as follow up and not a bug fix this PR is about. 3. "PublishKafka already treats a null wrapper value as a tombstone, so a ConsumeKafka → PublishKafka flow (for example mirroring a compacted topic) would lose the deletes." - this currently only works for FLOW_FILE strategy so not applicable for RECORD strategy. Given above, I filed https://issues.apache.org/jira/browse/NIFI-16521 to track work needed to preserve tombstones in RECORD strategy. > 4. Minor wording in the `@WritesAttribute` description: "Processing Strategy RECORD skip tombstone" → "With Processing Strategy RECORD, tombstones are skipped, so this attribute is not written." Moved the caveats to `additionalDetails.md` file to avoid polluting description with too many details. -- 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]
