Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Add v1 cortisol code #514

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add v1 cortisol code #514

wants to merge 2 commits into from

Conversation

ktkrg
Copy link
Contributor

@ktkrg ktkrg commented Nov 16, 2020

Fixes #:

Description of changes:
Simple load generator that ingests documents at a given rate.

Tests:

If new tests are added, how long do the new ones take to complete

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ktkrg ktkrg self-assigned this Nov 16, 2020
@sidheart sidheart self-requested a review November 20, 2020 17:33
Comment on lines +149 to +151
while (nTokens == 0) {
hasTokens.await();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be in a while loop?

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, to guard against spurious wake ups.

requestCount.addAndGet(bulkDocs.size());
}

private class TokenBucket {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should add a comment to this class since it functions slightly different than a standard token bucket. I believe this lets you make at most nTokens concurrent take() calls every second.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines +162 to +169
private void refill() {
lock.lock();
try {
nTokens = maxTokens;
hasTokens.signal();
} finally {
lock.unlock();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't the take() method just replenish the bucket by 1 once it's done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The work done after taking a token is dependent on the time each bulk() takes. We won't be able to guarantee a constant request rate. Some more explanation is in the next comment.

private class TokenBucket {
private final Lock lock = new ReentrantLock();
private final Condition hasTokens = lock.newCondition();
private final ScheduledExecutorService ses = Executors.newSingleThreadScheduledExecutor();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can avoid having a scheduler here. Have calls to take() block, and make sure every take() refills what it took from the bucket once it's complete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

take() already blocks with await() but I see what you mean here. The idea is to make sure that we make a fixed number of calls every second. If we made the call to makeBulkRequest() block and replenish the tokens once the requests are done, then we won't be able to guarantee a constant request rate. We don't want to replenish too soon(if bulk calls finish faster) or too late(if the bulk calls take longer to finish).

@ktkrg ktkrg requested a review from sidheart December 9, 2020 18:14
Base automatically changed from master to main February 8, 2021 19:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants