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

fix(actors): timer ticker bug #2673

Merged
merged 4 commits into from Jan 26, 2021
Merged

fix(actors): timer ticker bug #2673

merged 4 commits into from Jan 26, 2021

Conversation

1046102779
Copy link
Member

The timer task needs to wait for the execution of the first dueTime's task to start the period's timer

@codecov
Copy link

codecov bot commented Jan 17, 2021

Codecov Report

Merging #2673 (66ee557) into master (d3fbee9) will increase coverage by 0.05%.
The diff coverage is 68.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2673      +/-   ##
==========================================
+ Coverage   58.09%   58.14%   +0.05%     
==========================================
  Files          82       82              
  Lines        7385     7390       +5     
==========================================
+ Hits         4290     4297       +7     
+ Misses       2802     2799       -3     
- Partials      293      294       +1     
Impacted Files Coverage Δ
pkg/actors/actors.go 55.21% <68.42%> (+0.04%) ⬆️
pkg/placement/membership.go 53.90% <0.00%> (+2.83%) ⬆️

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 d3fbee9...4fee8ee. Read the comment docs.

@yaron2
Copy link
Member

yaron2 commented Jan 22, 2021

@vinayada1 can you review this?

@@ -828,6 +828,10 @@ func (a *actorsRuntime) CreateReminder(ctx context.Context, req *CreateReminderR
}

func (a *actorsRuntime) CreateTimer(ctx context.Context, req *CreateTimerRequest) error {
var (
err error
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add a UT?

Copy link
Member Author

Choose a reason for hiding this comment

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

it means Unit Test?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, unit test.

Copy link
Member Author

Choose a reason for hiding this comment

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

@yaron2
Copy link
Member

yaron2 commented Jan 26, 2021

@1046102779 we will merge this as is, thanks a lot for this fix. please find the time to add a unit test in a separate PR.

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.

None yet

4 participants