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.

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

Optimize aggregate read operation #892

Merged
merged 16 commits into from
Dec 11, 2018

Conversation

dmdashenkov
Copy link
Contributor

@dmdashenkov dmdashenkov commented Nov 26, 2018

In this PR we make the AggregateRepository read the required amount of history events, even if this number is greater than the snapshot trigger.

Previously, the number of read events was always equal to the snapshot trigger.

Also, now we cache the eventCountAfterLastSnapshot in the Aggregate instance. This helps us not to re-fetch the value when appending the aggregate history.

@dmdashenkov dmdashenkov self-assigned this Nov 26, 2018
@dmdashenkov
Copy link
Contributor Author

@armiol, PTAL.

@codecov
Copy link

codecov bot commented Nov 26, 2018

Codecov Report

Merging #892 into master will increase coverage by <.01%.
The diff coverage is 93.33%.

@@             Coverage Diff             @@
##             master    #892      +/-   ##
===========================================
+ Coverage     93.39%   93.4%   +<.01%     
- Complexity     3868    3870       +2     
===========================================
  Files           529     529              
  Lines         12627   12637      +10     
  Branches        704     704              
===========================================
+ Hits          11793   11803      +10     
+ Misses          606     605       -1     
- Partials        228     229       +1

@dmdashenkov dmdashenkov changed the title Remove redundant DB calls Optimize aggregate read operation Dec 7, 2018
@dmdashenkov
Copy link
Contributor Author

@armiol, PTAL.

Copy link
Contributor

@armiol armiol left a comment

Choose a reason for hiding this comment

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

@dmdashenkov let's discuss the naming. Other than that, LGTM — once, hopefully, the build passes.

*
* @see AggregateStorage#readEventCountAfterLastSnapshot(Object)
*/
private int persistedEventCount = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

To me, the name of the variable does not correspond to its description and usage.

@dmdashenkov
Copy link
Contributor Author

@armiol, PTAL.

@SpineEventEngine SpineEventEngine deleted a comment Dec 8, 2018
@dmdashenkov dmdashenkov merged commit ef6c949 into master Dec 11, 2018
@dmdashenkov dmdashenkov deleted the store-events-since-snapshot-in-mem branch December 11, 2018 10:30
@armiol armiol mentioned this pull request Dec 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants