Make Ringbuffer not throw StaleSequenceException on read-many operation #16303
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
|
readManyAsync javadoc would need updating also. |
mmedenjak
reviewed
Dec 20, 2019
hazelcast/src/main/java/com/hazelcast/topic/impl/reliable/MessageRunner.java
Outdated
Show resolved
Hide resolved
cangencer
force-pushed
the
ringbuffer_improvement
branch
5 times, most recently
from
b731826
to
3c8aa0c
Compare
|
run-lab-run |
mmedenjak
approved these changes
Dec 24, 2019
hazelcast/src/main/java/com/hazelcast/topic/impl/reliable/MessageRunner.java
Outdated
Show resolved
Hide resolved
hazelcast/src/main/java/com/hazelcast/topic/impl/reliable/MessageRunner.java
Outdated
Show resolved
Hide resolved
hazelcast/src/main/java/com/hazelcast/ringbuffer/impl/operations/ReadManyOperation.java
Show resolved
Hide resolved
hazelcast/src/main/java/com/hazelcast/ringbuffer/impl/operations/ReadManyOperation.java
Show resolved
Hide resolved
cangencer
reviewed
Dec 25, 2019
hazelcast/src/main/java/com/hazelcast/topic/impl/reliable/MessageRunner.java
Show resolved
Hide resolved
|
@jbartok could you add description of what this fixes? |
ihsandemir
reviewed
Dec 26, 2019
hazelcast/src/main/java/com/hazelcast/ringbuffer/impl/operations/ReadManyOperation.java
Outdated
Show resolved
Hide resolved
hazelcast/src/main/java/com/hazelcast/ringbuffer/impl/operations/ReadManyOperation.java
Show resolved
Hide resolved
hazelcast/src/main/java/com/hazelcast/topic/impl/reliable/MessageRunner.java
Show resolved
Hide resolved
hazelcast/src/test/java/com/hazelcast/client/ringbuffer/RingbufferTest.java
Outdated
Show resolved
Hide resolved
hazelcast/src/test/java/com/hazelcast/client/ringbuffer/RingbufferTest.java
Outdated
Show resolved
Hide resolved
Added to PR description. |
added 2 commits
December 27, 2019 10:13
|
@ihsandemir are all review comments addressed? |
ihsandemir
approved these changes
Jan 7, 2020
|
Thank you for the PR and the reviews, everyone! |
mmedenjak
pushed a commit
to mmedenjak/hazelcast
that referenced
this pull request
Jan 8, 2020
Because of the behaviour change introduced in hazelcast#16303, when the requested sequence is larger than the largest sequence (tailSequence) + 1, we don't listen from the oldest sequence (headSequence) but rather from the tailSequence + 1. Both approaches are fine and both approaches work better in some scenarios. Since the listener is loss tolerant, we can skip items from headSequence..tailSequence+1 anyway. Fixed the test to adhere to the new behaviour. We assume that eventually as we publish an item, it will reach the listener. A better fix would be to introduce unique IDs per ringbuffer, where we would then be able to distinguish between a completely lost ringbuffer and a ringbuffer which has not received the last few items and appropriately reset the requested sequence to the headSequence or tailSequence. Fixes: hazelcast#16430
Merged
mmedenjak
pushed a commit
to mmedenjak/hazelcast
that referenced
this pull request
Jan 9, 2020
Because of the behaviour change introduced in hazelcast#16303, when the requested sequence is larger than the largest sequence (tailSequence) + 1, we don't listen from the oldest sequence (headSequence) but rather from the tailSequence + 1. Both approaches are fine and both approaches work better in some scenarios. Since the listener is loss tolerant, we can skip items from headSequence..tailSequence+1 anyway. Fixed the test to adhere to the new behaviour. We assume that eventually as we publish an item, it will reach the listener. A better fix would be to introduce unique IDs per ringbuffer, where we would then be able to distinguish between a completely lost ringbuffer and a ringbuffer which has not received the last few items and appropriately reset the requested sequence to the headSequence or tailSequence. Fixes: hazelcast#16430
mmedenjak
added a commit
that referenced
this pull request
Jan 9, 2020
Fix LossToleranceTest Because of the behaviour change introduced in #16303, when the requested sequence is larger than the largest sequence (tailSequence) + 1, we don't listen from the oldest sequence (headSequence) but rather from the tailSequence + 1. Both approaches are fine and both approaches work better in some scenarios. Since the listener is loss tolerant, we can skip items from headSequence..tailSequence+1 anyway. Fixed the test to adhere to the new behaviour. We assume that eventually as we publish an item, it will reach the listener. A better fix would be to introduce unique IDs per ringbuffer, where we would then be able to distinguish between a completely lost ringbuffer and a ringbuffer which has not received the last few items and appropriately reset the requested sequence to the headSequence or tailSequence. Fixes: #16430
frant-hartm
added a commit
to frant-hartm/hazelcast
that referenced
this pull request
Oct 15, 2021
Fixes hazelcast#19696 ReliableTopicDestroyTest.whenDestroyedThenRingbufferRemoved Created a simpler reproducer RingbufferDestroyTest.whenDestroyAfterAdd_thenRingbufferRemoved The cause was recreation of the RingbufferContainer in ReadOneOperation.getWaitKey This also addresses review comment from hazelcast#19630. Fixes hazelcast#16469 RingbufferAddAllReadManyStressTest.whenShortTTLAndBigBuffer The stress test is incorrect, the ReadManyOperation doesn't throw the StaleSequenceException when head is stale and the items are just missing in the result. This was introduced in hazelcast#16303. Also fixed HashMap->ConcurrentHashMap in RingbufferService. This Map is modified from operation thread when RingbufferContainer is created and also from destroyContainer, which may run directly on the user's thread when the ringbuffer is local on the member.
4 tasks
frant-hartm
added a commit
to frant-hartm/hazelcast
that referenced
this pull request
Oct 15, 2021
Fixes hazelcast#19696 ReliableTopicDestroyTest.whenDestroyedThenRingbufferRemoved Created a simpler reproducer RingbufferDestroyTest.whenDestroyAfterAdd_thenRingbufferRemoved The cause was recreation of the RingbufferContainer in ReadOneOperation.getWaitKey This also addresses review comment from hazelcast#19630. Fixes hazelcast#16469 RingbufferAddAllReadManyStressTest.whenShortTTLAndBigBuffer The stress test is incorrect, the ReadManyOperation doesn't throw the StaleSequenceException when head is stale and the items are just missing in the result. This was introduced in hazelcast#16303. Also fixed HashMap->ConcurrentHashMap in RingbufferService. This Map is modified from operation thread when RingbufferContainer is created and also from destroyContainer, which may run directly on the user's thread when the ringbuffer is local on the member.
frant-hartm
added a commit
to frant-hartm/hazelcast
that referenced
this pull request
Oct 18, 2021
Fixes hazelcast#19696 ReliableTopicDestroyTest.whenDestroyedThenRingbufferRemoved Created a simpler reproducer RingbufferDestroyTest.whenDestroyAfterAdd_thenRingbufferRemoved The cause was recreation of the RingbufferContainer in ReadOneOperation.getWaitKey This also addresses review comment from hazelcast#19630. Fixes hazelcast#16469 RingbufferAddAllReadManyStressTest.whenShortTTLAndBigBuffer The stress test is incorrect, the ReadManyOperation doesn't throw the StaleSequenceException when head is stale and the items are just missing in the result. This was introduced in hazelcast#16303. Also fixed HashMap->ConcurrentHashMap in RingbufferService. This Map is modified from operation thread when RingbufferContainer is created and also from destroyContainer, which may run directly on the user's thread when the ringbuffer is local on the member.
frant-hartm
added a commit
that referenced
this pull request
Oct 22, 2021
Fixes #19696 ReliableTopicDestroyTest.whenDestroyedThenRingbufferRemoved Created a simpler reproducer RingbufferDestroyTest.whenDestroyAfterAdd_thenRingbufferRemoved The cause was recreation of the RingbufferContainer in ReadOneOperation.getWaitKey This also addresses review comment from #19630. Fixes #16469 RingbufferAddAllReadManyStressTest.whenShortTTLAndBigBuffer The stress test is incorrect, the ReadManyOperation doesn't throw the StaleSequenceException when head is stale and the items are just missing in the result. This was introduced in #16303. Also fixed HashMap->ConcurrentHashMap in RingbufferService. This Map is modified from operation thread when RingbufferContainer is created and also from destroyContainer, which may run directly on the user's thread when the ringbuffer is local on the member.
frant-hartm
added a commit
to frant-hartm/hazelcast
that referenced
this pull request
Oct 22, 2021
Fixes hazelcast#19696 ReliableTopicDestroyTest.whenDestroyedThenRingbufferRemoved Created a simpler reproducer RingbufferDestroyTest.whenDestroyAfterAdd_thenRingbufferRemoved The cause was recreation of the RingbufferContainer in ReadOneOperation.getWaitKey This also addresses review comment from hazelcast#19630. Fixes hazelcast#16469 RingbufferAddAllReadManyStressTest.whenShortTTLAndBigBuffer The stress test is incorrect, the ReadManyOperation doesn't throw the StaleSequenceException when head is stale and the items are just missing in the result. This was introduced in hazelcast#16303. Also fixed HashMap->ConcurrentHashMap in RingbufferService. This Map is modified from operation thread when RingbufferContainer is created and also from destroyContainer, which may run directly on the user's thread when the ringbuffer is local on the member. Backport of hazelcast#19788
frant-hartm
added a commit
that referenced
this pull request
Oct 25, 2021
* Fix Ringbuffer test failures [5.0.z] Fixes #19696 ReliableTopicDestroyTest.whenDestroyedThenRingbufferRemoved Created a simpler reproducer RingbufferDestroyTest.whenDestroyAfterAdd_thenRingbufferRemoved The cause was recreation of the RingbufferContainer in ReadOneOperation.getWaitKey This also addresses review comment from #19630. Fixes #16469 RingbufferAddAllReadManyStressTest.whenShortTTLAndBigBuffer The stress test is incorrect, the ReadManyOperation doesn't throw the StaleSequenceException when head is stale and the items are just missing in the result. This was introduced in #16303. Also fixed HashMap->ConcurrentHashMap in RingbufferService. This Map is modified from operation thread when RingbufferContainer is created and also from destroyContainer, which may run directly on the user's thread when the ringbuffer is local on the member. Backport of #19788
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.
The problem we want to fix is that when producers are fast, consumers can have a really hard time keeping- and catching up with them (they often fail to do so, even when they would be able to process fast enough). For example if your head is at 1, and you ask for 100 items from 0, the
Ringbuffer.readManyAsynccall just throws an exception, instead of returning what’s available. By the time the exception travels to the consumer, gets processed and a new request is made, the requested sequence number tends to be stale again and will result in yet another exception. Meanwhile data is being overwritten in theRingbufferand is lost to the consumer.The solution aims to remove the
StaleSequenceExceptionbeing thrown by theReadManyOperationin such situations and just have it return the data that's available.We are able to do this for
Ringbuffer.readManyAsync, because it returns aReadResultSetcarrying sequence numbers, thus allowing the client to notice potential sequence gaps and decide if it can tolerate them or not. On the other had we can't do the same forRingbuffer.readOnebecause there is no way to observe sequence numbers in that case (it returns just an element from theRingbuffer).