Skip to content
This repository has been archived by the owner on Jun 7, 2024. It is now read-only.

ARUHA-1604: added flushing collected events when reaching stream timeout #860

Merged
merged 5 commits into from
Apr 26, 2018

Conversation

v-stepanov
Copy link
Contributor

@v-stepanov v-stepanov commented Apr 12, 2018

Added flushing collected events when reaching stream timeout in subscription API

Zalando ticket : ARUHA-1604

Description

When stream_timeout is reached user wants to get events which were collected so far.

Review

  • Tests
  • Documentation
  • CHANGELOG

Deployment Notes

We should keep an eye on complaining users

@codecov-io
Copy link

codecov-io commented Apr 12, 2018

Codecov Report

Merging #860 into master will increase coverage by 0.01%.
The diff coverage is 40%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #860      +/-   ##
============================================
+ Coverage     53.56%   53.57%   +0.01%     
- Complexity     1691     1694       +3     
============================================
  Files           309      309              
  Lines          9359     9366       +7     
  Branches        841      843       +2     
============================================
+ Hits           5013     5018       +5     
- Misses         4046     4049       +3     
+ Partials        300      299       -1
Impacted Files Coverage Δ Complexity Δ
...adi/service/subscription/state/StreamingState.java 35.09% <0%> (-0.28%) 35 <0> (ø)
...kadi/service/subscription/state/PartitionData.java 78.57% <100%> (+2.32%) 23 <0> (+3) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46f9610...53a25fe. Read the comment docs.

@antban
Copy link
Contributor

antban commented Apr 13, 2018

👍

@v-stepanov
Copy link
Contributor Author

deploy validation please

3 similar comments
@v-stepanov
Copy link
Contributor Author

deploy validation please

@adyach
Copy link
Member

adyach commented Apr 16, 2018

deploy validation please

@adyach
Copy link
Member

adyach commented Apr 16, 2018

deploy validation please

@@ -101,6 +101,23 @@ public void whenStreamTimeoutReachedPossibleToCommit() throws Exception {
Assert.assertEquals(SC_NO_CONTENT, statusCode);
}

@Test(timeout = 10000)
public void whenStreamTimeoutReachedThenEventsFlushed() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested streaming using this same parameter in the old stack and the new one and both behave the same way. Both got 2 batches, one containing 3 events and the other with debug message. Does this test actually exercise or prove that the fix works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember that I was running this test against the old version and it failed. I will check it again now.

Copy link
Contributor Author

@v-stepanov v-stepanov Apr 24, 2018

Choose a reason for hiding this comment

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

Oh yes, sorry, I remembered now. I changed the test later and it actually now works for old version as well (as it reaches max_uncommitted_events)
I now changed the test. It will now fail in old version and will work for new version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rcillo
Copy link
Contributor

rcillo commented Apr 25, 2018

👍

…-stream-timeout

# Conflicts:
#	CHANGELOG.md
@antban
Copy link
Contributor

antban commented Apr 26, 2018

👍

1 similar comment
@v-stepanov
Copy link
Contributor Author

👍

@v-stepanov v-stepanov merged commit 87bf767 into master Apr 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants