Unify on IModel

[danielmarbach]

Proposed Changes

The worker service did mostly use IModel exception when the model was added with safely casted it to ModelBase but never actually verified if the model implements ModelBase. Any model not deriving from ModelBase would cause a NullRef. Given that the ModelBase method is only used in the exception case I moved the conversion to ModelBase into the worker implementation.

Of course this change has the tradeoff of making now everything an interface dispatch as was as safe casting on the exception path (which is slow anyway) so I'll leave it up to you if you think this is good or not. I'd suggest though if this PR is not taken to throw a meaningful exception if the model cannot be converted to model base.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

• Bug fix (non-breaking change which fixes issue #NNNN)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause an observable behavior change in existing systems)
• Documentation improvements (corrections, new content, etc)
• Cosmetic change (whitespace, formatting, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.

• I have read the CONTRIBUTING.md document
• I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• All tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• Any dependent changes have been merged and published in related repositories

Further Comments

[danielmarbach]

Happy to roll back if you want to keep the white space

[michaelklishin]

Nah, turning down a PR because of a single whitespace change sounds unreasonable :)

[michaelklishin]

@danielmarbach the callback exception path should not generally be executed very often. However, maybe we should borrow a complete enough (using real publishers and consumers) benchmark from one of the recent PRs to have some data to make a more informed decision?

Just curious, was some kind of mock implementation of IModel used in your case?

[danielmarbach]

No mock. I looked at it from a more open/close and liskov principle. I'm not disappointed if this is thrown away and I also don't want to spend too much time on this. For the recent ping I had to dig into the worker services and I found this weird state of IModel vs ModelBase. Given I found no guidance which one should be defacto standard I figured I send in a suggestion that uses the most abstract one which is the interface

[danielmarbach]

Any thoughts on this, is it worth a rebase or not?

[michaelklishin]

I'd say this change makes sense.

[danielmarbach]

Ok rebased

[michaelklishin]

@danielmarbach can you please submit this based on the 6.x branch? There are conflicts that are not obvious to me.

[danielmarbach]

Here we go #898