Conversation
product-auto-label
bot
added
size: m
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Google Cloud Spanner client library examples by introducing a new demonstration of Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 adds a helpful example for using MutableCredentials to handle credential rotation for long-running Spanner clients. The example is clear and demonstrates the key steps involved.
I've added a few review comments to improve the example:
- A suggestion to use
AtomicReferencefor better code clarity. - A critical fix to make the example correctly simulate a long-running process, as it currently shuts down immediately.
Additionally, it would be beneficial to add an integration test for this new example in SpannerStandaloneExamplesIT.java to ensure it runs correctly and prevent future regressions.
| try (Spanner spanner = options.getService(); | ||
| DatabaseAdminClient databaseAdminClient = spanner.createDatabaseAdminClient()) { | ||
| // Perform operations... | ||
| // long running client operations will always use the latest credentials wrapped in | ||
| // mutableCredentials | ||
| } finally { | ||
| // Ensure the executor is shut down when the application exits or the client is closed | ||
| executorService.shutdown(); | ||
| } |
There was a problem hiding this comment.
This example is intended to demonstrate credential updates for a long-running client, but the Spanner client and the ScheduledExecutorService are shut down immediately after creation because the try-with-resources block is empty. This prevents the credential rotation logic from ever running in a meaningful way.
To make the example more illustrative, consider adding a TimeUnit.sleep() within the try block to simulate a long-running process. This will keep the client and the background thread alive long enough to demonstrate the credential update mechanism.
// 5. Use the client
try (Spanner spanner = options.getService();
DatabaseAdminClient databaseAdminClient = spanner.createDatabaseAdminClient()) {
// Perform operations...
// long running client operations will always use the latest credentials wrapped in
// mutableCredentials.
// This example demonstrates this by keeping the client alive for 60 seconds.
System.out.println("Client created. It will remain active for 60 seconds.");
try {
TimeUnit.SECONDS.sleep(60);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}
System.out.println("60 seconds passed. Closing client.");
} finally {
// Ensure the executor is shut down when the application exits or the client is closed
executorService.shutdown();
}| // Use an array to hold the mutable lastModifiedTime so it can be accessed in the lambda | ||
| FileTime[] lastModifiedTime = new FileTime[] {Files.getLastModifiedTime(path)}; |
There was a problem hiding this comment.
For better clarity and to use modern Java concurrency utilities, consider using java.util.concurrent.atomic.AtomicReference instead of a single-element array to hold a mutable reference. This makes the intent of sharing a mutable object with the lambda more explicit. This change should be applied along with an update to the lambda that uses this variable.
| // Use an array to hold the mutable lastModifiedTime so it can be accessed in the lambda | |
| FileTime[] lastModifiedTime = new FileTime[] {Files.getLastModifiedTime(path)}; | |
| // Use an AtomicReference to hold the mutable lastModifiedTime so it can be accessed in the lambda | |
| final java.util.concurrent.atomic.AtomicReference<FileTime> lastModifiedTime = | |
| new java.util.concurrent.atomic.AtomicReference<>(Files.getLastModifiedTime(path)); |
| if (currentModifiedTime.compareTo(lastModifiedTime[0]) > 0) { | ||
| lastModifiedTime[0] = currentModifiedTime; |
There was a problem hiding this comment.
As part of refactoring to use AtomicReference for lastModifiedTime, you'll need to update this code to use get() and set() to access and modify the value.
| if (currentModifiedTime.compareTo(lastModifiedTime[0]) > 0) { | |
| lastModifiedTime[0] = currentModifiedTime; | |
| if (currentModifiedTime.compareTo(lastModifiedTime.get()) > 0) { | |
| lastModifiedTime.set(currentModifiedTime); |
2 participants
This adds an example of how to use MutableCredentials to update credentials on a long running client if the timestamp on a service account credentials file has changed.