Skip to content

Modifications for intermittent test failure in Throttling pattern. #646

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

Merged
merged 3 commits into from
Oct 9, 2017

Conversation

rastdeepanshu
Copy link
Contributor

@rastdeepanshu rastdeepanshu commented Sep 25, 2017

Used AtomicLong instead of Integer in ConcurrentHashMap to make the increment calls atomic.
Ref: 643

@@ -36,14 +36,15 @@
@Test
public void dummyCustomerApiTest() {
Tenant tenant = new Tenant("testTenant", 2);
Throttler timer = new ThrottleTimerImpl(10);
Throttler timer = new ThrottleTimerImpl(100);
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to change value from 10 to 100?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sometimes the counter was being reset within the 10 ms period. To avoid that, the value is increased.

}

/**
*
* @param tenantName name of the tenant.
* @return the count of the tenant.
*/
public static int getCount(String tenantName) {
return tenantCallsCount.get(tenantName);
public static Long getCount(String tenantName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Keep it long instead of wrapper counterpart

private static Map<String, Integer> tenantCallsCount = new ConcurrentHashMap<>();

private static final Logger LOGGER = LoggerFactory.getLogger(CallsCount.class);
private static Map<String, AtomicLong> tenantCallsCount = new ConcurrentHashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Why change from int to long? Why not AtomicInteger? Is it because the call count can breach MAXINT limit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

B2BService service = new B2BService(timer);


long counter = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain briefly why are these changes needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I am unable to figure out this. The code works perfectly this way but if the counter is calculated outside the for loop(like done previously), then sometimes it is not reflecting the proper changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am sorry. I think I tested this method before incrementing the count limit from 10 to 100(which was the root of intermittent failure). Will change this back to the original

@rastdeepanshu
Copy link
Contributor Author

rastdeepanshu commented Sep 28, 2017

@npathai i have made the necessary changes and comments.

@npathai npathai merged commit 31120a8 into iluwatar:master Oct 9, 2017
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