-
Notifications
You must be signed in to change notification settings - Fork 258
feat: Implementation of a session service for the ADK using Firestore #578
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Implementation of a session service for the ADK using Firestore #578
Conversation
Summary of ChangesHello @mohan-ganesh, 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 delivers a significant new feature to the Agent Development Kit (ADK) by providing a robust and scalable session management solution using Google Firestore. It allows ADK applications to persist user conversation sessions, manage application and user-specific state, and perform memory searches on past interactions. This enhancement provides developers with a powerful tool for building stateful and intelligent agents. Highlights
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant new feature: a session and memory service implementation using Google Firestore. The implementation is comprehensive, including the core services, a runner, configuration management, and a full suite of tests. The overall structure is well-designed. However, there are a few critical and high-severity issues that must be addressed. Most importantly, there's a resource leak in ApiFutureUtils due to an un-shutdown executor, which will prevent the application from terminating. Several classes also inefficiently create new executors on each method call. I've also included several medium-severity suggestions to improve code robustness, consistency, and maintainability, such as migrating to JUnit 5, improving the session deletion logic, and hardening the singleton pattern and type casting.
contrib/firestore-session-service/src/main/java/com/google/adk/utils/ApiFutureUtils.java
Outdated
Show resolved
Hide resolved
...ib/firestore-session-service/src/main/java/com/google/adk/memory/FirestoreMemoryService.java
Outdated
Show resolved
Hide resolved
...firestore-session-service/src/main/java/com/google/adk/sessions/FirestoreSessionService.java
Outdated
Show resolved
Hide resolved
| // 1. Delete all events in the subcollection | ||
| CollectionReference eventsRef = sessionRef.collection(EVENTS_SUBCOLLECTION_NAME); | ||
| com.google.api.core.ApiFuture<com.google.cloud.firestore.QuerySnapshot> eventsQuery = | ||
| eventsRef.get(); | ||
| List<QueryDocumentSnapshot> eventDocuments = eventsQuery.get().getDocuments(); | ||
| List<ApiFuture<WriteResult>> deleteFutures = new ArrayList<>(); | ||
| for (QueryDocumentSnapshot doc : eventDocuments) { | ||
| deleteFutures.add(doc.getReference().delete()); | ||
| } | ||
| // Wait for all event deletions to complete | ||
| if (!deleteFutures.isEmpty()) { | ||
| ApiFutures.allAsList(deleteFutures).get(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation deletes documents in the events subcollection one by one after fetching them. This is inefficient and can be slow and costly for sessions with many events. It's also not atomic. You should use batched writes (WriteBatch) to delete documents in chunks of up to 500. This will be much more performant and cost-effective.
contrib/firestore-session-service/src/main/java/com/google/adk/utils/Constants.java
Outdated
Show resolved
Hide resolved
| /** The template for the environment-specific property file name. */ | ||
| private static final String ENV_PROPERTY_FILE_TEMPLATE = "adk-firestore-%s.properties"; | ||
|
|
||
| private static volatile FirestoreProperties INSTANCE = new FirestoreProperties(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The singleton INSTANCE is eagerly initialized here, but the getInstance() method uses a double-checked locking pattern, which is meant for lazy initialization. This is confusing and unnecessarily complex. Since the instance is already created, you can simplify the getInstance() method to just return INSTANCE;.
...store-session-service/src/test/java/com/google/adk/sessions/FirestoreSessionServiceTest.java
Show resolved
Hide resolved
46cae9b to
750a4b2
Compare
glaforge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very solid PR!
It's a big one as well, and I'm not very familiar with the Firestore APIs, but on the surface it looks very good.
Gemini Code Assist made a few suggestions you might want to address (on executors, for example, and some volatile fields...) so it'd be good to have a look at some of them which make sense.
thanks for taking a look at this PR @glaforge .. all of the identified suggetions are already updated and committed . i just resolved all of the open comments. |
f4b8db8 to
59bd22f
Compare
…ent Kit) that uses Google Firestore as the backend for storing session data.
59bd22f to
f6f8ef2
Compare
|
Hi @glaforge , the earlier build was fine but ran in to merge conflicts. I've force-pushed an update that resolves the merge conflicts and build issues. The branch should now be clean and ready for merging. Could you please take another look when you have a moment? Thanks! |
This pull request adds
FirestoreSessionServiceby implementingBaseSessionServiceinterface that uses Firestore as the data store to manage user sessions.Added the feature to contrib folder
here is the discussion request link https://github.com/google/adk-java/discussions/568
Why Firestore:
Firestore is a scalable, managed, and widely-used Google Cloud database. It would provide a clear, simple, and robust path for developers needing persistent sessions in production scale
Transactional Design Consideration
The
createSession,appendEvent, anddeleteSessionmethods in this service use blocking.get()calls on their FirestoreApiFutureobjects. This is a deliberate design choice to guarantee data consistency within the ADK's sequential execution flow.The
appendEventmethod is particularly critical. A fully non-blocking implementation could create a race condition where theRunnerproceeds to the next step (e.g., calling the LLM) before the latest event and state changes are persisted. This would result in the model operating on stale data, leading to an incomplete chat history.By blocking within the reactive stream, we ensure that the database transaction is complete before the
SingleorCompletableemits, providing strong consistency for theRunner. This synchronous-within-asynchronous pattern is applied to all write operations for predictable and reliable behavior.Dependencies:
Added new dependacy 'Firestore'
Testing:
All test passed with > 80% test coverage