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

Throttle Hive splits discovery from the HiveSplitSource #534

Closed
wants to merge 4 commits into from

Conversation

3 participants
@BenoitHanotte
Copy link
Member

commented Mar 25, 2019

Add hive.max-splits-per-second configuration option to allow throttling
the number of splits released by the HiveSplitSource to provide strong
guarantees on the rate of read operations on the namenode.
Original issue discussion: prestodb/presto#12495

@cla-bot

This comment has been minimized.

Copy link

commented Mar 25, 2019

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

@cla-bot

This comment has been minimized.

Copy link

commented Mar 25, 2019

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

@BenoitHanotte BenoitHanotte force-pushed the BenoitHanotte:DPM-144-2 branch from 82c2b3d to 5da42e4 Mar 26, 2019

@cla-bot cla-bot bot added the cla-signed label Mar 26, 2019

@findepi
Copy link
Member

left a comment

Thanks @BenoitHanotte for your PR!

I did a quick pass only.
I would hear @electrum's opinion on whether this is reasonable addition in general (it is reasonable to me) and whether this is the best place to enforce rate limiting.

@BenoitHanotte BenoitHanotte force-pushed the BenoitHanotte:DPM-144-2 branch from 5da42e4 to 21b23ab Mar 26, 2019

Throttle Hive splits discovery from the HiveSplitSource
Add `hive.max-splits-per-sec` configuration option to allow throttling
the number of splits released by the HiveSplitSource to provide strong
guarantee on the rate of read operations on the namenode.

@BenoitHanotte BenoitHanotte force-pushed the BenoitHanotte:DPM-144-2 branch from 21b23ab to a75bf3b Mar 26, 2019

@BenoitHanotte BenoitHanotte force-pushed the BenoitHanotte:DPM-144-2 branch from a75bf3b to a52f0d1 Apr 3, 2019

@BenoitHanotte BenoitHanotte force-pushed the BenoitHanotte:DPM-144-2 branch from a52f0d1 to d77afd8 Apr 5, 2019

@martint martint requested a review from electrum Apr 22, 2019

@electrum
Copy link
Member

left a comment

Some minor comments. Over looks good, and much simpler than before. Apologies on the delay in reviewing this.

Please ping me when these are addressed so that we can get this merged.

Show resolved Hide resolved ...ive/src/main/java/io/prestosql/plugin/hive/util/ThrottledAsyncQueue.java
Show resolved Hide resolved ...ive/src/main/java/io/prestosql/plugin/hive/util/ThrottledAsyncQueue.java
}
}
else if (!isFinished()) {
// the queue is not empty, wait before we can query a batch

This comment has been minimized.

Copy link
@electrum

electrum May 6, 2019

Member

I think the not is a typo and it should be

// the queue is empty, wait before we can query a batch
@@ -45,7 +45,7 @@
private SettableFuture<?> notFullSignal = SettableFuture.create();
// This future is completed when the queue transitions from empty to not. But it will be replaced by a new instance of future immediately.
@GuardedBy("this")
private SettableFuture<?> notEmptySignal = SettableFuture.create();
protected SettableFuture<?> notEmptySignal = SettableFuture.create();

This comment has been minimized.

Copy link
@electrum

electrum May 6, 2019

Member

Add a protected getter for this that returns ListenableFuture. We wouldn't want subclasses to modify the field or to set the future.

* An asynchronous queue that limits the rate at which batches will be made available as well as the number
* of elements they will contain.
*
* @param <T> The type of elements accepted by the queue.

This comment has been minimized.

Copy link
@electrum

electrum May 6, 2019

Member

Nit: remove extra space after <T>

Show resolved Hide resolved ...src/test/java/io/prestosql/plugin/hive/util/TestThrottledAsyncQueue.java Outdated
Show resolved Hide resolved ...src/test/java/io/prestosql/plugin/hive/util/TestThrottledAsyncQueue.java Outdated
Show resolved Hide resolved ...src/test/java/io/prestosql/plugin/hive/util/TestThrottledAsyncQueue.java Outdated
Show resolved Hide resolved ...src/test/java/io/prestosql/plugin/hive/util/TestThrottledAsyncQueue.java Outdated
Show resolved Hide resolved presto-hive/src/main/java/io/prestosql/plugin/hive/HiveConfig.java Outdated

@BenoitHanotte BenoitHanotte force-pushed the BenoitHanotte:DPM-144-2 branch from d77afd8 to 1b77ecf Jun 5, 2019

@BenoitHanotte BenoitHanotte force-pushed the BenoitHanotte:DPM-144-2 branch from 1b77ecf to c9c1f1d Jun 5, 2019

@electrum

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

Merged, thanks!

@electrum electrum closed this Jun 7, 2019

@electrum

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

I fixed the rebase conflict, squashed and merged as cb62e11. Thanks the contribution, and I apologize for the very long turnaround on reviews.

@electrum

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

This will be in release 314

@electrum electrum referenced this pull request Jun 7, 2019

Closed

Release notes for 314 #879

2 of 6 tasks complete
@findepi

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

This is cool, thanks @BenoitHanotte!

@findepi findepi added this to the 314 milestone Jun 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.