Skip to content

Throttling pattern#629

Merged
iluwatar merged 4 commits into
iluwatar:masterfrom
rastdeepanshu:throttling-pattern
Sep 11, 2017
Merged

Throttling pattern#629
iluwatar merged 4 commits into
iluwatar:masterfrom
rastdeepanshu:throttling-pattern

Conversation

@rastdeepanshu

Copy link
Copy Markdown
Contributor

#456

Created throttling pattern which throttles request of a particular tenant.
The service only allows a specified number of requests for a tenant and rejects the remaining ones.

Comment thread throttling/README.md Outdated
---

## Intent
Ensure that a given tenant is not able to access resources more than the assigned limit.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Should we talk about client here instead of tenant? And service resources instead of just resources. Check for occurrences elsewhere too.

* THE SOFTWARE.
*/

package com.iluwatar.tls;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

com.iluwatar.throttling?

Tenant nike = new Tenant("Nike", 70);

B2BService adidasService = new B2BService(adidas);
B2BService nikeService = new B2BService(nike);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The relationship between service and tenant here is one-to-one which seems unnatural. The service should have multiple tenants, I think.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

B2BService is one service which serves multiple tenants. As I am not dealing with the APIs, one instance of the service deals with one tenant.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I get your point now.

Runnable nikeTask = () -> makeServiceCalls(nikeService);

new Thread(adidasTask).start();
new Thread(nikeTask).start();

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Threads should not be instantiated directly. Instead use ExecutorService.

try {
Thread.sleep(10);
} catch (InterruptedException e) {
e.printStackTrace();

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Should use logger here


/**
* A service which accepts a tenant and throttles the resource based on the time given to the tenant.
*/

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The class seems to have multiple responsibilities. It offers the service and handles the throttling. Can we separate the throttling to another class? Maybe time based throttling would be one strategy but it could have others.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see


/**
* A timer is initiated as soon as the Service is initiated. The timer runs every minute and resets the
* counter.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Seems to run every second


public void setName(String name) {
this.name = name;
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The method is unused and should be removed


public void setAllowedCallsPerSecond(int allowedCallsPerSecond) {
this.allowedCallsPerSecond = allowedCallsPerSecond;
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The method is unused and should be removed

}
}
int counter = service.getCurrentCallsCount();
Assert.assertTrue("", counter < 11);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Where this magic number 11 comes from?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sleep for 100 ms means counter can go till 10. Hence the magic number :D
I can do <= 10 if 11 is looking odd.

@iluwatar

iluwatar commented Sep 7, 2017

Copy link
Copy Markdown
Owner

Add AppTest that runs the example. See the other patterns for an example.

Comment thread throttling/pom.xml
@@ -0,0 +1,44 @@
<?xml version="1.0" encoding="UTF-8"?>

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The license is incorrectly positioned

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate. I guess the first line has to be this for pom to work correctly.

@iluwatar

iluwatar commented Sep 7, 2017

Copy link
Copy Markdown
Owner

@rastdeepanshu you have my review comments. Please notify when you've implemented the changes.

@rastdeepanshu

Copy link
Copy Markdown
Contributor Author

@iluwatar Can you take a look now?

@iluwatar iluwatar merged commit 4e29041 into iluwatar:master Sep 11, 2017
@iluwatar

Copy link
Copy Markdown
Owner

@rastdeepanshu well done. Thanks for your effort to add this new pattern 👍

@rastdeepanshu rastdeepanshu deleted the throttling-pattern branch September 11, 2017 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants