perf(spanner-jdbc): cache JDBC metadata query strings - #14041
Conversation
Cache the SQL strings that are loaded from disk for the standard JDBC metadata queries.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces caching for SQL queries loaded from files in JdbcDatabaseMetaData using a ConcurrentHashMap to improve performance, and adds a corresponding unit test to verify the caching behavior. It also updates the file reading logic to use UTF-8 explicitly and adds a null check for the resource stream. The reviewer suggests managing all closeable resources (InputStream, InputStreamReader, and Scanner) within a try-with-resources block to prevent potential resource leaks and notes that BufferedReader is redundant.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces caching for SQL files read from resources in JdbcDatabaseMetaData using a ConcurrentHashMap to avoid redundant file I/O operations, and adds corresponding unit tests. The review feedback suggests optimizing the cache lookup by performing a fast get check before calling computeIfAbsent to prevent unnecessary lambda allocations on cache hits.
14ca10f to
40d5d98
Compare
olavloite
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces SQL file caching using a ConcurrentHashMap in JdbcDatabaseMetaData to optimize metadata queries, along with corresponding unit tests. It also updates the MyBatis sample tests to spin up a Spanner emulator container using Testcontainers. The review feedback suggests avoiding blocking I/O inside computeIfAbsent to prevent thread contention, removing the alwaysPull policy on the emulator container to leverage local caches, and clearing modified system properties in the test cleanup phase to ensure JVM isolation.
| builder.append(line).append("\n"); | ||
| } | ||
| } catch (IOException e) { | ||
| throw SpannerExceptionFactory.newSpannerException( |
There was a problem hiding this comment.
nit: earlier there was a chance of NPE, and now better
Just wanted to fyi since the PR title did not mention it
| private static final String PRODUCT_NAME = "Google Cloud Spanner"; | ||
| private static final String POSTGRESQL_PRODUCT_NAME = PRODUCT_NAME + " PostgreSQL"; | ||
|
|
||
| private static final ConcurrentMap<String, String> SQL_CACHE = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Cache is unbounded but negligible risk since metadata are bounded.
3 participants
Cache the SQL strings that are loaded from disk for the standard JDBC metadata queries.