feat: migrate connection API to use the new auto-generated admin clients. #2879
Conversation
arpan14
commented
Feb 12, 2024
•
arpan14
commented
- Connection API internally uses the handwritten admin client earlier. Migrating them to new auto-generated admin clients which we would we recommending to customers.
- Ensuring that all tests pass and we don't see any surprises.
…od while retrying exceptions in unit tests. * For details on issue see - googleapis#2206
fix: prevent illegal negative timeout values into thread sleep() method while retrying exceptions in unit tests.
product-auto-label
bot
added
the
api: spanner
arpan14
force-pushed
the
admin-connection-api
branch
from
2b5f0c0 to
53bcb3e
Compare
product-auto-label
bot
added
size: u
product-auto-label
bot
added
size: m
arpan14
added
the
owlbot:run
gcf-owl-bot
bot
removed
the
owlbot:run
arpan14
added
the
owlbot:run
gcf-owl-bot
bot
removed
the
owlbot:run
| * | ||
| * @return the SQL dialect that is used by the database. | ||
| */ | ||
| default DatabaseDialect getDatabaseDialect() { |
There was a problem hiding this comment.
Do we really need this? I think it will be confusing for customers when to use which of these two methods (getDialect() and getDatabaseDialect()). Could we instead instruct users to use Dialect.toProto() instead when they need the proto version?
| @@ -273,7 +275,8 @@ static UnitOfWorkType of(TransactionMode transactionMode) { | |||
| this.autoPartitionMode = options.isAutoPartitionMode(); | |||
| this.maxPartitions = options.getMaxPartitions(); | |||
| this.maxPartitionedParallelism = options.getMaxPartitionedParallelism(); | |||
| this.ddlClient = createDdlClient(); | |||
| this.databaseAdminClient = spanner.createDatabaseAdminClient(); | |||
There was a problem hiding this comment.
This will create a separate admin client for each connection that is created, including underlying gRPC channels. That means that an application that uses a JDBC connection that opens 100 connections, all of a sudden uses 4 channels (for the normal Spanner instance) + 100 (or 400? I don't remember exactly what channel settings we are using for the generated admin clients) channels for the 100 DatabaseAdmin clients.
It also means that creating a new Connection is suddenly a much heavier operation, meaning that applications that do not use connection pooling could potentially experience a performance degradation.
Currently, creating a Connection instance is an non-blocking method, and creating multiple connections to the same database does not incur much cost (it only costs the bit of memory that is required to keep track of the Connection state, but it does not create any additional physical connections to Spanner).
| databaseAdminClient.close(); | ||
| } | ||
| } catch (RuntimeException ex) { | ||
| throw SpannerExceptionFactory.newSpannerException(ex); |
There was a problem hiding this comment.
Are we sure that we want to propagate this exception? Or should we just ignore it? Are there any potential things that the user might have done wrong that could cause an exception here? If yes, then it is probably best to propagate it. If this is more of a 'better-safe-than-sorry' and/or 'this sometimes randomly causes an exception, no idea why'-kind of situation, then I would suggest that we ignore the exception.
But see also above: I don't think we should create a separate DatabaseAdminClient for each connection that is created.
|
Closing as there has not been any activity in the past several weeks. Please re-open if this is still relevant. |
