Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

Move session_name out of the Connection *Param structs #445

Closed
devjgm opened this issue Aug 26, 2019 · 1 comment · Fixed by #465
Closed

Move session_name out of the Connection *Param structs #445

devjgm opened this issue Aug 26, 2019 · 1 comment · Fixed by #465
Assignees
Labels
api: spanner Issues related to the googleapis/google-cloud-cpp-spanner API. type: cleanup An internal cleanup or hygiene concern.

Comments

@devjgm
Copy link
Contributor

devjgm commented Aug 26, 2019

We do not want to expose session_name in the *Params structs that are defined in connection.h, so we need to move the session_name out into some other place.

From discussions here, it seems that the leading option is to move the session_name string into the Transaction object itself.

@devbww has other ideas too.

Moving it anywhere out of our public API and into an implementation detail SGTM.

@devjgm devjgm added this to the Spanner: Alpha milestone Aug 26, 2019
@devjgm
Copy link
Contributor Author

devjgm commented Aug 26, 2019

@devbww and @mr-salty FYI

@mr-salty mr-salty self-assigned this Aug 26, 2019
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Aug 27, 2019
@devjgm devjgm added type: cleanup An internal cleanup or hygiene concern. and removed triage me I really want to be triaged. labels Aug 27, 2019
mr-salty added a commit that referenced this issue Aug 28, 2019
mr-salty added a commit that referenced this issue Aug 28, 2019
`Transaction` now has a `SessionHolder` which is populated on-demand during
the first operation involving that `Transaction`.

The session is (normally) returned to the pool it was allocated from when
the `Transaction` is destroyed, but in the interim it will not be used by
any other `Transaction`.

- `Transaction::Visit` passes a reference to its `SessionHolder` to its
  functor. These are usually `ConnectionImpl` methods, which now use the
  session a `Transaction` already holds, or get a new session from the pool
  if needed.
- For Partition Read/Query cases:
  - Allow constructing a `Transaction` with session and transaction IDs.
  - Remove session from *Params structs (fixes #445).
  - Partitioned `Transactions` do not return their sessions to the pool, nor
    do the `PartitionRead` or `PartitionQuery` operations that created them.
- Update tests; add a matcher to check the `Transaction` session and
  transaction IDs.

Fixes #464
Fixes #445
@google-cloud-label-sync google-cloud-label-sync bot added the api: spanner Issues related to the googleapis/google-cloud-cpp-spanner API. label Jan 31, 2020
devjgm pushed a commit to devjgm/google-cloud-cpp that referenced this issue May 7, 2020
…ud-cpp-spanner#465)

`Transaction` now has a `SessionHolder` which is populated on-demand during
the first operation involving that `Transaction`.

The session is (normally) returned to the pool it was allocated from when
the `Transaction` is destroyed, but in the interim it will not be used by
any other `Transaction`.

- `Transaction::Visit` passes a reference to its `SessionHolder` to its
  functor. These are usually `ConnectionImpl` methods, which now use the
  session a `Transaction` already holds, or get a new session from the pool
  if needed.
- For Partition Read/Query cases:
  - Allow constructing a `Transaction` with session and transaction IDs.
  - Remove session from *Params structs (fixes googleapis/google-cloud-cpp-spanner#445).
  - Partitioned `Transactions` do not return their sessions to the pool, nor
    do the `PartitionRead` or `PartitionQuery` operations that created them.
- Update tests; add a matcher to check the `Transaction` session and
  transaction IDs.

Fixes googleapis/google-cloud-cpp-spanner#464
Fixes googleapis/google-cloud-cpp-spanner#445
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api: spanner Issues related to the googleapis/google-cloud-cpp-spanner API. type: cleanup An internal cleanup or hygiene concern.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants