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

Improve the adaptation speed of polling algorithm #88

Merged
merged 9 commits into from Sep 16, 2015

Conversation

SzymonPobiega
Copy link
Member

  • Added a bunch of simulation tests that are meant to verify various scenarios of adapting to load
  • Refactored BackOff class to be testable
  • Decouple the receiver and sender from the BehaviorContext in order to test them easier
  • Centralized all the important parameters of the adaptive algorithm in AdaptivePollingReceiver class
  • Added code for starting additional threads when the current thread is processing a slow message (> 5 seconds of processing time) so that the adaptive algorithm can keep adapting

@SzymonPobiega
Copy link
Member Author

@Particular/engineers @johnsimons can you review?

@mikeminutillo
Copy link
Member

Is the use of DateTime going to be problematic around Daylight Savings change-overs (i.e. Suddenly everything claims to be long running). You could do this with 2 flags. First check for "is long running" and "hasn't triggered another task" and trigger new tasks. Then mark everything as "long running". If they don't get reset before you get back around to them then they're long running.

@johnsimons
Copy link
Member

Is the use of DateTime going to be problematic around Daylight Savings change-overs

Not if we use UTC

@mikeminutillo
Copy link
Member

This works too :)

class TaskState
{
    int seen;

    public void Reset()
    {
        seen = 0;
    }

    public void TriggerIfFirstDetectedSlow(Action action)
    {
        seen +=1;
        if(seen == 2)
            action();
    }
}

@SzymonPobiega
Copy link
Member Author

I like ❤️ ❤️ ❤️ the seen version. Thanks @johnsimons and @WolfByte !


public RealSimulator(string address, string connectionString)
{

Copy link
Contributor

Choose a reason for hiding this comment

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

You little @andreasohlund you!

Copy link
Member

Choose a reason for hiding this comment

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

Huh? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

See the unnecessary line break?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Ufff. Disaster averted.

@danielmarbach
Copy link
Contributor

There is much complexity involved here. I wonder if it is really needed. The default TaskScheduler itself already has ramp up strategies built in. I wonder if we could just use that and do some smart connection pooling instead.

@SzymonPobiega
Copy link
Member Author

Applied the suggestions.

 * Extracted all constants controlling scheduler to AdaptivePollingReceiver
 * Added role interface for the back-off strategy
 * Decoupled the sender and receiver from the pipeline ContextItemRemovalDisposable
 * Removed the ITaskTracker abstraction
@adamralph
Copy link
Member

@SzymonPobiega I reviewed the PR on my flight on Friday and it all seemed to make sense.

BTW - I did some minor refactorings as I was reading the code, if you want to cherry pick them they are on https://github.com/adamralph/NServiceBus.SqlServer/commits/make-scheduler-adapt-faster-refactor

Before we merge, I'd like to second @danielmarbach's comment. Do we have evidence that this isn't already a solved problem .NET's default TaskScheduler?

SzymonPobiega added a commit that referenced this pull request Sep 16, 2015
@SzymonPobiega SzymonPobiega merged commit 01104dc into develop Sep 16, 2015
@SzymonPobiega SzymonPobiega deleted the make-scheduler-adapt-faster branch September 16, 2015 10:30
@adamralph
Copy link
Member

Woot! My first commits to core 😛

@SzymonPobiega
Copy link
Member Author

@adamralph What about Rabbit?

@adamralph
Copy link
Member

not core?

@adamralph
Copy link
Member

Oops, this is sql... And I already have 3 commits in core.

Ignore me. As you were. 😊

@SzymonPobiega
Copy link
Member Author

Be quick, merge this one: Particular/NServiceBus#2884

@SzymonPobiega SzymonPobiega self-assigned this Sep 16, 2015
@SzymonPobiega SzymonPobiega added this to the 2.2.0 milestone Sep 16, 2015
@SzymonPobiega SzymonPobiega changed the title Make scheduler adapt faster Improve the adaptation speed of polling algorithm Sep 24, 2015
@yta1
Copy link

yta1 commented Oct 1, 2015

@ramonsmits and all, it looks like 2.1.4 fix[https://github.com//issues/90] was not merged into 2.2.0? If we upgrade we'll reintroduce the memory leak of 2.1.3: https://github.com/Particular/NServiceBus.SqlServer/blob/2.2.0/src/NServiceBus.SqlServer/AdaptiveExecutor.cs

@yta1
Copy link

yta1 commented Oct 1, 2015

My bad, it is merged in master.,,

@particularbot particularbot removed this from the 2.2.0 milestone Oct 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants