chore: cleanup session pool

[surbhigarg92]

No description provided.

[gemini-code-assist]

Summary of Changes

Hello @surbhigarg92, I'm Gemini Code Assist¹! 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 introduces a significant architectural change by deprecating and removing the legacy SessionPool implementation. The system now operates entirely on MultiplexedSession for all database interactions, leading to a more robust and simplified session management model. This refactoring streamlines the codebase, enhances maintainability, and removes outdated retry mechanisms, focusing on a single, efficient session handling strategy.

Highlights

• Session Pool Removal: The traditional SessionPool class and its associated complex session management logic have been entirely removed from the library.
• Exclusive Multiplexed Sessions: The library now exclusively utilizes MultiplexedSession for all session handling, simplifying the overall architecture and reducing overhead.
• Streamlined Session Factory: The SessionFactory has been refactored to solely provide multiplexed sessions, eliminating methods and properties related to the deprecated traditional session pooling.
• Removed Session Retry Logic: Complex retry mechanisms previously implemented for 'Session not found' errors, which were tied to the old session pool, have been removed as they are no longer necessary with the multiplexed session approach.
• Extensive Test Cleanup: Numerous tests covering the old session pooling behavior, session leaks, and specific session types (read-write, partitioned ops) have been removed or updated to align with the new, simplified session management.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request significantly refactors the session management by deprecating the traditional SessionPool in favor of always using multiplexed sessions. This change simplifies the codebase by removing a substantial amount of logic related to session pooling, session error handling, and associated test cases. The SessionFactory has been streamlined to consistently provide multiplexed sessions, which is a fundamental shift in how sessions are acquired and managed. The removal of the SessionPool class and related functionalities should improve maintainability and reduce complexity. However, it's important to ensure that all previous functionalities and edge cases handled by the old session pool are adequately covered by the new multiplexed session approach, especially regarding performance and resource management under various workloads.

I am having trouble creating individual review comments. Click here to see my feedback.

test/spanner.ts (2671-3779)

The removal of numerous test cases related to query options on database options, session-not-found retries, and session-pool specific tests (leaks, reuse, exhaustion, pre-fill, error propagation) indicates a complete overhaul of session management. These tests were critical for validating the behavior of the old session pool under various conditions. Their removal is consistent with the deprecation of the old session pool. It is crucial to ensure that the new multiplexed session management inherently handles these scenarios correctly or that new tests are in place to cover the new behavior.

test/spanner.ts (2078-2187)

The removal of several test cases related to Session not found errors and session pool behavior when multiplexed sessions are disabled indicates a significant shift in session management. These tests were crucial for verifying retry logic and session creation behavior under specific conditions. Their removal is consistent with the deprecation of the old session pool. Please ensure that the new multiplexed session management correctly handles transient session errors and retries where appropriate, or that new tests cover this behavior.

observability-test/database.ts (584)

The test case it('with retries on beginerrors withSession not found', done => { ... }); has been removed. This test was crucial for verifying retry logic on session not found errors. While the old session pool is being removed, the underlying retry mechanism for session errors should still be robust. Please ensure that this specific scenario is covered by existing or new tests, or clarify why this retry logic is no longer applicable.

observability-test/database.ts (586)

The describe('createBatchTransaction', () => { block has been skipped. Skipping tests should generally be avoided unless the functionality is completely removed or replaced. If createBatchTransaction is still a valid API, its tests should be updated to reflect the new session management rather than being skipped. If the functionality is deprecated, it should be explicitly marked as such in the code and documentation.

observability-test/database.ts (1289-1365)

The test case it('on retry with "Session not found" error', done => { ... }); has been removed. This test specifically validated retry behavior for batchWriteAtLeastOnce when a session is not found. Given the removal of the old session pool and its specific error handling, this test might no longer be relevant in its original form. However, it's critical to ensure that the new session management correctly handles transient session errors and retries where appropriate. Please confirm that the new design inherently handles such scenarios or that new tests cover this behavior.

observability-test/database.ts (1902-1991)

The test case it('retries with "Session not found" error', done => { ... }); has been removed. This test specifically validated retry behavior for runStream when a session is not found. Similar to other removals, this indicates a shift in session management. Please ensure that the new multiplexed session handling implicitly covers such retry scenarios or that new tests are in place to validate this behavior.

src/database.ts (3180-3197)

The removal of the dataReceived flag and the isSessionNotFoundError retry logic indicates a significant change in how streaming errors are handled. Previously, if a Session not found error occurred before any data was received, the stream would be retried on a new session. This behavior is now gone. Please confirm that the new multiplexed session management handles transient session errors during streaming appropriately, or if this specific retry mechanism is no longer needed.

test/database.ts (2436-2519)

The test cases related to Session not found errors, session release, and retries in getSnapshot have been removed. These tests validated specific retry and cleanup behaviors of the old session pool. Their removal is consistent with the deprecation of the old session pool. Please ensure that the new multiplexed session management correctly handles transient session errors and cleanup, or that new tests cover this behavior.

test/database.ts (2026-2089)

The test cases related to Session not found errors and session release in runStream have been removed. These tests validated specific retry and cleanup behaviors of the old session pool. Their removal is consistent with the deprecation of the old session pool. Please ensure that the new multiplexed session management correctly handles transient session errors and cleanup, or that new tests cover this behavior.

test/database.ts (870)

The describe('error', () => { block has been skipped. Skipping tests should generally be avoided unless the functionality is completely removed or replaced. If the error handling for close is still relevant, its tests should be updated to reflect the new session management rather than being skipped. If the functionality is deprecated, it should be explicitly marked as such in the code and documentation.

src/database.ts (812-821)

The close method now simulates success by calling the callback immediately or resolving a promise. This change indicates that the Database object no longer delegates its close operation to a SessionPool. This is a significant change in resource management. Please ensure that all resources (especially sessions) are properly closed and released by the new session management strategy, as this stubbed behavior might hide actual resource leaks if not handled elsewhere.

test/database.ts (799-819)

The test case it('should retry on "Session not found" error', done => { ... }); has been removed. This test specifically validated retry logic for batchWriteAtLeastOnce when a session is not found. Given the removal of the old session pool and its specific error handling, this test might no longer be relevant in its original form. Please confirm that this scenario is covered by existing or new tests, or clarify why this retry logic is no longer applicable.

src/database.ts (3473)

The removal of dataReceived flag and the isSessionNotFoundError retry logic indicates a significant change in how streaming errors are handled. Previously, if a Session not found error occurred before any data was received, the stream would be retried on a new session. This behavior is now gone. Please confirm that the new multiplexed session management handles transient session errors during streaming appropriately, or if this specific retry mechanism is no longer needed.

src/database.ts (3340)

The removal of the while (true) loop for retries indicates a significant change in the retry strategy for runTransactionAsync. Previously, this loop handled retries for Session not found errors. This is now gone. Please confirm that the new multiplexed session management handles transient session errors and retries where appropriate, or if this specific retry mechanism is no longer needed.

src/database.ts (3478)

The removal of this.sessionFactory_.release(session!); indicates that the session is no longer explicitly released by this method. This aligns with the new session management where sessions are not manually released back to a pool in the same way. Ensure that the session lifecycle is correctly managed by the multiplexed session factory.

test/database.ts (355-376)

The describe('when multiplexed session is disabled', () => { ... }); block and its contained test it('should re-emit SessionPool errors', done => { ... }); have been removed. These tests were specific to the behavior of the old session pool when multiplexed sessions were disabled. Their removal is consistent with the deprecation of the old session pool. Ensure that error propagation from the new session management is adequately tested.

src/database.ts (2469)

The removal of releaseSession(); from the abort handler indicates that the session is no longer explicitly released when a streaming request is aborted. This aligns with the new session management where sessions are not manually released back to a pool in the same way. Ensure that the session lifecycle is correctly managed by the multiplexed session factory.

src/database.ts (2572-2576)

The releaseSession function has been removed. This function was responsible for releasing a session back to the pool. Its removal is consistent with the deprecation of the old session pool. Ensure that the new multiplexed session management handles session cleanup and release appropriately without this explicit mechanism.

src/database.ts (2489)

The removal of requestStream.on('error', releaseSession).on('end', releaseSession) indicates that the session is no longer explicitly released on stream error or end. This aligns with the new session management where sessions are not manually released back to a pool in the same way. Ensure that the session lifecycle is correctly managed by the multiplexed session factory.

src/database.ts (3063)

The removal of this._releaseOnEnd(session!, snapshot, span); indicates that the session is no longer explicitly released by this method. This aligns with the new session management where sessions are not manually released back to a pool in the same way. Ensure that the session lifecycle is correctly managed by the multiplexed session factory.

src/database.ts (2124)

The removal of this._releaseOnEnd(session!, snapshot, span); indicates that the session is no longer explicitly released by this method. This aligns with the new session management where sessions are not manually released back to a pool in the same way. Ensure that the session lifecycle is correctly managed by the multiplexed session factory.

src/database.ts (3210)

The removal of the isSessionNotFoundError check indicates that this specific error handling for session not found errors is no longer present here. This is consistent with the removal of the old session pool's retry mechanisms. Ensure that the new session management correctly handles transient session errors and retries where appropriate, possibly at a lower level in the SessionFactory.

src/database.ts (3236-3239)

The removal of release() call in the then and catch blocks of runner.run() indicates that the session is no longer explicitly released by this method. This aligns with the new session management where sessions are not manually released back to a pool in the same way. Ensure that the session lifecycle is correctly managed by the multiplexed session factory.

src/database.ts (2120-2122)

The removal of the isSessionNotFoundError check and the span.addEvent('No session available', ...) logic indicates that this specific error handling for session not found errors is no longer present here. This is consistent with the removal of the old session pool's retry mechanisms. Ensure that the new session management correctly handles transient session errors and retries where appropriate, possibly at a lower level in the SessionFactory.

src/database.ts (3531-3534)

The removal of the isSessionNotFoundError check and the span.addEvent('No session available', ...) logic indicates that this specific error handling for session not found errors is no longer present here. This is consistent with the removal of the old session pool's retry mechanisms. Ensure that the new session management correctly handles transient session errors and retries where appropriate, possibly at a lower level in the SessionFactory.

src/database.ts (878)

The removal of the isSessionNotFoundError check and the span.addEvent('No session available', ...) logic indicates that this specific error handling for session not found errors is no longer present here. This is consistent with the removal of the old session pool's retry mechanisms. Ensure that the new session management correctly handles transient session errors and retries where appropriate, possibly at a lower level in the SessionFactory.

observability-test/database.ts (49)

The removal of MockError import suggests that MockError is no longer used. Please confirm that this type is indeed obsolete and its removal does not impact any other part of the codebase or testing logic.

src/database.ts (3565)

The removal of the isSessionNotFoundError check indicates that this specific error handling for session not found errors is no longer present here. This is consistent with the removal of the old session pool's retry mechanisms. Ensure that the new session management correctly handles transient session errors and retries where appropriate, possibly at a lower level in the SessionFactory.

src/database.ts (3572)

The removal of this._releaseOnEnd(session!, transaction!, span); indicates that the session is no longer explicitly released by this method. This aligns with the new session management where sessions are not manually released back to a pool in the same way. Ensure that the session lifecycle is correctly managed by the multiplexed session factory.

src/transaction-runner.ts (179-183)

The removal of the isSessionNotFoundError check in getNextDelay means that session not found errors will no longer result in a 0ms retry delay. This is consistent with the deprecation of the old session pool's specific retry behavior. Ensure that the new retry strategy is appropriate for all types of transient errors, including session not found errors.

src/transaction-runner.ts (187-188)

The removal of isSessionNotFoundError from the shouldRetry logic means that session not found errors will no longer be explicitly considered retryable at this level. This is consistent with the deprecation of the old session pool's specific retry behavior. Ensure that the new retry strategy is appropriate for all types of transient errors, including session not found errors.

test/database.ts (32)

The removal of MockError import suggests that MockError is no longer used. Please confirm that this type is indeed obsolete and its removal does not impact any other part of the codebase or testing logic.

test/database.ts (345-352)

The test case it('should accept a custom Pool class', () => { ... }); has been removed. This test validated the ability to provide a custom session pool implementation. Its removal is consistent with the deprecation of the old session pool and the move to always-on multiplexed sessions. Ensure that this functionality is no longer needed or that the new design provides an alternative extension point if custom session management is still a requirement.

src/database.ts (2447)

The removal of sessionFactory_.release(session!); indicates that the session is no longer explicitly released by this method. This aligns with the new session management where sessions are not manually released back to a pool in the same way. Ensure that the session lifecycle is correctly managed by the multiplexed session factory.

src/database.ts (876)

The removal of this._releaseOnEnd(session!, transaction, span); indicates that the session is no longer explicitly released by this method. This aligns with the new session management where sessions are not manually released back to a pool in the same way. Ensure that the session lifecycle is correctly managed by the multiplexed session factory.

test/database.ts (821-832)

The test case it('should release session on stream end', () => { ... }); has been removed. This test validated that sessions were released back to the pool after a stream ended. With the removal of the traditional session pool, this specific test is no longer relevant. However, the underlying mechanism for session cleanup and resource management should still be robust. Please confirm that the multiplexed session factory handles session termination appropriately.

src/database.ts (471-474)

The removal of this.databaseRole = poolOptions.databaseRole || null; and this.labels = poolOptions.labels || null; indicates that these properties are no longer directly set from poolOptions. This is consistent with the removal of the old session pool. Ensure that databaseRole and labels are still correctly handled if they are relevant for session creation or other functionalities.

test/database.ts (1624-1635)

The test case it('should release the session after calling the method', done => { ... }); has been removed. This test validated that sessions were released after a pooled request. With the removal of the traditional session pool, this specific test is no longer relevant. However, the underlying mechanism for session cleanup and resource management should still be robust. Please confirm that the multiplexed session factory handles session termination appropriately.

test/database.ts (1737-1759)

The test cases it('should release session when request stream ends', done => { ... }); and it('should release session when request stream errors', done => { ... }); have been removed. These tests validated that sessions were released back to the pool on stream completion or error. With the removal of the traditional session pool, these specific tests are no longer relevant. However, the underlying mechanism for session cleanup and resource management should still be robust. Please confirm that the multiplexed session factory handles session termination appropriately.

test/database.ts (1793-1828)

The test cases it('should release the session', done => { ... }); and it('should not release the session more than once', done => { ... }); have been removed. These tests validated session release behavior when a streaming request was aborted. With the removal of the traditional session pool, these specific tests are no longer relevant. However, the underlying mechanism for session cleanup and resource management should still be robust. Please confirm that the multiplexed session factory handles session termination appropriately.

samples/temp.js (1-53)

This new sample file temp.js seems to be a placeholder or a temporary addition. It contains a transactionTag function that runs a simple Select 1 query multiple times after a delay. If this is intended to be a permanent sample, please consider renaming it to something more descriptive and adding comments that explain its purpose and expected output. If it's temporary, it should be removed before merging.

observability-test/observability.ts (42)

The SEMATTRS_DB_SYSTEM semantic attribute has been removed. Please confirm that this attribute is no longer relevant or necessary for observability in the context of these changes. If the database system is still a valuable piece of information for tracing, consider if there's an alternative way to capture it.

test/database.ts (2574-2601)

The test cases it('should propagate an error', done => { ... }); and it('should release the session on transaction end', done => { ... }); have been removed. These tests validated error propagation and session release behavior for getTransaction. With the removal of the traditional session pool, these specific tests are no longer relevant. However, the underlying mechanism for session cleanup and resource management should still be robust. Please confirm that the multiplexed session factory handles session termination appropriately.

test/database.ts (3013-3022)

The test case it('should release the session on transaction end', () => { ... }); has been removed. This test validated that sessions were released back to the pool after a partitioned update. With the removal of the traditional session pool, this specific test is no longer relevant. However, the underlying mechanism for session cleanup and resource management should still be robust. Please confirm that the multiplexed session factory handles session termination appropriately.

test/database.ts (3175-3187)

The test case it('should release the session when finished', done => { ... }); has been removed. This test validated that sessions were released after a transaction finished. With the removal of the traditional session pool, this specific test is no longer relevant. However, the underlying mechanism for session cleanup and resource management should still be robust. Please confirm that the multiplexed session factory handles session termination appropriately.

test/database.ts (3281-3290)

The test case it('should release the session when finished', async () => { ... }); has been removed. This test validated that sessions were released after an async transaction finished. With the removal of the traditional session pool, this specific test is no longer relevant. However, the underlying mechanism for session cleanup and resource management should still be robust. Please confirm that the multiplexed session factory handles session termination appropriately.

test/index.ts (242)

The test case it('should localize a cached gapic client map', () => { ... }); has been skipped. Skipping tests should generally be avoided unless the functionality is completely removed or replaced. If this test is still relevant, it should be fixed and re-enabled. If the functionality is deprecated, it should be explicitly marked as such in the code and documentation.

observability-test/database.ts (2166-2210)

The test case it('session released on transaction end', done => { ... }); has been removed. This test validated that sessions were released back to the pool after a partitioned update. With the removal of the traditional session pool, this specific test is no longer relevant. However, the underlying mechanism for session cleanup and resource management should still be robust. Please confirm that the multiplexed session factory handles session termination appropriately.

test/spanner.ts (2263-2296)

The describe('when multiplexed session is disabled for partitioned ops', () => { ... }); block and its contained test have been removed. These tests were specific to the behavior of the old session pool when multiplexed sessions were disabled for partitioned operations. Their removal is consistent with the deprecation of the old session pool. Ensure that the new multiplexed session management correctly handles partitioned operations.

test/spanner.ts (2331-2366)

The describe('when only GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSIONS is disabled', () => { ... }); block and its contained test have been removed. These tests were specific to the behavior of the old session pool when multiplexed sessions were disabled. Their removal is consistent with the deprecation of the old session pool. Ensure that the new multiplexed session management correctly handles batch writes.

test/spanner.ts (2369-2404)

The describe('when only GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSIONS_FOR_RW is disabled', () => { ... }); block and its contained test have been removed. These tests were specific to the behavior of the old session pool when multiplexed sessions were disabled for read/write operations. Their removal is consistent with the deprecation of the old session pool. Ensure that the new multiplexed session management correctly handles batch writes.

observability-test/database.ts (527-529)

The removal of sandbox.stub(fakeSessionFactory, 'isMultiplexedEnabled').returns(false); indicates a change in how multiplexed sessions are tested or configured. Given the overall PR aims to remove the old session pool, this change is expected. However, ensure that the new session factory correctly defaults to multiplexed sessions or that the test environment is set up to reflect the intended behavior.

test/spanner.ts (2918-2927)

The describe('when multiplexed session is disabled', () => { ... }); block and its contained tests have been removed. These tests were specific to the behavior of the old session pool when multiplexed sessions were disabled for batch-readonly-transactions. Their removal is consistent with the deprecation of the old session pool. Ensure that the new multiplexed session management correctly handles batch-readonly-transactions.

test/spanner.ts (7184-7195)

The describe('session-factory', () => { ... }); block and its contained test have been removed. This test validated specific behavior of the SessionFactory when multiplexed sessions were disabled after client initialization. Its removal is consistent with the deprecation of the old session pool and the simplification of SessionFactory to always manage multiplexed sessions.