Skip to content

feat: add session pool option for modelling a timeout around session acquisition. #2641

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 13 commits into from
Sep 28, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix: review comments.
  • Loading branch information
arpan14 committed Sep 27, 2023
commit 2c9053b67b9f519b20cf958c24d17100396c855d
Original file line number Diff line number Diff line change
Expand Up @@ -1665,8 +1665,6 @@ public PooledSession get() {
try (Scope waitScope = tracer.withSpan(span)) {
PooledSession s =
pollUninterruptiblyWithTimeout(currentTimeout, options.getAcquireSessionTimeout());
// TODO clean up this code since acquireSessionTimeout will never be null and hence
// we won't have a use-case when s == null
if (s == null) {
// Set the status to DEADLINE_EXCEEDED and retry.
numWaiterTimeouts.incrementAndGet();
Expand Down Expand Up @@ -1695,8 +1693,6 @@ private PooledSession pollUninterruptiblyWithTimeout(
try {
while (true) {
try {
// TODO refactor this code since eventually acquireSessionTimeout will always have a
// default value and won't be null
return acquireSessionTimeout == null
? waiter.get(timeoutMillis, TimeUnit.MILLISECONDS)
: waiter.get(acquireSessionTimeout.toMillis(), TimeUnit.MILLISECONDS);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -718,9 +718,18 @@ public Builder setWaitForMinSessions(Duration waitForMinSessions) {

/**
* If greater than zero, we wait for said duration when no sessions are available in the {@link
* SessionPool}. When no configuration is passed, the default is a 60s timeout.
* SessionPool}. When no configuration is passed, the default is a 60s timeout. To avoid setting
* a default value, set the value as null.
*/
public Builder setAcquireSessionTimeout(Duration acquireSessionTimeout) {
try {
Preconditions.checkArgument(
acquireSessionTimeout.toMillis() > 0,
"acquireSessionTimeout should be greater than 0 ns");
} catch (ArithmeticException ex) {
throw new IllegalArgumentException(
"acquireSessionTimeout in millis should be lesser than Long.MAX_VALUE");
}
this.acquireSessionTimeout = acquireSessionTimeout;
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,10 +189,27 @@ public void setNegativeIdleTimeThreshold() {

@Test
public void setAcquireSessionTimeout() {
SessionPoolOptions sessionPoolOptions =
SessionPoolOptions sessionPoolOptions1 =
SessionPoolOptions.newBuilder().setAcquireSessionTimeout(Duration.ofSeconds(20)).build();
SessionPoolOptions sessionPoolOptions2 =
SessionPoolOptions.newBuilder()
.setAcquireSessionTimeout(Duration.ofMillis(Long.MAX_VALUE))
.build();

assertEquals(Duration.ofSeconds(20), sessionPoolOptions1.getAcquireSessionTimeout());
assertEquals(Duration.ofMillis(Long.MAX_VALUE), sessionPoolOptions2.getAcquireSessionTimeout());
}

@Test(expected = IllegalArgumentException.class)
public void setAcquireSessionTimeout_valueLessThanLowerBound() {
SessionPoolOptions.newBuilder().setAcquireSessionTimeout(Duration.ofMillis(0)).build();
}

assertEquals(Duration.ofSeconds(20), sessionPoolOptions.getAcquireSessionTimeout());
@Test(expected = IllegalArgumentException.class)
public void setAcquireSessionTimeout_valueMoreThanUpperBound() {
SessionPoolOptions.newBuilder()
.setAcquireSessionTimeout(Duration.ofSeconds(Long.MAX_VALUE))
.build();
}

@Test
Expand Down