chore: add multiplexed session implementations for CachedSession/SessionFuture interfaces. #2973

arpan14 · 2024-03-26T04:09:03Z

Unit tests haven't been added in current PR. Will include in next PR which will contain the pool create/get methods for multiplexed session.

…ionFuture interfaces.

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java

olavloite · 2024-03-26T08:39:27Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java

+
+    @Override
+    public void markBusy(ISpan span) {
+      this.delegate.setCurrentSpan(span);


Isn't delegate the single SessionImpl instance that we have for our only multiplexed session? Meaning that calling this will overwrite the value that was set by the previous MultiplexedSession instance, even when that other MultiplexedSession is still being used for something?

Yes, it will overwrite the value. But I also think the setSpan method is obsolete and won't have any impact on the traces. See this line in ISpan span = tracer.spanBuilder(READ_ONLY_TRANSACTION); for singleUse method.

So for each new singleUse() method call a new span is getting created. And I don't think calling this setter will impact how the traces would appear for concurrent transactions. I'll keep this comment open till I do a few more manual tests.

If it is not needed, then we should remove it. This would otherwise just cause confusion.

Done, I added a comment.

olavloite · 2024-03-26T08:40:52Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java

+      // multiplexed sessions run more than one transaction concurrently. we cannot store the
+      // exception state as that is not applicable to all transactions running on the session.
+      return exception;


Shouldn't this then throw something like an UnsupportedOperationException? Or is it OK to call this method for multiplexed sessions, even though we can't store the exception?

It is ok to call this method. Additionally I found 1 use-case where its still helpful to maintain the last exception. We can continue to keep a check within the close() method of MultiplexedSession for when the exception is of for instance/database not found errors. If its this kind of an error then we can set the existing resourceNotFoundException variable of SessionPool class.

While this won't be needed immediately since regular sessions already have this check but I'm just adding it for safety so that we don't miss it when we make more methods applicable for multiplexed session.

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java

olavloite · 2024-03-26T12:10:22Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java

+
+    @Override
+    public void markBusy(ISpan span) {
+      this.delegate.setCurrentSpan(span);


If it is not needed, then we should remove it. This would otherwise just cause confusion.

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java

olavloite · 2024-03-28T07:35:15Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java

  interface CachedSession extends Session {

    SessionImpl getDelegate();

+    @ObsoleteApi("This method can be removed once we fully migrate to multiplexed sessions.")


nit: I think that you're absolutely right that this will become obsolete, and it is very good to document that already now, but I don't think we should do it with this annotation, as it is not actually obsolete yet. This might cause some IDEs or build systems to issue warnings. So could we change it to just a normal comment (or potentially a TODO comment).

Sure, makes sense. I have added a TODO.

olavloite · 2024-03-28T07:35:25Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java

    void markBusy(ISpan span);

    void markUsed();

    SpannerException setLastException(SpannerException exception);

+    @ObsoleteApi("This method can be removed once we fully migrate to multiplexed sessions.")


nit: Same here as above

olavloite · 2024-03-28T07:36:25Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java

+        if (lastException != null && isDatabaseOrInstanceNotFound(lastException)) {
+          // Mark this session pool as no longer valid and then release the session into the pool as
+          // there is nothing we can do with it anyways.
+          synchronized (lock) {


nit: remove, we already hold the lock

…ionFuture interfaces. (googleapis#2973) * chore: add multiplexed session implementations for CachedSession/SessionFuture interfaces. * chore: add comments. * chore: add session replacement handler for multiplexed session. * chore: address comments. * chore: fix comments. * chore: fix comments.

* feat: add PG OID support * chore: fix lint errors * Update PG.OID implementation according to recent changes. * Update PG.OID implementation according to recent changes. * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * chore: keep session pool ordering when pinging (#2695) * chore: keep session pool ordering when pinging Pinging sessions would move the sessions that were pinged to either the front or the back of the pool (dependingin the session pool configuration), instead of keeping the sessions in the place where they were when being pinged. Bringing a session that is pinged to the front of the pool means that we will prefer using a session that has not really been used for a while, other than for the ping. Keeping the sessions in place is therefore preferable. * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * deps: update dependency com.google.cloud:google-cloud-monitoring to v3.38.0 (#2942) * feat: allow attempt direct path xds via env var (#2950) To enable Direct Access, [both `setAttemptDirectPath` and `setAttemptDirectPathXds` should be called](https://togithub.com/googleapis/sdk-platform-java/blob/4b44a7851dc1d4fd2ac21a54df6c24db5625223c/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java#L373-L386) for gax to append the correct google-c2p scheme. This PR adds a env var `GOOGLE_SPANNER_ENABLE_DIRECT_ACCESS` to control the enable/disable of Direct Access. When it is true, it calls `setAttemptDirectPathXds` which effectively turns on Direct Access (as `options.isAttemptDirectPath` is by default true and we don't need to call `setAttemptDirectPath` again). * build(deps): update dependency org.apache.maven.plugins:maven-compiler-plugin to v3.13.0 (#2956) * build(deps): update dependency org.apache.maven.plugins:maven-assembly-plugin to v3.7.1 (#2955) * deps: update dependency com.google.cloud:sdk-platform-java-config to v3.28.1 (#2952) * refactor: move skip methods to abstract parser (#2948) Move the PostgreSQL skip methods from the PostgreSQL parser to the abstract parser. This is step 1 in refactoring the GoogleSQL and PostgreSQL parser so they can share more code. The eventual goal is to allow the GoogleSQL parser to be able to handle SQL string without having to remove the comments from the string first. * fix: return type of max commit delay option. (#2953) * Use `TransactionOption` as return type instead of `TransactionOption` * refactor: generalize skip methods (#2949) Generalize the various skip methods so these can be used for both dialects. Each dialect implements a number of abstract methods to indicate what type of statements and constructs they support. These methods are used by the generalized skip methods to determine the start and end of literals, identifiers, and comments. This is step 2 of the refactor that is needed to share more of the code between the SpannerStatementParser and PostgreSQLStatementParser. * perf: keep comments when searching for params (#2951) Keep all comments in the SQL string in place when converting positional parameters to named parameters. This reduces the amount of string operations that are needed for each query that is executed, and also enables actually sending comments from the client to Spanner when using positional parameters (e.g. in JDBC). This is step 3 in the refactoring to share more code between the SpannerStatementParser and PostgreSQLStatementParser. * chore: randomize session pool order based on TPS (#2792) * chore: randomize session pool order based on TPS * chore: remove unnecessary changes * chore(main): release 6.62.0 (#2940) Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> * chore(main): release 6.62.1-SNAPSHOT (#2957) :robot: I have created a release *beep* *boop* --- ### Updating meta-information for bleeding-edge SNAPSHOT release. --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please). * chore(deps): update dependency com.google.cloud:google-cloud-spanner to v6.62.0 (#2958) * chore(deps): update dependency com.google.cloud:google-cloud-spanner to v6.62.0 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * chore: add session pool options for multiplexed session. (#2960) * fix: prevent illegal negative timeout values into thread sleep() method while retrying exceptions in unit tests. * For details on issue see - #2206 * Fixing lint issues. * chore: add session pool options for multiplexed session. * Update google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPoolOptions.java Co-authored-by: Knut Olav Løite <koloite@gmail.com> * Update google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPoolOptions.java Co-authored-by: Knut Olav Løite <koloite@gmail.com> * fix: comments. * chore: lint fix. --------- Co-authored-by: Knut Olav Løite <koloite@gmail.com> * deps: update dependency com.google.cloud:google-cloud-trace to v2.38.0 (#2967) * chore: add new members in SessionImpl for multiplexed session. Add a … (#2961) * chore: add new members in SessionImpl for multiplexed session. Add a new method to create multiplexed session. * chore: add unit tests. * Update google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionClient.java Co-authored-by: Knut Olav Løite <koloite@gmail.com> * fix: comments. * chore: prefer junit assertions. * chore: change to default method in SpannerRpc interface. --------- Co-authored-by: Knut Olav Løite <koloite@gmail.com> * Update .gitignore to remove IDE specific files and remove unnecessary entries from CLIRR ignores * Remove PG.OID external getters. User should use Long/LongArray/LongList getters instead to get PgOid/PgOidArray/PgOidList columns. * chore: generalise session pool class for multiplexed session. (#2964) * chore: generalise session pool class for multiplexed session. * chore: add back previous code. * chore: address comments. * chore: emove unnecessary debug. * chore: add multiplexed session implementations for CachedSession/SessionFuture interfaces. (#2973) * chore: add multiplexed session implementations for CachedSession/SessionFuture interfaces. * chore: add comments. * chore: add session replacement handler for multiplexed session. * chore: address comments. * chore: fix comments. * chore: fix comments. * Remove internal PG.OID getters. * deps: update dependency com.google.cloud:google-cloud-monitoring to v3.39.0 (#2966) * chore(main): release 6.62.1 (#2968) :robot: I have created a release *beep* *boop* --- ## [6.62.1](https://togithub.com/googleapis/java-spanner/compare/v6.62.0...v6.62.1) (2024-03-28) ### Dependencies * Update dependency com.google.cloud:google-cloud-monitoring to v3.39.0 ([#2966](https://togithub.com/googleapis/java-spanner/issues/2966)) ([a5cb1dd](https://togithub.com/googleapis/java-spanner/commit/a5cb1ddd065100497d9215eff30d57361d7e84de)) * Update dependency com.google.cloud:google-cloud-trace to v2.38.0 ([#2967](https://togithub.com/googleapis/java-spanner/issues/2967)) ([b2dc788](https://togithub.com/googleapis/java-spanner/commit/b2dc788d5a54244d83a192ecac894ff931f884c4)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please). * chore(main): release 6.62.2-SNAPSHOT (#2983) :robot: I have created a release *beep* *boop* --- ### Updating meta-information for bleeding-edge SNAPSHOT release. --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please). * feat: add support for transaction-level exclusion from change streams (#2959) * Add support for transaction-level exclusion from change streams * cleanup * refactor: introduce PartitionedUpdateOption * Revert "refactor: introduce PartitionedUpdateOption" This reverts commit 96b508b. * Add error handling in DML update APIs where excludeTxnFromChangeStreams option is not applicable * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * deps: update dependency com.google.cloud:google-cloud-trace to v2.39.0 (#2988) * deps: update dependency commons-io:commons-io to v2.16.0 (#2986) * deps: update dependency com.google.cloud:google-cloud-monitoring to v3.40.0 (#2987) * deps: update dependency com.google.cloud:google-cloud-monitoring to v3.40.0 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * chore(deps): update dependency com.google.cloud:libraries-bom to v26.35.0 (#2989) * chore(deps): update dependency com.google.cloud:libraries-bom to v26.35.0 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * chore(main): release 6.63.0 (#2985) Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> * chore(main): release 6.63.1-SNAPSHOT (#2991) :robot: I have created a release *beep* *boop* --- ### Updating meta-information for bleeding-edge SNAPSHOT release. --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please). * chore: clean up some warnings and malformed comments (#2977) * chore: clean up some warnings and malformed comments * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * chore(deps): update dependency com.google.cloud:google-cloud-spanner to v6.63.0 (#2992) * chore(deps): update dependency com.google.cloud:google-cloud-spanner to v6.63.0 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * feat: add endpoint connection URL property (#2969) Adds an 'endpoint' connection URL property for the Connection API. This property can be used instead of adding the endpoint to the host group part of the Connection URL, which again removes the need to actually change the connection URL when connecting to for example the emulator from the JDBC driver. The latter can instead just add the endpoint to the Properties set that is given to the JDBC driver. * feat: support max_commit_delay in Connection API (#2954) * feat: support max_commit_delay in Connection API Adds support for max_commit_delay to the Connection API: 1. Adds a setMaxCommitDelay(Duration) method to Connection 2. Adds a maxCommitDelay connection URL property 3. Adds a SET MAX_COMMIT_DELAY=<duration> SQL statement * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * chore: minor improvements to default benchmarks. (#2993) * chore: minor improvements to default benchmarks. * chore: lint issues fix. * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * build(deps): update dependency org.jacoco:jacoco-maven-plugin to v0.8.12 (#2996) * chore: add regex to match unmanaged dependency check (#1941) (#2971) Source-Link: googleapis/synthtool@ca7a716 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:cecae6152a85d55c932a64515643cf2e32a1f1b6e17503080eb07744b2177f28 Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * feat: Add SessionPoolOptions, SpannerOptions protos in executor protos (#2932) * feat: Add instance partition support to spanner instance proto PiperOrigin-RevId: 611127452 Source-Link: googleapis/googleapis@618d47c Source-Link: googleapis/googleapis-gen@92d8555 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiOTJkODU1NTg4ODI4NDMwZThiNDI4ZWQ3ODIxOWUyM2VlNjY2ZGE3OCJ9 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * fix(deps): Update the Java code generator (gapic-generator-java) to 2.37.0 PiperOrigin-RevId: 611816371 Source-Link: googleapis/googleapis@2a40f63 Source-Link: googleapis/googleapis-gen@d30ff07 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZDMwZmYwNzY3Nzc3YjM4MWZiMTYxN2Y2N2E5MGUzYWJkM2JkYzZkYyJ9 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * feat: Add SessionPoolOptions, SpannerOptions protos in executor protos PiperOrigin-RevId: 621265883 Source-Link: googleapis/googleapis@fed9845 Source-Link: googleapis/googleapis-gen@c66a769 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiYzY2YTc2OTU3ZTJlMTYzNDdiYzFkZDNmNGM2MzgyMjNmMDY1ZWU4MCJ9 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> * chore: Remove unused CLIRR entries --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> Co-authored-by: Knut Olav Løite <koloite@gmail.com> Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: Hailong Wen <youxiabsyw@gmail.com> Co-authored-by: Arpan Mishra <arpanmishra@google.com> Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: dengwe1 <159199800+dengwe1@users.noreply.github.com> Co-authored-by: gcf-owl-bot[bot] <78513119+gcf-owl-bot[bot]@users.noreply.github.com>

arpan14 added 2 commits March 26, 2024 09:37

chore: add multiplexed session implementations for CachedSession/Sess…

06a5773

…ionFuture interfaces.

chore: add comments.

c6a3ba3

arpan14 requested a review from a team as a code owner March 26, 2024 04:09

product-auto-label bot added size: l Pull request size is large. api: spanner Issues related to the googleapis/java-spanner API. labels Mar 26, 2024

arpan14 requested a review from olavloite March 26, 2024 04:09

chore: add session replacement handler for multiplexed session.

3166fb9

olavloite reviewed Mar 26, 2024

View reviewed changes

chore: address comments.

c38107f

olavloite reviewed Mar 26, 2024

View reviewed changes

chore: fix comments.

c3d8f3e

arpan14 requested a review from olavloite March 27, 2024 05:54

arpan14 mentioned this pull request Mar 27, 2024

chore: create create/get methods for multiplexed session. add session… #2979

Closed

olavloite approved these changes Mar 28, 2024

View reviewed changes

chore: fix comments.

37ff613

arpan14 added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 28, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 28, 2024

arpan14 merged commit dfac7ff into googleapis:main Mar 28, 2024
25 of 26 checks passed

arpan14 deleted the mux-pr-pool-interface branch March 28, 2024 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add multiplexed session implementations for CachedSession/SessionFuture interfaces. #2973

chore: add multiplexed session implementations for CachedSession/SessionFuture interfaces. #2973

arpan14 commented Mar 26, 2024 •

edited

olavloite Mar 26, 2024

arpan14 Mar 26, 2024

olavloite Mar 26, 2024

arpan14 Mar 27, 2024

olavloite Mar 26, 2024

arpan14 Mar 27, 2024

olavloite Mar 26, 2024

olavloite Mar 28, 2024

arpan14 Mar 28, 2024

olavloite Mar 28, 2024

olavloite Mar 28, 2024

arpan14 Mar 28, 2024

chore: add multiplexed session implementations for CachedSession/SessionFuture interfaces. #2973

chore: add multiplexed session implementations for CachedSession/SessionFuture interfaces. #2973

Conversation

arpan14 commented Mar 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arpan14 commented Mar 26, 2024 •

edited