Allow providing Spanner client

[iangudger]

This library would be easier to integrate if it had an option to construct it with an existing Spanner client.

[yfuruyama]

Hi @iangudger What is the use case for providing an existing Spanner client?

You can pass spanner.ClientConfig and []option.ClientOption when calling changestreams.NewReaderWithConfig to customize the Spanner client: https://pkg.go.dev/github.com/cloudspannerecosystem/spanner-change-streams-tail/changestreams#NewReaderWithConfig

[iangudger]

A common pattern in Go is creating clients in main and pass them to constructors. That way test code can inject dependencies. Requiring that this library creates its own client requires managing a new type of dependency (including production and test constructors and plumbing).

Is there a reason why it is beneficial for this library to have a dedicated Spanner client?

[yfuruyama]

Is there a reason why it is beneficial for this library to have a dedicated Spanner client?

The usage of Spanner client is different between normal transactional requests and change streams, so I designed the Go package to create a dedicated Spanner client for change streams.

Specifically, change streams client tends to consume more Spanner sessions to read the change streams partitions in parallel. So if the same client is used in both transactional requests and change streams, it might be a problem.

[iangudger]

Thanks for explaining. It might be worth adding a comment in the code.

[majelbstoat]

I agree that change streams shouldn't use the main client, but this feels a little weird to me in applications that need multiple readers. Is it a deliberate choice that there's a new client for every reader in this case?

In one application, have multiple discrete components that each care about certain tables. I have a change stream for each. I would have expected something like:

client := changestreams.NewClient(projectID, instanceID, databaseID)

reader := client.NewReader(ctx, streamName)
go reader.Read(ctx, callback)

otherReader := client.NewReader(ctx, otherStreamName)
go otherReader.Read(ctx, callback)

instead, every call to NewReader creates a new client. I don't know whether it's actually problematic, but it's certainly more things for the library to keep track of, keep open, and close down.

Rather than restructuring the library like that, this could also be mitigated by providing NewReaderWithClient and allowing the user to make a choice about which one to pass in.

[yfuruyama]

Yeah that totally makes sense > multiple streams in one application.

I'm open for pull request, so please feel free to add NewReaderWithClient function.

Probably we also need to modify reader.Close() since it's currently closes the enclosing client. If multiple readers share the same client, it would be a problem if one of them closes the client.

[majelbstoat]

thanks :)

maybe a flag on reader like userManagedClient bool? and then if that is set, making reader.Close() a no-op, or a straight up error. If the user is responsible for passing in the client, they should also be responsible for its closure, i think. does that sound ok?

(otherwise, we have to create a common place where the shared client is stored, and keep track of how many readers are using it etc, which feels overly complex.)

[yfuruyama]

Yes, that sounds very reasonable to me! :)