instancestorage: read instances in a transaction #95800

healthy-pod · 2023-01-25T00:27:36Z

This code change adds (instancestorage.Reader).GetAllInstancesUsingTxn to read all instances using the given *kv.Txn.

Release note: None
Epic: CRDB-18735

cockroach-teamcity · 2023-01-25T00:27:47Z

This change is

ajwerner · 2023-01-26T16:22:16Z

pkg/sql/sqlinstance/instancestorage/instancereader.go

+func (r *Reader) getAllUniqueLiveInstances(
+	ctx context.Context, rows []instancerow,
+) ([]instancerow, error) {
+	instancesSet := make(map[base.SQLInstanceID]struct{}, len(rows))


I don't think this is needed. See the logic below after the sort.

ajwerner · 2023-01-26T16:30:55Z

pkg/sql/sqlinstance/instancestorage/instancereader_test.go

 				instances, err := reader.GetAllInstances(ctx)
 				if err != nil {
 					return err
 				}
 				sortInstances(instances)
-				return testOutputFn(instanceIDs, rpcAddresses, sqlAddresses, sessionIDs, localities, instances)
+				if err := testOutputFn(instanceIDs, rpcAddresses, sqlAddresses, sessionIDs, localities, instances); err != nil {
+					return err
+				}
+				instancesUsingTxn, err := getInstancesUsingTxnWrapper()
+				if err != nil {
+					return err
+				}
+				return testOutputFn(instanceIDs, rpcAddresses, sqlAddresses, sessionIDs, localities, instancesUsingTxn)


how about a helper to remove some of this boilerplate and to make the test easier to read?
Firstly, let's make a structure to pass to testOutputFn, let's call it expectations. Then, let's make a helper

type expectations struct { /* ... */ } testOutputFn := func(exp expectations, actualInstances []sqlinstance.InstanceInfo) { /* ... */ } expectationsFromOffset := func(offset int) expectations { return expectations{ instanceIDs: instanceIDs[offset:], rpcAddresses: rpcAddresses[offset:], sqlAddresses: sqlAddresses[offset:], sessionIDs: sessionIDs[offset:], localities: localities[offset:] } } // ... verifyInstancesWithGetter := func(t *testing.T, name string, exp expectations, getInstances func()([]sqlinstance.InstanceInfo, error)) (retErr error) { defer func() { errors.Wrap(retErr, name) } instances, err := getInstances() if err != nil { return err } sortInstances(instances) return testOutputFn(exp, instances) } verifyInstances := func(t *testing.T, exp expectations) { if err := verifyInstancesWithGetter(t, "reader", offset, func() ([]sqlinstance.InstanceInfo, error) { return reader.GetAllInstances(ctx) }); err != nil { return err } return verifyInstancesWithGetter(t, "txn", offset, func() ([]sqlinstance.InstanceInfo, error) { return getInstancesUsingTxn(t) }) }

ajwerner · 2023-01-26T16:31:23Z

pkg/sql/sqlinstance/instancestorage/instancereader_test.go

@@ -175,6 +175,15 @@ func TestReader(t *testing.T) {
 			}
 			return nil
 		}
+		getInstancesUsingTxnWrapper := func() ([]sqlinstance.InstanceInfo, error) {


nit: you should take t as a parameter to deal with future refactorings and subtests. Also, I don't think the Wrapper suffix adds value; just call it getInstancedUsingTxn

ajwerner · 2023-01-26T16:31:50Z

pkg/sql/sqlinstance/instancestorage/instancereader_test.go

@@ -175,6 +175,15 @@ func TestReader(t *testing.T) {
 			}
 			return nil
 		}
+		getInstancesUsingTxnWrapper := func() ([]sqlinstance.InstanceInfo, error) {
+			txn := s.DB().NewTxn(ctx, "testing GetAllInstancesUsingTxn")


NewTxn is low-level and generally to be avoided. Instead you ought to use s.DB().Txn

ajwerner · 2023-01-26T16:32:16Z

pkg/sql/sqlinstance/instancestorage/instancereader_test.go

+			if err != nil {
+				return nil, err
+			}
+			sortInstances(instancesUsingTxn)


leave this for later. It'll make the next helper simpler.

healthy-pod · 2023-01-26T20:16:18Z

Ready for a second pass

ajwerner

LGTM, the comments are all nits

ajwerner · 2023-01-26T22:22:02Z

pkg/sql/sqlinstance/instancestorage/instancereader.go

+	instances := make([]sqlinstance.InstanceInfo, 0, len(rows))
+	for _, row := range filteredRows {
+		instanceInfo := sqlinstance.InstanceInfo{
+			InstanceID:      row.instanceID,
+			InstanceRPCAddr: row.rpcAddr,
+			InstanceSQLAddr: row.sqlAddr,
+			SessionID:       row.sessionID,
+			Locality:        row.locality,
+		}
+		instances = append(instances, instanceInfo)
+	}
+	return instances, nil


nit: extract a helper to convert the []instancerow into []sqlinstance.InstanceInfo. I'd probably also extract a constructor for a single InstanceInfo from a row like makeInstanceInfo(row instancerow) sqlinstance.InstanceInfo so that is' like:

func makeInstanceInfos(rows []instancerow) []sqlinstance.InstanceInfo { ret := make([]sqlinstance.InstanceInfo, len(rows)) for i := range rows { ret[i] = makeInstanceInfo(rows[i]) } return ret }

ajwerner · 2023-01-26T22:29:50Z

pkg/sql/sqlinstance/instancestorage/instancereader.go

@@ -225,8 +264,9 @@ func (r *Reader) GetAllInstances(
 	return sqlInstances, nil
 }

-func (r *Reader) getAllLiveInstances(ctx context.Context) ([]instancerow, error) {
-	rows := r.getAllInstanceRows()
+func (r *Reader) getAllUniqueLiveInstances(


uber-nit: I think I'd take this off of the *Reader and make it a free standing function which takes a sqlliveness.Reader. I'd probably call it selectDistinctLiveRows(context.Context, sqlliveness.Reader, []instanceRow) ([]instancerow, error)

Note also that the function will modify the slice in place.

ajwerner · 2023-01-26T22:30:20Z

pkg/sql/sqlinstance/instancestorage/instancereader.go

+// GetAllInstancesUsingTxn reads all instances using the given transaction and returns
+// live instances only.
+func (r *Reader) GetAllInstancesUsingTxn(
+	ctx context.Context, tenantCodec keys.SQLCodec, tableID catid.DescID, txn *kv.Txn,


nit: everywhere else we just call this codec

ajwerner · 2023-01-26T22:32:19Z

pkg/sql/sqlinstance/instancestorage/instancereader_test.go

-				}
-				sortInstances(instances)
-				return testOutputFn(
+				return verifyInstances(t, expectations{


I'd expect the linter to want field names, generally our style is to write them

I don't think we lint against that although it'd be nice. I didn't add them initially because I assumed the types would be enough.

healthy-pod · 2023-01-26T23:33:37Z

TFTR!

This code change adds `(instancestorage.Reader).GetAllInstancesUsingTxn` to read all instances using the given `*kv.Txn`. Release note: None Epic: CRDB-18735

healthy-pod · 2023-01-27T01:39:26Z

bors r=ajwerner

craig · 2023-01-27T07:15:11Z

Build succeeded:

Bazel Essential CI (Cockroach)

healthy-pod requested review from ajwerner, jeffswenson and ajstorm January 25, 2023 00:27

ajwerner reviewed Jan 26, 2023

View reviewed changes

healthy-pod force-pushed the transactional-instances-read branch 2 times, most recently from 068e40e to 694d363 Compare January 26, 2023 20:14

healthy-pod requested a review from ajwerner January 26, 2023 20:16

healthy-pod force-pushed the transactional-instances-read branch from 694d363 to 07111ed Compare January 26, 2023 20:46

ajwerner approved these changes Jan 26, 2023

View reviewed changes

healthy-pod force-pushed the transactional-instances-read branch from 07111ed to df9ff31 Compare January 26, 2023 23:32

instancestorage: read instances in a transaction

b88b17c

This code change adds `(instancestorage.Reader).GetAllInstancesUsingTxn` to read all instances using the given `*kv.Txn`. Release note: None Epic: CRDB-18735

healthy-pod force-pushed the transactional-instances-read branch from df9ff31 to b88b17c Compare January 27, 2023 01:38

craig bot merged commit efb4481 into cockroachdb:master Jan 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

instancestorage: read instances in a transaction #95800

instancestorage: read instances in a transaction #95800

healthy-pod commented Jan 25, 2023

cockroach-teamcity commented Jan 25, 2023

ajwerner Jan 26, 2023

ajwerner Jan 26, 2023

healthy-pod Jan 26, 2023

ajwerner Jan 26, 2023

ajwerner Jan 26, 2023

ajwerner Jan 26, 2023

healthy-pod commented Jan 26, 2023

ajwerner left a comment

ajwerner Jan 26, 2023

ajwerner Jan 26, 2023

ajwerner Jan 26, 2023

ajwerner Jan 26, 2023

ajwerner Jan 26, 2023

healthy-pod Jan 26, 2023

healthy-pod commented Jan 26, 2023

healthy-pod commented Jan 27, 2023

craig bot commented Jan 27, 2023

instancestorage: read instances in a transaction #95800

instancestorage: read instances in a transaction #95800

Conversation

healthy-pod commented Jan 25, 2023

cockroach-teamcity commented Jan 25, 2023

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

healthy-pod commented Jan 26, 2023

ajwerner 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

healthy-pod commented Jan 26, 2023

healthy-pod commented Jan 27, 2023

craig bot commented Jan 27, 2023