Skip to content

Commit

Permalink
spanner: switch the session keepalive method from GetSession to SELECT 1
Browse files Browse the repository at this point in the history
Change-Id: Idf8ed090f628a6c8d1ffe7b347b93b10471380cd
Reviewed-on: https://code-review.googlesource.com/c/gocloud/+/54090
Reviewed-by: Knut Olav Løite <koloite@gmail.com>
Reviewed-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Shanika Kuruppu <skuruppu@google.com>
  • Loading branch information
hengfengli committed Apr 7, 2020
1 parent f62fbb4 commit f2776fc
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 8 deletions.
8 changes: 5 additions & 3 deletions spanner/internal/testutil/inmem_spanner_server.go
Expand Up @@ -633,9 +633,6 @@ func (s *inMemSpannerServer) GetSession(ctx context.Context, req *spannerpb.GetS
if err := s.simulateExecutionTime(MethodGetSession, req); err != nil {
return nil, err
}
s.mu.Lock()
s.pings = append(s.pings, req.Name)
s.mu.Unlock()
if req.Name == "" {
return nil, gstatus.Error(codes.InvalidArgument, "Missing session name")
}
Expand Down Expand Up @@ -694,6 +691,11 @@ func (s *inMemSpannerServer) ExecuteSql(ctx context.Context, req *spannerpb.Exec
if err := s.simulateExecutionTime(MethodExecuteSql, req); err != nil {
return nil, err
}
if req.Sql == "SELECT 1" {
s.mu.Lock()
s.pings = append(s.pings, req.Session)
s.mu.Unlock()
}
if req.Session == "" {
return nil, gstatus.Error(codes.InvalidArgument, "Missing session name")
}
Expand Down
5 changes: 4 additions & 1 deletion spanner/session.go
Expand Up @@ -228,7 +228,10 @@ func (s *session) ping() error {
ctx, cancel := context.WithTimeout(context.Background(), time.Second)
defer cancel()
// s.getID is safe even when s is invalid.
_, err := s.client.GetSession(contextWithOutgoingMetadata(ctx, s.md), &sppb.GetSessionRequest{Name: s.getID()})
_, err := s.client.ExecuteSql(contextWithOutgoingMetadata(ctx, s.md), &sppb.ExecuteSqlRequest{
Session: s.getID(),
Sql: "SELECT 1",
})
return err
}

Expand Down
8 changes: 4 additions & 4 deletions spanner/session_test.go
Expand Up @@ -371,7 +371,7 @@ func TestTakeFromIdleListChecked(t *testing.T) {
// Inject session error to server stub, and take the session from the
// session pool, the old session should be destroyed and the session pool
// will create a new session.
server.TestSpanner.PutExecutionTime(MethodGetSession,
server.TestSpanner.PutExecutionTime(MethodExecuteSql,
SimulatedExecutionTime{
Errors: []error{newSessionNotFoundError("projects/p/instances/i/databases/d/sessions/s")},
})
Expand Down Expand Up @@ -451,7 +451,7 @@ func TestTakeFromIdleWriteListChecked(t *testing.T) {
// Inject session error to mockclient, and take the session from the
// session pool, the old session should be destroyed and the session pool
// will create a new session.
server.TestSpanner.PutExecutionTime(MethodGetSession,
server.TestSpanner.PutExecutionTime(MethodExecuteSql,
SimulatedExecutionTime{
Errors: []error{newSessionNotFoundError("projects/p/instances/i/databases/d/sessions/s")},
})
Expand Down Expand Up @@ -1303,7 +1303,7 @@ func TestSessionHealthCheck(t *testing.T) {
}

server.TestSpanner.Freeze()
server.TestSpanner.PutExecutionTime(MethodGetSession,
server.TestSpanner.PutExecutionTime(MethodExecuteSql,
SimulatedExecutionTime{
Errors: []error{newSessionNotFoundError("projects/p/instances/i/databases/d/sessions/s")},
KeepError: true,
Expand All @@ -1319,7 +1319,7 @@ func TestSessionHealthCheck(t *testing.T) {
})

server.TestSpanner.Freeze()
server.TestSpanner.PutExecutionTime(MethodGetSession, SimulatedExecutionTime{})
server.TestSpanner.PutExecutionTime(MethodExecuteSql, SimulatedExecutionTime{})
server.TestSpanner.Unfreeze()

// Test garbage collection.
Expand Down

0 comments on commit f2776fc

Please sign in to comment.