Skip to content

Conversation

@Arkatufus
Copy link
Contributor

Fixes #7379

Changes

  • Modernize Shard code
  • Block lease Release() in PostStop instead of doing a detached task. This should be save because Lease is required to have a per operation timeout limit.

Checklist

For significant changes, please ensure that the following have been completed (delete if not relevant):

Copy link
Contributor Author

@Arkatufus Arkatufus left a comment

Choose a reason for hiding this comment

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

Self-review


try
{
ReleaseLease().GetAwaiter().GetResult();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actual fix, we block instead of running Release() in a detached task.


return;

async Task ReleaseLease()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code modernization, refactor ContinueWith to internal method

TryLoadRememberedEntities();
return true;

case LeaseAcquireResult lar when !lar.Acquired && lar.Reason == null:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code removed and simplified, ILoggingAdaptor.Error can accept null exception, this code is not needed.

Acquire().PipeTo(self);
return;

async Task<LeaseAcquireResult> Acquire()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code modernization, refactor ContinueWith to internal method

@Arkatufus Arkatufus changed the title Fix shard lease Make shard lease Release block in PostStop Nov 8, 2024
@Aaronontheweb Aaronontheweb added akka-cluster-sharding akka-coordination Akka.Coordination package updates and materials labels Nov 8, 2024
Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

LGTM

}
catch
{
// no-op, we're shutting down anyway.
Copy link
Member

Choose a reason for hiding this comment

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

Might want to log an Exception here just in case

Copy link
Member

Choose a reason for hiding this comment

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

Ah NM - you do it down below

@Aaronontheweb Aaronontheweb enabled auto-merge (squash) November 8, 2024 19:01
@Aaronontheweb Aaronontheweb merged commit 322c494 into akkadotnet:dev Nov 8, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

akka-cluster-sharding akka-coordination Akka.Coordination package updates and materials

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Shard lease did not release held leases cleanly when downed

2 participants