[#1481] Update peekEvent accordingly #1484
Merged
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
Fix javadoc discrepancy stating that the SequentialPolicy is the default policy, whilst the SequentialPerAggregatePolicy is the defautl
The EventBuffer used by the AxonServerConnector as the TrackingEventStream currently contains a blocking loop, taking up to 500ms to exit. This so happens to correspond to the minimum wait time used by the hasNextAvailable method. This occurs because the peakEvent, validated in a do-while-loop, isn't updated when the lock's Condition is signaled. Subsequently, adjust the test to timeout if it takes longer dan 450ms, as that's a signal the loop occurs again whilst it shouldn't #1481
smcvb
added
Type: Bug
Add short trace logging showing whether we were signaled or waited 500ms #1481
The current solution introduces a form of side effect _why_ it no longer loops in the do-while-loop. Namely, subsequent invocations of the hasNextAvailable method currently introduce the scenario that the next entry from the Iterator is retrieved. This means, the following entry is removed from the delegate EventStream. Although this make sure an early return occurs, it still does an additional unnecessary loop. Instead, we should consistently use one way to validate an event is present, and omit the side effect inducing operation. As this means we'll no longer perform direct invocations of the EventStream which could lead to an exception, we should validate whether the stream is closed prior to proceeding. #1481
MGathier
approved these changes
Aug 10, 2020
For some, the createEventData method was to slow, hence causing the testDataUpcastAndDeserialized with the timeout to fail. Making the result a static field will resolve this #1481
abuijze
requested changes
Aug 13, 2020
...r-connector/src/main/java/org/axonframework/axonserver/connector/event/axon/EventBuffer.java
Outdated
Show resolved
Hide resolved
...r-connector/src/main/java/org/axonframework/axonserver/connector/event/axon/EventBuffer.java
Outdated
Show resolved
Hide resolved
...r-connector/src/main/java/org/axonframework/axonserver/connector/event/axon/EventBuffer.java
Outdated
Show resolved
Hide resolved
...r-connector/src/main/java/org/axonframework/axonserver/connector/event/axon/EventBuffer.java
Outdated
Show resolved
Hide resolved
abuijze
reviewed
Aug 13, 2020
...r-connector/src/main/java/org/axonframework/axonserver/connector/event/axon/EventBuffer.java
Outdated
Show resolved
Hide resolved
- Rename the constant - Add private peekNullable method and use instead of peek() / peekEvent == null - Pair two if-breaks - Switch do-while-loop to while-loop - Inline waitForData - Remove unintended debug log line #1481
- Reorder field assignment in constructor - Drop if-continue check, as it's no longer needed through the switch from a do-while-loop to the while-loop - Move waitTime creation and validation to occur just before the await invocation #1481
abuijze
reviewed
Aug 14, 2020
...r-connector/src/main/java/org/axonframework/axonserver/connector/event/axon/EventBuffer.java
Outdated
Show resolved
Hide resolved
Waiting on events should happen if there is no event AND the wait time is bigger than zero. #1481
Further reordering, for a different kind of OCD #1481
Move the EventStream#isClosed check to the peekNullable method. That way we can ensure we validate this operation everytime we try to retrieve a new event from the delegate stream. #1481
|
Kudos, SonarCloud Quality Gate passed!
|
abuijze
approved these changes
Aug 14, 2020
smcvb
added
Status: Resolved
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.
This pull request updates the
EventBuffer, to ensure it no longer loops for the defined 500ms "wait time" in thehasNextAvailablemethod. This loop was performed as the while-loop validated whether thepeekEvent == null, thus the event of the constructedIterator<TrackedEventMessage<?>> eventStream. However thewaitForDatamethod calls thedelegateEventStream#peekmethod. The latter returns true, causing the lock to be released, but the while-loop kept on going as it didn't change thepeekEvent.The javadoc adjustments of the
SequencingPolicywere added unintentionally, but don't hurt this PR.This PR resolves #1481