Skip to content

KAFKA-14834: [2/N] Test coverage for out-of-order data in joins#13497

Merged
mjsax merged 3 commits into
apache:trunkfrom
vcrfxia:kip-914-test-coverage
Apr 12, 2023
Merged

KAFKA-14834: [2/N] Test coverage for out-of-order data in joins#13497
mjsax merged 3 commits into
apache:trunkfrom
vcrfxia:kip-914-test-coverage

Conversation

@vcrfxia

@vcrfxia vcrfxia commented Apr 3, 2023

Copy link
Copy Markdown
Contributor

In preparation for updating DSL join processors to have updated semantics when versioned stores are used (cf KIP-914), this PR adds test coverage for out-of-order data in joins to the existing integration tests for stream-table joins and primary-key table-table joins. Follow-up PRs will build on top of this change by adding new tests for versioned stores, and the out-of-order data will produce different results in those settings.

Committer Checklist (excluded from commit message)

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

STREAMS_CONFIG.put(StreamsConfig.APPLICATION_ID_CONFIG, appID + "-multi-inner");

final List<List<TestRecord<Long, String>>> expectedResult = Arrays.asList(
@SuppressWarnings("RedundantTypeArguments (explicit type arguments speedup compilation and analysis time)") final List<List<TestRecord<Long, String>>> expectedResult = Arrays.asList(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For my own education: why do we need this, and why do we add the generic types below?

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.

Hmm.. I'm not sure where that suppression came from. Probably an accident, I will remove it.

The reason the explicit types are added below is because I was getting an error that type inference slows down if there are too many variadic arguments passed to a method with a generic that requires type inference. I forget if it was just my IDE complaining or if spotbugs was failing too but I figure adding the types in doesn't introduce any harm.

new Input<>(INPUT_TOPIC_RIGHT, null, 13),
new Input<>(INPUT_TOPIC_RIGHT, "d", 14),
new Input<>(INPUT_TOPIC_LEFT, "D", 15),
new Input<>(INPUT_TOPIC_LEFT, "E", 4), // out-of-order data

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems we only need this for table-table join tests, but it only blows up stream-stream join results? I am ok to merge as is, but seems it's adding "some noise" the stream-stream-join case (not too bad for binary joins, but for 3-way join it's a lot).

Not sure if we could have different inputs for stream vs table case?

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.

Yeah, I noticed this as well. If you don't think having the out-of-order data in stream-stream join tests is valuable, I can open a follow-up PR to remove this (to reduce noise). Just LMK.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't think it does really harm, but it makes it very hard to actually read the test.

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.

Fixed in #13609.

@mjsax mjsax added the kip Requires or implements a KIP label Apr 12, 2023
@mjsax
mjsax merged commit 17b4569 into apache:trunk Apr 12, 2023
@vcrfxia
vcrfxia deleted the kip-914-test-coverage branch April 12, 2023 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kip Requires or implements a KIP streams tests Test fixes (including flaky tests)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants