Skip to content

KAFKA-15179: Add integration tests for the file sink and source connectors#14279

Merged
C0urante merged 9 commits into
apache:trunkfrom
yashmayya:KAFKA-15179-file-stream-integration-tests
Sep 7, 2023
Merged

KAFKA-15179: Add integration tests for the file sink and source connectors#14279
C0urante merged 9 commits into
apache:trunkfrom
yashmayya:KAFKA-15179-file-stream-integration-tests

Conversation

@yashmayya

Copy link
Copy Markdown
Contributor

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@yashmayya yashmayya added connect tests Test fixes (including flaky tests) labels Aug 23, 2023
@yashmayya

Copy link
Copy Markdown
Contributor Author

@C0urante would you be able to take a look at this sometime (for context - we'd discussed this earlier here)? There's no hurry at all though, since this isn't very high priority.

@C0urante C0urante left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Yash! Looking pretty good, had a few thoughts. Hoping most should be fairly straightforward

@yashmayya yashmayya left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review Chris!

* @param numLines the expected number of lines in the file
* @param verifyLinearity true if the line contents are to be verified
*/
private void verifyLinesInFile(Path filePath, int numLines, boolean verifyLinearity) throws Exception {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @yashmayya - we could also check the number of times a message is expected to be present in the file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I intentionally kept the check looser to make it a little more generic and allow for covering all the scenarios. In the alter offsets case for instance, only the last message would be written twice and all the others would only be written once.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the checks here, while fairly loose, are sufficient. We have pretty granular unit testing coverage for the sink and source task classes. It's not impossible to add the same level of granularity to these tests but it'd be fairly complex to do it right (i.e., handle offset resets and consumption from multiple topics). I'm fine with erring on the side of simplicity for now and, if necessary, hardening these tests in the future.

@C0urante C0urante left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Yash, LGTM!

@C0urante
C0urante merged commit 88b554f into apache:trunk Sep 7, 2023
@yashmayya

Copy link
Copy Markdown
Contributor Author

Thanks Chris!

AnatolyPopov pushed a commit to aiven/kafka that referenced this pull request Feb 16, 2024
…ctors (apache#14279)

Reviewers: Ashwin Pankaj <apankaj@confluent.io>, Chris Egerton <chrise@aiven.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

connect tests Test fixes (including flaky tests)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants