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.

Sign up for GitHub

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

fix: update max_in_use_session at 10 mins interval #3570

Merged
merged 2 commits into from
Dec 31, 2024
Merged

fix: update max_in_use_session at 10 mins interval #3570

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab9f3c5
fix: update max_in_use_session at 10 mins interval
surbhigarg92 Dec 26, 2024
cb3495e
review comments
surbhigarg92 Dec 31, 2024
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
12 changes: 11 additions & 1 deletion google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2048,6 +2048,7 @@ boolean isClosed() {

// Does various pool maintenance activities.
void maintainPool() {
Instant currTime = clock.instant();
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Do we really have to initialize here? Can we initialize before if statement inside synchronized block so that we will get exact current time. It would be okay to initialize here if we don't have synchronized block.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just noticed that go does the same way where they are assigning right before if statement. https://github.com/googleapis/google-cloud-go/blob/main/spanner/session.go#L1775

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this currTime is used at multiple places. Inside synchronized block and outside as well. It will be an overkill if we initialize at multiple times. Inside block and outside. WDYT ?

synchronized (lock) {
if (SessionPool.this.isClosed()) {
return;
Expand All @@ -2059,8 +2060,14 @@ void maintainPool() {
/ (loopFrequency / 1000L);
}
this.prevNumSessionsAcquired = SessionPool.this.numSessionsAcquired;

// Reset the start time for recording the maximum number of sessions in the pool
if (currTime.isAfter(SessionPool.this.lastResetTime.plus(Duration.ofMinutes(10)))) {
SessionPool.this.maxSessionsInUse = SessionPool.this.numSessionsInUse;
SessionPool.this.lastResetTime = currTime;
}
}
Instant currTime = clock.instant();

removeIdleSessions(currTime);
// Now go over all the remaining sessions and see if they need to be kept alive explicitly.
keepAliveSessions(currTime);
Expand Down Expand Up @@ -2309,6 +2316,9 @@ enum Position {
@GuardedBy("lock")
private int maxSessionsInUse = 0;

@GuardedBy("lock")
private Instant lastResetTime = Clock.INSTANCE.instant();

@GuardedBy("lock")
private long numSessionsAcquired = 0;

Expand Down
48 changes: 48 additions & 0 deletions google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2247,6 +2247,54 @@ public void testWaitOnMinSessionsThrowsExceptionWhenTimeoutIsReached() {
pool.maybeWaitOnMinSessions();
}

@Test
public void reset_maxSessionsInUse() {
Clock clock = mock(Clock.class);
when(clock.instant()).thenReturn(Instant.now());
options =
SessionPoolOptions.newBuilder()
.setMinSessions(1)
.setMaxSessions(3)
.setIncStep(1)
.setMaxIdleSessions(0)
.setPoolMaintainerClock(clock)
.build();
setupForLongRunningTransactionsCleanup(options);

pool = createPool(clock);
// Make sure pool has been initialized
pool.getSession().close();

// All 3 sessions used. 100% of pool utilised.
PooledSessionFuture readSession1 = pool.getSession();
PooledSessionFuture readSession2 = pool.getSession();
PooledSessionFuture readSession3 = pool.getSession();

// complete the async tasks
readSession1.get().setEligibleForLongRunning(false);
readSession2.get().setEligibleForLongRunning(false);
readSession3.get().setEligibleForLongRunning(true);

assertEquals(3, pool.getMaxSessionsInUse());
assertEquals(3, pool.getNumberOfSessionsInUse());

// Release 1 session
readSession1.get().close();

// Verify that numSessionsInUse reduces to 2 while maxSessionsInUse remain 3
assertEquals(3, pool.getMaxSessionsInUse());
assertEquals(2, pool.getNumberOfSessionsInUse());

// ensure that the lastResetTime for maxSessionsInUse > 10 minutes
when(clock.instant()).thenReturn(Instant.now().plus(11, ChronoUnit.MINUTES));

pool.poolMaintainer.maintainPool();

// Verify that maxSessionsInUse is reset to numSessionsInUse
assertEquals(2, pool.getMaxSessionsInUse());
assertEquals(2, pool.getNumberOfSessionsInUse());
}

private void mockKeepAlive(ReadContext context) {
ResultSet resultSet = mock(ResultSet.class);
when(resultSet.next()).thenReturn(true, false);
Expand Down
Loading