Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(spanner): update session bookkeeping for session NotFound #15009

Merged
merged 1 commit into from
Mar 5, 2025
Merged

fix(spanner): update session bookkeeping for session NotFound #15009

merged 1 commit into from
Mar 5, 2025
+33 −13

Conversation

scotthart
Copy link
Member

@scotthart scotthart commented Mar 4, 2025 •
edited
Loading

When attempting to refresh sessions, we would Erase a session if it was "Not Found". Erase would remove the session from the pool, but did not update the session counts which could prevent creation of new sessions to replace those that were erased.

part of the work for #14958

This change is Reviewable

@product-auto-label product-auto-label bot added the api: spanner Issues related to the Spanner API. label Mar 4, 2025
@codecov Codecov
Copy link

codecov bot commented Mar 4, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.90%. Comparing base (a7a6715) to head (d23f901).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #15009   +/-   ##
=======================================
  Coverage   92.90%   92.90%           
=======================================
  Files        2351     2351           
  Lines      210199   210205    +6     
=======================================
+ Hits       195284   195290    +6     
  Misses      14915    14915

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@scotthart scotthart marked this pull request as ready for review March 4, 2025 22:15
@scotthart scotthart requested a review from a team as a code owner March 4, 2025 22:15
@scotthart scotthart force-pushed the spanner_session_server_deleted branch from 4b58a6b to d23f901 Compare March 4, 2025 22:15
@scotthart scotthart merged commit 4556494 into googleapis:main Mar 5, 2025
73 checks passed
devbww
devbww reviewed Mar 6, 2025
View reviewed changes
google/cloud/spanner/internal/session_pool.cc
@@ -369,6 +370,16 @@ std::shared_ptr<SpannerStub> SessionPool::GetStub(Session const& session) {
return GetStub(std::unique_lock<std::mutex>(mu_));
}

void SessionPool::DecrementSessionCount(
std::unique_lock<std::mutex> const&,
google::cloud::spanner_internal::Session& session) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're already in the google::cloud::spanner_internal namespace.

It also looks like a Session const& would suffice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #15107

google/cloud/spanner/internal/session_pool.h
* Returns the number of sessions in the session pool plus the number of
* sessions allocated to running transactions.
*/
int total_sessions() const { return total_sessions_; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

total_sessions_ is guarded by mu_.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added lock in #15017

scotthart
scotthart commented Mar 6, 2025
View reviewed changes
Copy link
Member Author

@scotthart scotthart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions

google/cloud/spanner/internal/session_pool.h
* Returns the number of sessions in the session pool plus the number of
* sessions allocated to running transactions.
*/
int total_sessions() const { return total_sessions_; }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added lock in #15017

google/cloud/spanner/internal/session_pool.cc
@@ -369,6 +370,16 @@ std::shared_ptr<SpannerStub> SessionPool::GetStub(Session const& session) {
return GetStub(std::unique_lock<std::mutex>(mu_));
}

void SessionPool::DecrementSessionCount(
std::unique_lock<std::mutex> const&,
google::cloud::spanner_internal::Session& session) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #15107

@scotthart scotthart mentioned this pull request Mar 14, 2025
Merged
ddelgrosso1 added a commit that referenced this pull request Apr 9, 2025
@ddelgrosso1 @danielduhh
@scotthart @renovate-bot @cuiy0006
chore: merge from public circa 2025-03-24 (#111)
24b5c56 
* chore: set gcs-sdk-team as CODEOWNERS (#15000)

Replace outdated GCS codeowners name to gcs-sdk-team

* chore: update googleapis SHA circa 2025-02-27 (#15003)

* chore: update googleapis SHA circa 2025-02-27

PiperOrigin-RevId: 731731741

* chore(deps): update dependency rules_python to v1.2.0 (#15002)

* ci: disable external account integration test (#15004)

* refactor(storage): avoid initializing json object with empty initializer list (#15006)

* docs(release): update changelog for the 2025-03 release (#15008)

* docs(release): update changelog for the 2025-03 release

* update changelog

* update changelog

* chore: version bump to 2.37.0-rc (#15012)

* fix(spanner): update session bookkeeping for session NotFound (#15009)

* chore(deps): update dependency google_cloud_cpp to v2.36.0 (#15010)

* feat!: remove client library resourcesettings (#15014)

* remove resourcesettings

* checkers format changes

* cleanup

* exclude resourcesettings from quickstart cmake

* add changelog

* Chore: update googleapis SHA circa 2025-03-06 (#15016)

* chore: update googleapis SHA circa 2025-03-06

PiperOrigin-RevId: 734192973

* Regenerate libraries

* impl(spanner): lock mutex in total_sessions accessor (#15017)

* checkers

---------

Co-authored-by: Daniel B <danielduhh@gmail.com>
Co-authored-by: Scott Hart <sdhart@google.com>
Co-authored-by: Mend Renovate <bot@renovateapp.com>
Co-authored-by: Yao Cui <cuiyao@google.com>
ddelgrosso1 added a commit that referenced this pull request Apr 9, 2025
@ddelgrosso1 @danielduhh
@scotthart @renovate-bot @cuiy0006
chore: merge from public circa 2025-03-24 (#111)
6f69f6b 
* chore: set gcs-sdk-team as CODEOWNERS (#15000)

Replace outdated GCS codeowners name to gcs-sdk-team

* chore: update googleapis SHA circa 2025-02-27 (#15003)

* chore: update googleapis SHA circa 2025-02-27

PiperOrigin-RevId: 731731741

* chore(deps): update dependency rules_python to v1.2.0 (#15002)

* ci: disable external account integration test (#15004)

* refactor(storage): avoid initializing json object with empty initializer list (#15006)

* docs(release): update changelog for the 2025-03 release (#15008)

* docs(release): update changelog for the 2025-03 release

* update changelog

* update changelog

* chore: version bump to 2.37.0-rc (#15012)

* fix(spanner): update session bookkeeping for session NotFound (#15009)

* chore(deps): update dependency google_cloud_cpp to v2.36.0 (#15010)

* feat!: remove client library resourcesettings (#15014)

* remove resourcesettings

* checkers format changes

* cleanup

* exclude resourcesettings from quickstart cmake

* add changelog

* Chore: update googleapis SHA circa 2025-03-06 (#15016)

* chore: update googleapis SHA circa 2025-03-06

PiperOrigin-RevId: 734192973

* Regenerate libraries

* impl(spanner): lock mutex in total_sessions accessor (#15017)

* checkers

---------

Co-authored-by: Daniel B <danielduhh@gmail.com>
Co-authored-by: Scott Hart <sdhart@google.com>
Co-authored-by: Mend Renovate <bot@renovateapp.com>
Co-authored-by: Yao Cui <cuiyao@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devbww devbww

@dbolduc dbolduc

Assignees
No one assigned
Labels
api: spanner Issues related to the Spanner API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@scotthart @devbww @dbolduc