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(spanner): fix negative values for max_in_use_sessions metrics #10449

Merged
merged 6 commits into from
Jun 28, 2024
Merged

fix(spanner): fix negative values for max_in_use_sessions metrics #10449

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6ad748f
fix(spanner): fix negative values for max_in_use_sessions metrics
rahul2393 Jun 27, 2024
837ea3b
fix failing tests
rahul2393 Jun 27, 2024
78e81d5
incorporate changes
rahul2393 Jun 27, 2024
0d4cbca
add comment
rahul2393 Jun 27, 2024
82b7b78
Merge branch 'main' into fix_negative_metrics
gcf-merge-on-green[bot] Jun 27, 2024
2b12dd5
Merge branch 'main' into fix_negative_metrics
gcf-merge-on-green[bot] Jun 28, 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
6 changes: 4 additions & 2 deletions spanner/session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1152,8 +1152,10 @@ func (p *sessionPool) remove(s *session, isExpire bool) bool {
if s.invalidate() {
// Decrease the number of opened sessions.
p.numOpened--
// Decrease the number of sessions in use.
p.decNumInUseLocked(ctx)
// Decrease the number of sessions in use, only when not from idle list.
if !isExpire {
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 that it would be better to add an additional argument to the remove(..) method that explicitly says whether the session was in use or not. So something like this:

func (p *sessionPool) remove(s *session, isExpire bool, wasInUse bool) {
  ...
  if wasInUse {
    p.decNumInUseLocked(ctx)
  }
}

In theory, it could be that this method is called in the future to remove sessions that have not expired, but that also were not in use at the time that they were being removed, and then we could re-introduce a similar bug as the one here. That is less likely with an explicit argument that clearly says what it is for.

p.decNumInUseLocked(ctx)
}
p.recordStat(ctx, OpenSessionCount, int64(p.numOpened))
// Broadcast that a session has been destroyed.
close(p.mayGetSession)
Expand Down
51 changes: 51 additions & 0 deletions spanner/session_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1022,6 +1022,57 @@ func TestMinOpenedSessions(t *testing.T) {
}
}

// TestPositiveNumInUseSessions tests that num_in_use session should always be greater than 0.
func TestPositiveNumInUseSessions(t *testing.T) {
t.Parallel()
ctx := context.Background()
_, client, teardown := setupMockedTestServerWithConfig(t,
ClientConfig{
SessionPoolConfig: SessionPoolConfig{
MinOpened: 1,
healthCheckSampleInterval: time.Millisecond,
},
})
defer teardown()
sp := client.idleSessions
defer sp.close(ctx)
// Take ten sessions from session pool and recycle them.
var shs []*sessionHandle
for i := 0; i < 10; i++ {
sh, err := sp.take(ctx)
if err != nil {
t.Fatalf("failed to get session(%v): %v", i, err)
}
shs = append(shs, sh)
}
for _, sh := range shs {
sh.recycle()
}

for true {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can we add an escape hatch to the loop (e.g. stop after X time)? Now a future bug could cause this to loop for ever, which is harder to debug than a test failure after for example 2 seconds.

sp.mu.Lock()
if sp.idleList.Len() == 1 {
sp.mu.Unlock()
break
}
sp.mu.Unlock()
continue
}
sp.mu.Lock()
defer sp.mu.Unlock()
if int64(sp.numInUse) < 0 {
t.Fatal("numInUse must be >= 0")
}
// There should be still one session left in either the idle list or in one
// of the other opened states due to the min open sessions constraint.
if (sp.idleList.Len() +
int(sp.createReqs)) != 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can this be on one line (I found it hard to read in the current form)

t.Fatalf(
"got %v sessions in idle lists, want 1. Opened: %d, Creation: %d",
sp.idleList.Len(), sp.numOpened, sp.createReqs)
}
}

// TestMaxBurst tests max burst constraint.
func TestMaxBurst(t *testing.T) {
t.Parallel()
Expand Down
Loading