-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Emit tombstones for deleted documents #88
Conversation
fca5481
to
ce13258
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor doc suggestions and it would be nice to have a test please.
Schema.STRING_SCHEMA, // key schema | ||
id, // key | ||
docSchema, // value schema | ||
docValue); // value | ||
records.add(sourceRecord); | ||
if (doc.isDeleted() != null && doc.isDeleted()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not specific to this line, but I think it would be worth adding a test for the tombstone to protect against regressing it.
f090c7d
to
c42052e
Compare
TODO - tests need cleaning up Fixes #73
c42052e
to
91b0902
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 couple of nits copyright headers/todo comment
expect(mockResult.getLastSeq()).andReturn("100"); | ||
expect(changesResultItem.getChanges()).andReturn(Collections.singletonList(change)); | ||
expect(changesResultItem.getDoc()).andReturn(document); | ||
expect(changesResultItem.getId()).andReturn(id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was surprised to not see ChangesResultItem
's isDeleted
mocked out here. Of course checking the implementation again I see we are checking the field on the doc instead.
I think the implementation is fine as we have the doc anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - the test fails if I mock isDeleted
but don't set deleted on the document itself.
Assert.assertEquals(2, records.size()); | ||
Assert.assertNotNull(records.get(0).value()); | ||
Assert.assertNull(records.get(1).value()); | ||
// TODO unpack record json and assert on it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this TODO is necessary, asserting the record value is null
is sufficient to know it is a tombstone.
Arguably we could have a second test that validates that we don't get a second record when the change wasn't a deleted doc, but I expect assertions elsewhere are covering that.
@@ -0,0 +1,100 @@ | |||
package com.ibm.cloud.cloudant.kafka.connect; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs the copyright header
@@ -0,0 +1,10 @@ | |||
package com.ibm.cloud.cloudant.kafka.connect; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs the copyright header
Documentation
Update README and CHANGES
Testing
Tested locally by deleting document from Cloudant and observing event with
./bin/kafka-console-consumer.sh --bootstrap-server localhost:9092 --topic kafka_test --from-beginning --property print.key=true