feat(spanner): add support of using multiplexed session with ReadOnlyTransactions by rahul2393 · Pull Request #10269 · googleapis/google-cloud-go

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

 	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

 	// 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

 	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

 	// 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)

spanner/sessionclient.go

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

spanner/test/opentelemetry/test/ot_metrics_test.go

 	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

spanner/client_test.go

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

spanner/session.go

 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

 }

+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

 }

+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

olavloite · 2024-08-06T08:48:02Z

spanner/client_test.go

+					expectedSessionCount = uint(25) // BatchCreateSession request from regular session pool
+				}
+				if !testEqual(expectedSessionCount, server.TotalSessionsCreated()) {
+					t.Errorf("TestClient_MultiplexedSession expected session creation with multiplexed=%s should be=%v, got: %v", strconv.FormatBool(isMultiplexEnabled), 25, server.TotalSessionsCreated())


nit:

Suggested change

t.Errorf("TestClient_MultiplexedSession expected session creation with multiplexed=%s should be=%v, got: %v", strconv.FormatBool(isMultiplexEnabled), 25, server.TotalSessionsCreated())

t.Errorf("TestClient_MultiplexedSession expected session creation with multiplexed=%s should be=%v, got: %v", strconv.FormatBool(isMultiplexEnabled), expectedSessionCount, server.TotalSessionsCreated())

olavloite · 2024-08-06T08:50:43Z

spanner/client_test.go

+					switch s.(type) {
+					case *sppb.ReadRequest:
+						req, _ := s.(*sppb.ReadRequest)
+						// Validate the session is not multiplexed


Suggested change

// Validate the session is not multiplexed

// Verify that a multiplexed session is used when that is enabled.

olavloite · 2024-08-06T09:02:43Z

spanner/oc_test.go

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

+	expectedSpans := 2
+	if isMultiplexEnabled {


nit: Would you mind adding a comment here that explains which span will be skipped when multiplexed sessions are enabled?

olavloite · 2024-08-06T09:04:10Z

spanner/oc_test.go

 }

 func TestOCStats_SessionPool_GetSessionTimeoutsCount(t *testing.T) {
+	if isMultiplexEnabled {


I don't think we should skip this test when multiplexed sessions are enabled (or otherwise, we should add a separate test for multiplexed sessions). This test verifies the behavior of the client if for whatever reason the creation of session(s) is taking longer than expected. That could also happen with multiplexed sessions.

olavloite · 2024-08-06T09:07:03Z

spanner/session.go

 	// Defaults to 50m.
 	HealthCheckInterval time.Duration

+	// MultiplexSessionCheckInterval is the interval at which the multiplexed session is refreshed.


Suggested change

// MultiplexSessionCheckInterval is the interval at which the multiplexed session is refreshed.

// MultiplexSessionCheckInterval is the interval at which the multiplexed session is checked whether it needs to be refreshed.

olavloite · 2024-08-06T12:52:29Z

spanner/session.go

 // 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,


nit: remove comment

olavloite · 2024-08-06T12:55:04Z

spanner/session.go

+			createTime = hc.pool.multiplexedSession.createTime
+		}
+		hc.pool.mu.Unlock()
+		ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)


Do I understand it correctly that this is the deadline that we will use for creating a new multiplexed session? If so, then we should increase the timeout significantly. Maybe 30 seconds?

olavloite · 2024-08-06T12:58:34Z

spanner/session_test.go

+			SessionPoolConfig: SessionPoolConfig{
+				MaxIdle:                   10,
+				MaxOpened:                 10,
+				healthCheckSampleInterval: 10 * time.Millisecond,


Is this change needed?

Yes because of this unit tests are taking several minutes to complete, change is unrelated to the feature but is an improvement.

olavloite · 2024-08-06T13:01:49Z

spanner/session_test.go

+	// Will cause session creation RPC to be fail.
+	server.TestSpanner.PutExecutionTime(MethodCreateSession,
+		SimulatedExecutionTime{
+			Errors:    []error{status.Errorf(codes.Unavailable, "try later")},


Unavailable will be retried by Gax, so it won't bubble up to the client library. Instead, it will just seem like it takes a longer time. So I think that the error code that is used by this test should be something else.

olavloite · 2024-08-06T13:04:49Z

spanner/sessionclient_test.go

+				if isMultiplexEnabled {
+					// The multiplexed session creation will use one of the connections
+					if c != 2 {
+						t.Fatalf("connection %q used an unexpected number of times\ngot: %v\nwant %v", a, c, 1)


Suggested change

t.Fatalf("connection %q used an unexpected number of times\ngot: %v\nwant %v", a, c, 1)

t.Fatalf("connection %q used an unexpected number of times\ngot: %v\nwant %v", a, c, 2)

🤖 I have created a release *beep* *boop* --- ## [1.66.0](https://togithub.com/googleapis/google-cloud-go/compare/spanner/v1.65.0...spanner/v1.66.0) (2024-08-07) ### Features * **spanner/admin/database:** Add support for Cloud Spanner Incremental Backups ([d949cc0](https://togithub.com/googleapis/google-cloud-go/commit/d949cc0e5d44af62154d9d5fd393f25a852f93ed)) * **spanner:** Add support of multiplexed session support in writeAtleastOnce mutations ([#10646](https://togithub.com/googleapis/google-cloud-go/issues/10646)) ([54009ea](https://togithub.com/googleapis/google-cloud-go/commit/54009eab1c3b11a28531ad9e621917d01c9e5339)) * **spanner:** Add support of using multiplexed session with ReadOnlyTransactions ([#10269](https://togithub.com/googleapis/google-cloud-go/issues/10269)) ([7797022](https://togithub.com/googleapis/google-cloud-go/commit/7797022e51d1ac07b8d919c421a8bfdf34a1d53c)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).

rahul2393 requested review from a team 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

olavloite approved these changes Aug 6, 2024

View reviewed changes

rahul2393 and others added 4 commits August 7, 2024 01:04

incorporate changes

3b45b8e

fix integration test for TestIntegration_DbRemovalRecovery

7941056

add TODO comment for TestIntegration_DbRemovalRecovery integration tests

0a746f9

Merge branch 'main' into multiplexed-sessions

6cb0b11

rahul2393 merged commit 7797022 into main Aug 7, 2024

rahul2393 deleted the multiplexed-sessions branch August 7, 2024 05:01

release-please bot mentioned this pull request Aug 7, 2024

chore(main): release spanner 1.66.0 #10614

Merged

	// 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

	t.Errorf("TestClient_MultiplexedSession expected session creation with multiplexed=%s should be=%v, got: %v", strconv.FormatBool(isMultiplexEnabled), 25, server.TotalSessionsCreated())
	t.Errorf("TestClient_MultiplexedSession expected session creation with multiplexed=%s should be=%v, got: %v", strconv.FormatBool(isMultiplexEnabled), expectedSessionCount, server.TotalSessionsCreated())

	// Validate the session is not multiplexed
	// Verify that a multiplexed session is used when that is enabled.

	// MultiplexSessionCheckInterval is the interval at which the multiplexed session is refreshed.
	// MultiplexSessionCheckInterval is the interval at which the multiplexed session is checked whether it needs to be refreshed.

	t.Fatalf("connection %q used an unexpected number of times\ngot: %v\nwant %v", a, c, 1)
	t.Fatalf("connection %q used an unexpected number of times\ngot: %v\nwant %v", a, c, 2)

Conversation

rahul2393 commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rahul2393 commented Jul 15, 2024

Uh oh!

olavloite left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rahul2393 Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rahul2393 Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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