mqtt-streaming: fix flaky 'duplicate publish given two client connections' test #456
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ions' test It appears the flakiness was caused by the two connections using the same connection id for different connections in the same session. I'm not intimately familiar with this codebase, but AFAICS it seems this connection id is an internal concern to the component, and not an MQTT concept. I think we should probably provide an API that does not bother the user with this concept at all, and manages non-overlapping connection ids entirely internal to the component. I don't get the impression that it was the purpose of the test to test the behaviour in case of overlapping connection ids. Fixes apache#454 After this change the 'unsubscribe a client session' seems to fail a lot on CI, but that seems unrelated and something to fix in a separate PR.
raboof
force-pushed
the
mqtt-streaming-fix-flaky-test
branch
2 times, most recently
from
def3f73 to
c8611a5
Compare
raboof
force-pushed
the
mqtt-streaming-fix-flaky-test
branch
from
e9bd5f3 to
c8611a5
Compare
This was referenced Feb 8, 2024
Closed
mdedetrich
approved these changes
Feb 8, 2024
There was a problem hiding this comment.
lgtm. I think that we should also backport this to 1.0.x since we will still be supporting these connectors @pjfanning wdyt?
raboof
added a commit
to raboof/incubator-pekko-connectors
that referenced
this pull request
Jul 7, 2024
While looking into apache#468, I noticed the two failing tests were sharing the same session id, which reminded of apache#456. While in this case the two tests aren't sharing the same session, and I haven't investigated the details of this codebase further, I'm curious to see if the problem remains when we use unique session ids.
raboof
added a commit
to raboof/incubator-pekko-connectors
that referenced
this pull request
Jul 7, 2024
While looking into apache#468, I noticed the two failing tests were sharing the same session id, which reminded of apache#456. While in this case the two tests aren't sharing the same session, and I haven't investigated the details of this codebase further, I'm curious to see if the problem remains when we use unique session ids.
raboof
added a commit
that referenced
this pull request
Jul 7, 2024
While looking into #468, I noticed the two failing tests were sharing the same session id, which reminded of #456. While in this case the two tests aren't sharing the same session, and I haven't investigated the details of this codebase further, I'm curious to see if the problem remains when we use unique session ids.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It appears the flakiness was caused by the two connections using the same connection id for different connections in the same session. I'm not intimately familiar with this codebase, but AFAICS it seems this connection id is an internal concern to the component, and not an MQTT concept. I think we should probably provide an API that does not bother the user with this concept at all, and manages non-overlapping connection ids entirely internal to the component - filed #457. I don't get the impression that it was the purpose of the test to test the behaviour in case of overlapping connection ids.
Fixes #454
This PR still fails due to #458 but #460 suggests it should be OK when combined with #459.
(note the diff is much simpler than it looks, use the 'Hide whitespace' under the cog in the diff view to ignore the indent change)