Skip to content

STORM-3693: Pending tuples expired only once per tick.#3338

Closed
Chandan83 wants to merge 1 commit into
apache:masterfrom
Chandan83:STORM-3693
Closed

STORM-3693: Pending tuples expired only once per tick.#3338
Chandan83 wants to merge 1 commit into
apache:masterfrom
Chandan83:STORM-3693

Conversation

@Chandan83

Copy link
Copy Markdown

What is the purpose of the change

(To ensure spout pending tuples are expired only once per tick. Otherwise, tuples get expired prematurely when one spout executor has multiple tasks.)

How was the change tested

(1. mvn clean install -P '!examples,!externals' 2. Used a dummy topology to test the correct behavior.)

@Chandan83

Copy link
Copy Markdown
Author

@nathanmarz I am wondering if I can do something more to garner some attention to this PR. The fix seems to address a fundamental bug related to tuple expiry.

@rzo1

rzo1 commented Dec 17, 2024

Copy link
Copy Markdown
Contributor

@reiabreu wdyt about this?

@reiabreu

Copy link
Copy Markdown
Contributor

@rzo1 I'm not familiar with this code. Let me have a look and report back.

@reiabreu

Copy link
Copy Markdown
Contributor

@rzo1 @Chandan83
Had a look at the PR and it seems to be something we should fix. I would be more comfortable with some unit tests covering this, but we seem to be lacking specific tests for SpoutExecutor.

@reiabreu

reiabreu commented Jan 3, 2025

Copy link
Copy Markdown
Contributor

@rzo1 since the source branch is quite outdated, I can create a new PR with the diff applied to it and check the test pipelines.

@rzo1

rzo1 commented Jan 3, 2025

Copy link
Copy Markdown
Contributor

Let's do that. Thanks @reiabreu

@reiabreu

Copy link
Copy Markdown
Contributor

I did not find the time for this yet, but I plan on opening a PR for this over the coming days. Cheers

@reiabreu

Copy link
Copy Markdown
Contributor

Changes were moved to #3828 to have Github Actions validate the changes.
Thanks for the PR @Chandan83 !

@reiabreu reiabreu closed this Jan 13, 2025
@Chandan83

Chandan83 commented Jan 13, 2025 via email

Copy link
Copy Markdown
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants