Skip to content

[Obsolete] IWithBoundedStash > IWithStash #6511

Closed
eaba wants to merge 4 commits intoakkadotnet:devfrom
eaba:warning_CS0618_`IWithBoundedStash`
Closed

[Obsolete] IWithBoundedStash > IWithStash #6511
eaba wants to merge 4 commits intoakkadotnet:devfrom
eaba:warning_CS0618_`IWithBoundedStash`

Conversation

@eaba
Copy link
Contributor

@eaba eaba commented Mar 13, 2023

Changes

"Use IWithStash with a configured BoundedDeque-based mailbox instead."

Checklist

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

Latest dev Benchmarks

Include data from the relevant benchmark prior to this change here.

This PR's Benchmarks

Include data from after this change here.

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.

Didn't we just have a PR where we tried to do this prior to v1.5? cc @Arkatufus @ismaelhamed

{
if (actorType.Implements<IWithBoundedStash>())
if (actorType.Implements<IWithStash>())
return new BoundedStashImpl(context);
Copy link
Member

Choose a reason for hiding this comment

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

IWithStash must resolve to an unbounded deque mailbox by default, which is how it is configured in the hocon (L466)

@Aaronontheweb I don't think this code has ever worked as intended, because BoundedStashImpl and UnboundedStashImpl are the exact same implementation.

Unless I'm missing something we could simply it:

public static class StashFactory
{
    private class StashSupport : AbstractStash
    {
        public StashSupport(IActorContext context)
            : base(context)
        { }
    }

    public static IStash CreateStash(this IActorContext context) => new StashSupport(context);
}

Copy link
Contributor Author

@eaba eaba Mar 13, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Member

@ismaelhamed ismaelhamed Mar 14, 2023

Choose a reason for hiding this comment

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

IWithStash resolves to an unbounded deque by default, which is why it is falling. It is not a replacement for IWithBoundedStash. See its documentation.

@Aaronontheweb
Copy link
Member

@eaba closing this PR - just suppress the warnings where they show up. No one users this feature much anyway.

@Aaronontheweb
Copy link
Member

Actually, per @Arkatufus suggestion - remove the Obsolete attribute on IWithBoundedStash. It's completely misleading.

@ismaelhamed
Copy link
Member

Actually, per @Arkatufus suggestion - remove the Obsolete attribute on IWithBoundedStash. It's completely misleading.

Actually, it's telling how to properly create a working bounded stash. If you only rely on the IWithBoundedStash attribute you'll end up with a BoundedDeque with a stash capacity of -1, since you cannot set the stash capacity via the interface (needs to be done via configuration).

@Aaronontheweb
Copy link
Member

Actually, per @Arkatufus suggestion - remove the Obsolete attribute on IWithBoundedStash. It's completely misleading.

Actually, it's telling how to properly create a working bounded stash. If you only rely on the IWithBoundedStash attribute you'll end up with a BoundedDeque with a stash capacity of -1, since you cannot set the stash capacity via the interface (needs to be done via configuration).

I'm out of my depth here - would you care to send a PR?

@ismaelhamed
Copy link
Member

Sure thing, I'll see if I can find some time tomorrow.

@Aaronontheweb
Copy link
Member

Superseded via #6519

@eaba eaba deleted the warning_CS0618_`IWithBoundedStash` branch March 15, 2023 14:46
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.

3 participants