feat(spanner): add support of using multiplexed session with ReadOnlyTransactions #10269

rahul2393 · 2024-05-28T05:40:29Z

Enhanced support for multiplexed sessions in the Spanner client, improving session management and efficiency.
Added metrics tracking for multiplexed sessions, providing better insights into usage patterns.
Adjusted testing conditions for session pools and metrics to accurately reflect multiplexing configurations.
Expanded test coverage to include scenarios for multiplexed sessions, ensuring robust session management functionality.
Added automation script for testing processes in the CI environment, improving integration testing efficiency.

rahul2393 · 2024-07-15T11:06:23Z

olavloite

(First pass, continuing review tomorrow.)

olavloite · 2024-07-18T18:42:42Z

spanner/integration_test.go

+	if os.Getenv("GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSIONS") == "true" {
+		return "true"
+	}


Not very important, but this seems a bit strange. Why are we returning a standardized string from this method, instead of a bool?

olavloite · 2024-07-18T18:43:37Z

spanner/kokoro/presubmit.sh

+# See the License for the specific language governing permissions and
+# limitations under the License..
+
+# TODO(deklerk): Add integration tests when it's secure to do so. b/64723143


I think we can remove this TODO now ;-)

olavloite · 2024-07-18T18:48:24Z

spanner/session.go

+	if sh.client != nil {
+		sh.client = nil
+	}


nit: just remove the nil check, it is safe to always set it to nil

olavloite · 2024-07-18T18:50:06Z

spanner/session.go

@@ -149,6 +160,10 @@ func (sh *sessionHandle) getClient() *vkit.Client {
 	if sh.session == nil {
 		return nil
 	}
+	if sh.session.isMultiplexed {


I think it would be safer/easier to read if we just check for if sh.client != nil. That also allows us to use the same field for regular sessions if that would ever be handy, and it is clear to anyone reading the code.

olavloite · 2024-07-18T18:50:29Z

spanner/session.go

+	if sh.client != nil {
+		sh.client = nil
+	}


Here also: Just remove the if check and always set to nil

olavloite · 2024-07-18T18:51:08Z

spanner/session.go

@@ -570,12 +595,20 @@ type sessionPool struct {
 	// idleList caches idle session IDs. Session IDs in this list can be
 	// allocated for use.
 	idleList list.List
+	// multiplexedSessions contains the multiplexed sessions


nit:

Suggested change

// multiplexedSessions contains the multiplexed sessions

// multiplexedSession contains the multiplexed session

olavloite · 2024-07-18T18:53:06Z

spanner/session.go

+	// multiplexedSessionCreationError is the last error that occurred during multiplexed session
+	// creation and is propagated to any waiters waiting for a session.
+	multiplexedSessionCreationError error


(Adding a comment for now, maybe it turns out to be void after reading the rest of the code)

We should only propagate this error to any waiters for the first creation of a multiplexed session. If refreshing a multiplexed session fails, then we should not propagate that error to the application, as the multiplexed session is likely to be usable for many days still.

Yes, if refreshing fails it will use existing multiplexed session. Added tests in TestMultiplexSessionWorker

olavloite · 2024-07-19T05:51:10Z

spanner/session.go

+		attrs := p.otConfig.attributeMap
+		for _, attr := range attributes {
+			attrs = append(attrs, attr)
+		}
+		m.Add(ctx, val, metric.WithAttributes(attrs...))


Instead of creating a new attribute map here every time, can we just have two attribute maps set on otConfig? One with multiplexed=true and one with multiplexed=false. This method is called for every transaction that is executed, and it feels a bit wasteful to create this map over and over again, when there are only two possible values.

Added separate maps, and updated tests

olavloite · 2024-07-19T05:52:30Z

spanner/session.go

@@ -813,13 +859,34 @@ func (p *sessionPool) growPoolLocked(numSessions uint64, distributeOverChannels
 	return p.sc.batchCreateSessions(int32(numSessions), distributeOverChannels, p)
 }

+func (p *sessionPool) getMultiplexedSession(ctx context.Context) error {


The name of this method seems confusing to me. It is named get..., but it calls a method called executeCreateMultiplexedSessions(...)

olavloite · 2024-07-19T05:52:58Z

spanner/session.go

+	if err != nil {
+		return err
+	}
+	go p.sc.executeCreateMultiplexedSessions(ctx, client, p.sc.sessionLabels, p.sc.md, p)


Rename this method:

Suggested change

go p.sc.executeCreateMultiplexedSessions(ctx, client, p.sc.sessionLabels, p.sc.md, p)

go p.sc.executeCreateMultiplexedSession(ctx, client, p.sc.sessionLabels, p.sc.md, p)

olavloite · 2024-07-19T05:54:43Z

spanner/session.go

 // sessionReady is executed by the SessionClient when a session has been
 // created and is ready to use. This method will add the new session to the
 // pool and decrease the number of sessions that is being created.
 func (p *sessionPool) sessionReady(s *session) {
 	p.mu.Lock()
 	defer p.mu.Unlock()
 	// Clear any session creation error.
+	if s.isMultiplexed {
+		s.pool = p
+		p.hc.register(s)


Note for self: This seems to register the multiplexed session with the normal health checker. That can be OK, as long as we treat it as a multiplexed session (e.g. no pings)

Not registering now

olavloite · 2024-07-19T05:55:33Z

spanner/session.go

+		p.multiplexedSessionCreationError = nil
+		p.recordStat(context.Background(), SessionsCount, 1, tagNumSessions, tag.Tag{Key: tagKeyIsMultiplexed, Value: "true"})
+		close(p.mayGetMultiplexedSession)
+		p.mayGetMultiplexedSession = make(chan struct{})


Why are we re-creating the channel here?

olavloite · 2024-07-19T06:29:54Z

spanner/sessionclient.go

+	sc.mu.Unlock()
+	if closed {
+		err := spannerErrorf(codes.Canceled, "Session client closed")
+		trace.TracePrintf(ctx, nil, "Session client closed while creating multiplexed sessions: %v", err)


Suggested change

trace.TracePrintf(ctx, nil, "Session client closed while creating multiplexed sessions: %v", err)

trace.TracePrintf(ctx, nil, "Session client closed while creating a multiplexed session: %v", err)

olavloite · 2024-07-19T06:30:58Z

spanner/sessionclient.go

+	var mdForGFELatency metadata.MD
+	response, err := client.CreateSession(contextWithOutgoingMetadata(ctx, sc.md, sc.disableRouteToLeader), &sppb.CreateSessionRequest{
+		Database: sc.database,
+		// Multiplexed sessions do not support labels.


Either remove the labels from the input arguments of this method, or combine this method with the createSession(..) method and only conditionally add labels in the combined method.

Removed the labels

olavloite · 2024-07-19T06:33:36Z

spanner/test/opentelemetry/test/ot_metrics_test.go

@@ -192,7 +205,7 @@ func TestOTMetrics_SessionPool_SessionsCount(t *testing.T) {

 	client.Single().ReadRow(context.Background(), "Users", spanner.Key{"alice"}, []string{"email"})

-	attributesNumInUseSessions := append(getAttributes(client.ClientID()), attribute.Key("type").String("num_in_use_sessions"))
+	//attributesNumInUseSessions := append(getAttributes(client.ClientID()), attribute.Key("type").String("num_in_use_sessions"))


nit: remove commented code

olavloite · 2024-07-19T06:38:55Z

spanner/client_test.go

@@ -5208,7 +5208,7 @@ func TestClient_CloseWithUnresponsiveBackend(t *testing.T) {
 	server.TestSpanner.Freeze()
 	defer server.TestSpanner.Unfreeze()

-	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond)
+	ctx, cancel := context.WithTimeout(context.Background(), 20*time.Millisecond)


We should have tests that verify that:

Multiplexed sessions are used for single-use and multi-use read-only transactions (when enabled).

Regular sessions are used for read/write transactions (when multiplexed sessions are enabled).

Only one multiplexed session is created, even if there are more than 1 goroutine trying to execute a read-only transaction directly at startup before the pool has been initialized.

That multiplexed sessions are refreshed at the configured interval.

That if replacing a multiplexed session after the configured interval fails, it does not affect the existing multiplexed session. (That is: If the initial session creation succeeds, and replacing the session after 7 days fails, then the original session should continue to be used.)

1,2,3 => Added in TestClient_MultiplexedSession
4,5 => Added in TestMultiplexSessionWorker

olavloite · 2024-07-19T06:44:02Z

spanner/session.go

@@ -1462,6 +1666,11 @@ func (hc *healthChecker) markDone(s *session) {
 // healthCheck checks the health of the session and pings it if needed.
 func (hc *healthChecker) healthCheck(s *session) {
 	defer hc.markDone(s)
+	// If the session is multiplexed and has been idle for more than 7 days,
+	if s.isMultiplexed && s.createTime.Add(multiplexSessionIdleTime).Before(time.Now()) {


This should not be part of the standard health-check feature for regular sessions. This method is called by a worker that looks for sessions that need a ping. That logic is not the right logic that should determine whether we need to check whether the multiplexed session should be refreshed. It is probably better to put it in a separate method that only takes care of refreshing multiplexed sessions.
Also, we should ensure that:

The method (by default) does not need to be called very often. We are checking whether the session is more than 7 days old. That is something that can be checked at an interval of 10 minutes or something like that.

We should only have one createMultiplexedSession call in-flight at any time.

Created separate worker multiplexSessionWorker to refresh session with 10 minute interval.

Added test TestClient_MultiplexedSession to validate one in-flight req createMultiplexedSession at any time.

olavloite · 2024-07-26T12:46:01Z

spanner/client_test.go

+			},
+			validate: func(server InMemSpannerServer) {
+				// Validate the multiplexed session is used
+				expected := map[string]interface{}{


Here and in a couple of the other validations: Can we replace the expected map with just a simple uint variable, as that is the only thing that is put into the map?

olavloite · 2024-07-26T12:49:40Z

spanner/client_test.go

+				if !isMultiplexEnabled {
+					expected["SessionsCount"] = uint(25) // BatchCreateSession request from regular session pool
+				}
+				if !testEqual(expected["SessionsCount"], server.TotalSessionsCreated()) {


This check is going to be flaky in its current form (at least, I hope it will be), because:

We always initiate the creation of both regular and multiplexed sessions if multiplexed sessions are enabled directly at startup.

We by default create 4 * 25 regular sessions. This happens in the background.

If multiplexed sessions are enabled, then we also create one multiplexed session. This happens in the background.

Depending on what request is handled when, and whether mux sessions are enabled, then the number of sessions created will be 1, 25, 26, 50, 51, 75, 76, 100 or 101.

We always initiate the creation of both regular and multiplexed sessions if multiplexed sessions are enabled directly at startup.

Not true, regular sessions will only be created when number of requests waiting for regular session > minOpened, value for minOpened is 0 for setupMockedTestServer so as long as in test we only use ReadOnly txn it won't trigger regular session creation

We by default create 4 * 25 regular sessions. This happens in the background.

Not true, we only create regular sessions when there are pending requests for regular session given MinOpened=0 config, which is the case here.

Depending on what request is handled when, and whether mux sessions are enabled, then the number of sessions created will be 1, 25, 26, 50, 51, 75, 76, 100 or 101.

For the test cases either it will be 1(in case test is making R/O txn only) or 26(in case its doing R/W txn)

olavloite · 2024-07-26T12:56:33Z

spanner/client_test.go

+			},
+			validate: func(server InMemSpannerServer) {
+				// Validate the regular session is used
+				if !testEqual(uint(25), server.TotalSessionsCreated()) {


Note that also here the number of sessions is not deterministic, as we have 4 BatchCreateSessions requests running in parallel.

Note that also here the number of sessions is not deterministic, as we have 4 BatchCreateSessions requests running in parallel.

No, it will only create that many sessions which are needed which is 25(when mux disabled), 1 when enabled (because MinOpened=0 here)

olavloite · 2024-07-26T12:58:58Z

spanner/integration_test.go

@@ -386,6 +388,13 @@ func getInstanceConfig() string {
 	return os.Getenv("GCLOUD_TESTS_GOLANG_SPANNER_INSTANCE_CONFIG")
 }

+func getMultiplexEnableFlag() bool {
+	if os.Getenv("GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSIONS") == "true" {


nit: You can just do return os.Getenv("...") == "true"

olavloite · 2024-07-26T13:01:46Z

spanner/oc_test.go

+			"open_session_count": "25",
+		},
+		"true": {
+			"open_session_count": "1",


This here also surprises me a bit. Are we not creating any regular sessions if multiplexed sessions are enabled? I would have expected us to create both in parallel, as we need regular sessions for read/write transactions.

Since MinOpened in the test is 0 no we don't create regular session at start we wait for requests which need it.

olavloite · 2024-07-26T13:07:30Z

spanner/session.go

+	// multiplexedSessionInUse is the number of transactions using multiplexed sessions.
+	multiplexedSessionInUse uint64


nit:

Suggested change

// multiplexedSessionInUse is the number of transactions using multiplexed sessions.

multiplexedSessionInUse uint64

// numTransactionsUsingMultiplexedSession is the number of transactions using the multiplexed session.

numTransactionsUsingMultiplexedSession uint64

olavloite · 2024-07-26T13:08:14Z

spanner/session.go

+	// maxMultiplexedSessionInUse is the maximum number of multiplexed sessions in use concurrently in the
+	// current 10 minute interval.
+	maxMultiplexedSessionInUse uint64


Same here as above regarding naming.

But: Do we need these two numbers? What do they tell us and/or the user?

Removed this since it is not adding any real benefit to customer other than drop in InUse and maxInUse metrics on customer when using multiplexed session.

olavloite · 2024-07-26T14:08:58Z

spanner/session.go

+	if isMultiplexed {
+		// Ignore the error if multiplexed session already present
+		if p.multiplexedSession != nil {
+			p.multiplexedSession.checkingHealth = false


Do we need this?

olavloite · 2024-07-26T14:09:31Z

spanner/session.go

+		if p.multiplexedSession != nil {
+			p.multiplexedSession.checkingHealth = false
+			p.multiplexedSessionCreationError = nil
+			p.mayGetMultiplexedSession <- true


Is this needed? If there already was a multiplexed session, then there should be no waiters, meaning that we should also not need to send this signal, right?

yes this is to unblock maintainer to continue check, if we don't send here it will keep on waiting and maintainer will go blocked

olavloite · 2024-07-26T14:17:27Z

spanner/session.go

@@ -813,13 +882,53 @@ func (p *sessionPool) growPoolLocked(numSessions uint64, distributeOverChannels
 	return p.sc.batchCreateSessions(int32(numSessions), distributeOverChannels, p)
 }

+func (p *sessionPool) createMultiplexedSession(ctx context.Context) error {
+	for c := range p.multiplexedSessionsReq {


It took some time for me to understand what this does, but I think I now understand it:

During session pool initialization, we call go createMultiplexedSession(..).

That goroutine is then blocked on this for statement until there is a request for a multiplexed session.

The request can come from a read-only transaction (if multiplexed sessions are enabled), or from the background worker keeping the multiplexed session fresh.

That means that:

The multiplexed session creation is always on the critical path of the first read-only transaction.

The goroutine is stuck here forever if there is never a request for a multiplexed session.

I don't think that is what we would want. Instead, we should try to:

Always start the creation of a multiplexed session as soon as possible if multiplexed sessions are enabled. This means that the creation will not be on the critical path of the first read-only transaction.

Not call this method at all if multiplexed sessions are disabled.

I think that many of my above comments are also a direct consequence of this behavior.

Always start the creation of a multiplexed session as soon as possible if multiplexed sessions are enabled. This means that the creation will not be on the critical path of the first read-only transaction.

I updated to trigger createMultiplexedSession upon session pool init

Not call this method at all if multiplexed sessions are disabled.

Updated, now we spawn this background thread only when mux is enabled

…Transactions

rahul2393 requested review from a team as code owners May 28, 2024 05:40

rahul2393 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 28, 2024

product-auto-label bot added the api: spanner Issues related to the Spanner API. label May 28, 2024

rahul2393 force-pushed the multiplexed-sessions branch 4 times, most recently from 09cb87e to cf1ac68 Compare June 3, 2024 05:29

rahul2393 force-pushed the multiplexed-sessions branch from cf1ac68 to cab67e9 Compare June 4, 2024 06:28

rahul2393 removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 4, 2024

rahul2393 force-pushed the multiplexed-sessions branch 4 times, most recently from 09d5513 to 89c64c2 Compare June 5, 2024 09:23

rahul2393 force-pushed the multiplexed-sessions branch 6 times, most recently from b381f6c to 703ffae Compare July 9, 2024 10:51

olavloite reviewed Jul 18, 2024

View reviewed changes

rahul2393 force-pushed the multiplexed-sessions branch 2 times, most recently from 0da6099 to 2e24f7e Compare July 19, 2024 05:40

olavloite reviewed Jul 19, 2024

View reviewed changes

rahul2393 force-pushed the multiplexed-sessions branch 2 times, most recently from 8a0e84c to fcb133a Compare July 23, 2024 13:51

rahul2393 requested a review from olavloite July 23, 2024 13:56

olavloite reviewed Jul 26, 2024

View reviewed changes

feat(spanner): add support of using multiplexed session with ReadOnly…

21c837f

…Transactions

rahul2393 added 12 commits August 1, 2024 15:02

fallback to regular sessions if backend returns Unimplemented error

8de2d37

add metic label for is_multiplexed filter

2849e1c

fix integration tests

e41d900

refresh multiplex session asynchronously

8d1ac64

fix env

37023f5

fix permission issue

e9eb4ba

fix presubmit file

2ff8613

fixed tests

fc55bc9

incorporate changes

1602470

fix tests

c835989

incorporate changes

0b63e8d

fix vet

c58eff5

rahul2393 force-pushed the multiplexed-sessions branch from 90fff40 to c58eff5 Compare August 1, 2024 09:34

rahul2393 added 2 commits August 2, 2024 11:47

fix vet

ddfab86

fix tests

6356f0e

rahul2393 requested a review from olavloite August 2, 2024 06:21

remove idempotent assignment

a1160db

	// multiplexedSessions contains the multiplexed sessions
	// multiplexedSession contains the multiplexed session

	go p.sc.executeCreateMultiplexedSessions(ctx, client, p.sc.sessionLabels, p.sc.md, p)
	go p.sc.executeCreateMultiplexedSession(ctx, client, p.sc.sessionLabels, p.sc.md, p)

	trace.TracePrintf(ctx, nil, "Session client closed while creating multiplexed sessions: %v", err)
	trace.TracePrintf(ctx, nil, "Session client closed while creating a multiplexed session: %v", err)

		// multiplexedSessionInUse is the number of transactions using multiplexed sessions.
		multiplexedSessionInUse uint64

feat(spanner): add support of using multiplexed session with ReadOnlyTransactions #10269

Are you sure you want to change the base?

feat(spanner): add support of using multiplexed session with ReadOnlyTransactions #10269

Conversation

rahul2393 commented May 28, 2024 • edited Loading

rahul2393 commented Jul 15, 2024

olavloite left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahul2393 Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahul2393 Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahul2393 Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

rahul2393 commented May 28, 2024 •

edited

Loading

rahul2393 Aug 1, 2024 •

edited

Loading

rahul2393 Aug 1, 2024 •

edited

Loading

rahul2393 Aug 1, 2024 •

edited

Loading