Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Throw TimeoutException when loading next even for aggregate takes too long #210

Merged
merged 6 commits into from
Jul 11, 2022
Merged

Throw TimeoutException when loading next even for aggregate takes too long #210

merged 6 commits into from
Jul 11, 2022

Conversation

CodeDrivenMitch
Copy link
Contributor

In some very special cases it seems the aggregate keeps loading indefinitely. This PR puts a timeout, by default 10 seconds, on waiting for the next event in the stream.
This timeout can be customized using the AGGREGATE_TAKE_EVENT_TIMEOUT_MILLIS system property.

@CodeDrivenMitch CodeDrivenMitch self-assigned this Jul 8, 2022
@smcvb smcvb added this to the Release 4.6.0 milestone Jul 8, 2022
smcvb
smcvb requested changes Jul 8, 2022
View reviewed changes
Copy link
Contributor

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

All nits, but 7 nits on a small PR to me constitutes in a request for changes.

src/main/java/io/axoniq/axonserver/connector/event/impl/BufferedAggregateEventStream.java Outdated Show resolved Hide resolved
src/main/java/io/axoniq/axonserver/connector/event/impl/BufferedAggregateEventStream.java Show resolved Hide resolved
src/main/java/io/axoniq/axonserver/connector/event/impl/BufferedAggregateEventStream.java Outdated Show resolved Hide resolved
src/main/java/io/axoniq/axonserver/connector/event/impl/BufferedAggregateEventStream.java Outdated
@@ -31,12 +34,15 @@
implements AggregateEventStream {

private static final Event TERMINAL_MESSAGE = Event.newBuilder().setAggregateSequenceNumber(-1729).build();
private static final int TIMEOUT_MILLIS = Integer.parseInt(System.getProperty("AGGREGATE_TAKE_EVENT_TIMEOUT_MILLIS",
"10000"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I wouldn't expect the 10000 default to be on a different line honestly. Differently put, indentation is a little off here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have a coding standard, so it's intellij default column width. Pain in the behind for stuff like this.

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 adjusted it as well I could

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should start a code style...I've got something in mind for stuff like this, but that's my OCD doing that; not a rule set.

src/main/java/io/axoniq/axonserver/connector/impl/FlowControlledBuffer.java Outdated
try {
return tryTake(timeout, timeUnit, false);
} catch (TimeoutException e) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thought: isn't it worth logging this scenario, perhaps just on debug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an impossible scenario, it's only there because java enforces the checked exceptions. The third parameter having false indicates it should never throw

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 changed the API, so the catch is no longer needed

src/main/java/io/axoniq/axonserver/connector/impl/FlowControlledBuffer.java Outdated Show resolved Hide resolved
src/test/java/io/axoniq/axonserver/connector/event/impl/BufferedAggregateEventStreamTest.java Show resolved Hide resolved
CodeDrivenMitch and others added 2 commits July 8, 2022 14:29
@sonarcloud
Copy link

sonarcloud bot commented Jul 8, 2022 •
edited

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

smcvb
smcvb approved these changes Jul 11, 2022
View reviewed changes
Copy link
Contributor

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@CodeDrivenMitch CodeDrivenMitch changed the base branch from master to connector-4.5.x July 11, 2022 09:04
@CodeDrivenMitch CodeDrivenMitch merged commit 9269897 into connector-4.5.x Jul 11, 2022
@CodeDrivenMitch CodeDrivenMitch deleted the feature/aggregate-event-timeout branch July 11, 2022 09:06
@smcvb smcvb modified the milestones: Release 4.6.0, Release 4.5.6 Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smcvb smcvb smcvb approved these changes

@saratry saratry Awaiting requested review from saratry

@abuijze abuijze Awaiting requested review from abuijze

Assignees

@CodeDrivenMitch CodeDrivenMitch

Labels
Priority 1: Must Status: Resolved Type: Bug
Projects
None yet
Milestone
Release 4.5.6
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@CodeDrivenMitch @abuijze @smcvb @saratry