Skip to content

Add pause between stopping/starting runtime#526

Merged
halspang merged 2 commits into
dapr:masterfrom
halspang:flaky_it
Apr 5, 2021
Merged

Add pause between stopping/starting runtime#526
halspang merged 2 commits into
dapr:masterfrom
halspang:flaky_it

Conversation

@halspang

@halspang halspang commented Mar 24, 2021

Copy link
Copy Markdown
Contributor

Description

ActorReminderRecoveryIT and ActorTimerRecoveryIT both rely on reminders/timers being persisted when dapr restarts. On some restarts, it appears that the placement service shares its placement table during on ongoing update. This causes the dapr runtime to use the wrong address for the actor.

Placement service logs:

INFO[0322] Entering removeMember()                       instance=Hals-MacBook-Pro.local scope=dapr.placement.raft type=log ver=edge
INFO[0322] Loading: 192.168.0.219:50394, 0, 0, ActorReminderRecoveryIT  instance=Hals-MacBook-Pro.local scope=dapr.placement.raft type=log ver=edge
INFO[0322] Loading: 192.168.0.219:50489, 0, 0, ActorReminderRecoveryIT  instance=Hals-MacBook-Pro.local scope=dapr.placement.raft type=log ver=edge
INFO[0322] Start desseminating tables. memberUpdateCount: 1, streams: 2, targets: 2, table generation: 10  instance=Hals-MacBook-Pro.local scope=dapr.placement type=log ver=edge
INFO[0322] Removing 192.168.0.219:50394                  instance=Hals-MacBook-Pro.local scope=dapr.placement.raft type=log ver=edge
INFO[0322] Completed dessemination. memberUpdateCount: 1, streams: 2, targets: 2, table generation: 10  instance=Hals-MacBook-Pro.local scope=dapr.placement type=log ver=edge
java.lang.RuntimeException: io.dapr.exceptions.DaprException: INTERNAL: error invoke actor method: failed to invoke target 192.168.0.219:50394 after 3 retries

When this interleaving happens, the error message received references the actor with the port that was being removed. This implies that the table isn't being updated appropriately and the runtime gets the old port. When the actor is receiving continuous calls, the placement service does not update the location as it believes the actor is busy.

Issue reference

Closes: #425

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@halspang halspang changed the title Add app health check to flaky test WIP: Add app health check to flaky test Mar 24, 2021
@halspang halspang marked this pull request as draft March 24, 2021 23:53
ActorReminderRecoveryIT and ActorTimerRecoveryIT both rely
on reminders/timers being persisted when dapr restarts. On some
restarts, it appears that the placement service shares its placement
table during on ongoing update. This causes the dapr runtime to use
the wrong address for the actor.

dapr#425
@halspang halspang changed the title WIP: Add app health check to flaky test WIP: Add pause between stopping/starting runtime Mar 30, 2021
@codecov

codecov Bot commented Mar 30, 2021

Copy link
Copy Markdown

Codecov Report

Merging #526 (f33d99d) into master (621d4f8) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #526   +/-   ##
=========================================
  Coverage     80.59%   80.59%           
  Complexity      976      976           
=========================================
  Files            88       88           
  Lines          3015     3015           
  Branches        331      331           
=========================================
  Hits           2430     2430           
  Misses          420      420           
  Partials        165      165           

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 621d4f8...f33d99d. Read the comment docs.

@nobodyiam

Copy link
Copy Markdown

why is it 10 seconds? will it vary in different environments?
I would suggest we take a look at the awaitility project, it may help.

@halspang

halspang commented Apr 1, 2021

Copy link
Copy Markdown
Contributor Author

why is it 10 seconds? will it vary in different environments?
I would suggest we take a look at the awaitility project, it may help.

Admittedly it's fairly arbitrary. The issue here is that what we're waiting to propagate is information that exists inside the runtime itself. This information isn't available to the Java SDK (nor should it be). Since we can't access the required state, I don't think there's anything we could really gate on. The library looks useful though.

For context I did at first try and do something similar where I would wait and ensure the app started responding again. The issue with this was that in this state, the continued requests to the actor would cause it to be held in memory longer so there'd have to be some arbitrary wait time regardless.

@halspang halspang marked this pull request as ready for review April 1, 2021 21:21
@halspang halspang requested a review from artursouza April 1, 2021 21:21
@halspang halspang changed the title WIP: Add pause between stopping/starting runtime Add pause between stopping/starting runtime Apr 1, 2021
@artursouza

Copy link
Copy Markdown
Contributor

why is it 10 seconds? will it vary in different environments?
I would suggest we take a look at the awaitility project, it may help.

Admittedly it's fairly arbitrary. The issue here is that what we're waiting to propagate is information that exists inside the runtime itself. This information isn't available to the Java SDK (nor should it be). Since we can't access the required state, I don't think there's anything we could really gate on. The library looks useful though.

For context I did at first try and do something similar where I would wait and ensure the app started responding again. The issue with this was that in this state, the continued requests to the actor would cause it to be held in memory longer so there'd have to be some arbitrary wait time regardless.

I like the awaitility project - we should use this in our test project here. Like Hal said, this is to avoid a race condition in the runtime where the state is not exposed. @halspang please, create an issue in dapr/dapr - we should not have to wait to restart.

@halspang halspang merged commit bdf8e86 into dapr:master Apr 5, 2021
fackee pushed a commit to fackee/java-sdk that referenced this pull request Mar 28, 2022
ActorReminderRecoveryIT and ActorTimerRecoveryIT both rely
on reminders/timers being persisted when dapr restarts. On some
restarts, it appears that the placement service shares its placement
table during on ongoing update. This causes the dapr runtime to use
the wrong address for the actor.

dapr#425
Signed-off-by: zjx244729 <zjx244729@alibaba-inc.com>
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.

Fix flaky reminderRecoveryTest

4 participants