chore: add support for class MultiplexedSession by alkatrivedi · Pull Request #2191 · googleapis/nodejs-spanner

alkatrivedi · 2024-11-13T15:26:42Z

This PR contains the code for the newly introduced class MultiplexedSession.

generated-files-bot · 2024-11-18T08:20:06Z

Warning: This pull request is touching the following templated files:

README.md - README.md is managed by synthtool. However, a partials file can be used to update the README, e.g.: https://github.com/googleapis/nodejs-storage/blob/main/.readme-partials.yaml
samples/README.md - samples/README.md is managed by synthtool. However, a partials file can be used to update the README, e.g.: https://github.com/googleapis/nodejs-storage/blob/main/.readme-partials.yaml

snippet-bot · 2024-11-18T08:20:15Z

No region tags are edited in this PR.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

src/multiplexed-session.ts

surbhigarg92 · 2024-11-28T10:50:01Z

src/multiplexed-session.ts

+          span.addEvent(
+            `Created multiplexed session ${this._multiplexedSession.formattedName_}`
+          );
+          this.emit('mux-session-available');


use the constant MUX_SESSION_AVAILABLE

and everywhere else, in the tests as well

harshachinta · 2024-12-02T08:56:26Z

src/multiplexed-session.ts

+  // frequency to create new mux session
+  refreshRate: number;
+  _multiplexedSession: Session | null;
+  _pingHandle!: NodeJS.Timer;


is this used anywhere?

no where, I have removed it. thanks!

src/multiplexed-session.ts

harshachinta · 2024-12-02T11:50:49Z

src/multiplexed-session.ts

+    span.addEvent('Waiting for a multiplexed session to become available');
+    let removeListener: Function;
+    const promise = new Promise(resolve => {
+      this.once(MUX_SESSION_AVAILABLE, resolve);


What happens if this event is never emitted? Is there any timeout?
As per my understanding the MUX_SESSION_AVAILABLE event will be emitted only when there is no error during initial multiplexed session creation

Thanks for pointing this out.

What happens if this event is never emitted?

In case MUX_SESSION_AVAILABLE event will not get emitted, an error event MUX_SESSION_CREATE_ERROR will get emitted.
Hence, in _getSession we either will wait for the MUX_SESSION_AVAILABLE or for the MUX_SESSION_CREATE_ERROR.

Is there any timeout?

There is no any timeout in the method, we are relying on the events

harshachinta · 2024-12-02T11:51:52Z

src/multiplexed-session.ts

+    });
+
+    try {
+      await promise;


In case the multiplexed session creation fails with error, then will that error be captured by getsession and surfaced to customers?

yes, the error will get surfaced to the customers

harshachinta · 2024-12-02T11:52:34Z

test/multiplexed-session.ts

+    });
+  });
+
+  describe('_getSession', () => {


Can you add a negative case when create session fails with error? What will be the behaviour of getSession?

Since, we are listening to the error event in _getSession in case of the error from the _createSession, I have added a test for the case when the error event will get emitted, if we are able to listen to that event in _getSession

src/multiplexed-session.ts

surbhigarg92 · 2024-12-06T07:20:54Z

test/multiplexed-session.ts

+      });
+    });
+
+    it('should propagate errors for Multiplexed Session which gets emitted', async () => {


There is no assert in this test ?

The assertion part is in the if condition

if (err === fakeError) { resolve(); }

if both the errors are same the promise will get resolve and test will get pass

else { reject(new Error('Unexpected error emitted')); }

otherwise the promise will fail with the rejected promise

Based on our 1:1. I have updated the test, and have used assert instead of if condition

test/multiplexed-session.ts

surbhigarg92 · 2024-12-06T07:26:14Z

test/multiplexed-session.ts

+      multiplexedSession.on(MUX_SESSION_CREATE_ERROR, () => {
+        done();
+      });
+      multiplexedSession._createSession().catch(err => {


Here will .on event called first ? In that case you are calling done here and then .catch will never be called ?

it will call the done as soon as the error event will get emitted, after that the pointer will go to throw e in getSession hence, we need to catch that error here.

.catch will never be called ?

it will get called, on not calling .catch here will result into the unhandled exception error

harshachinta · 2024-12-06T07:25:43Z

src/multiplexed-session.ts

+   * This method sets up a periodic refresh interval for maintaining the session. The interval duration
+   * is determined by the @param refreshRate option, which is provided in days.
+   * The default value is 7 days.
+   * @throws {Error} In case the multiplexed session creation will get fail, and an error occurs within `_createSession`,


Suggested change

* @throws {Error} In case the multiplexed session creation will get fail, and an error occurs within `_createSession`,

* @throws {Error} If the multiplexed session creation fails in `_createSession`, the error is caught and ignored. This is because the currently active multiplexed session has a 30-day expiry, providing the maintainer with four opportunities (one every 7 days) to refresh the active session.

harshachinta · 2024-12-06T07:33:12Z

src/multiplexed-session.ts

+    const promises = [
+      new Promise((_, reject) => {
+        this.once(MUX_SESSION_CREATE_ERROR, reject);
+        removeErrorListener = this.removeListener.bind(


For my understanding,
In get_session, we are listening only to the error event but not emitting any error? The actual error gets emitted from the create_Session call?
which means the error surfaced to customer is from the error event emitted by create_session and not get_session?

No, the _createSession call will emit the error along with the event, which the listener will listen in _getSession. Now, we have two promises in _getSession, for the error case the promise for error event will be there which on begin await will resolve into a rejected promise. hence, an error will get propagated to the parent call

harshachinta · 2024-12-06T07:35:14Z

src/multiplexed-session.ts

+   * @returns {Promise<void>} A Promise that resolves when the session has been successfully created and assigned, an event
+   * `mux-session-available` will be emitted to signal that the session is ready.
+   *
+   * In case of error, an error will get emitted along with the erorr event.


Suggested change

* In case of error, an error will get emitted along with the erorr event.

* In case of error, an error will get emitted along with the error event.

harshachinta · 2024-12-06T07:35:48Z

src/multiplexed-session.ts

+   *
+   * Waits for the `MUX_SESSION_AVAILABLE` event or for the `MUX_SESSION_CREATE_ERROR`
+   * to be emitted if the multiplexed session is not yet available. The method listens
+   * for these events, and once `mux-session-available` is emitted, it resolves and returns


Can you add the behavior when error event is emitted?

harshachinta · 2024-12-06T07:37:01Z

Marking it as chore: since we are not explicitly providing any functionality here for the customers to use?
Please feel free to modify this if that is not true.
@surbhigarg92 @alkatrivedi

alkatrivedi · 2024-12-09T13:04:19Z

Closing this PR, since we are facing issues in presubmit check for Owlbot Processor. I have moved the changes into another branch which has been created from the main repo. Link to the new PR.

alkatrivedi requested review from a team November 13, 2024 15:26

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

alkatrivedi force-pushed the class-multiplexed-session branch 2 times, most recently from 27cf336 to 51df606 Compare November 18, 2024 08:20

alkatrivedi force-pushed the class-multiplexed-session branch from 10899dc to ba378f2 Compare November 18, 2024 10:06

feat: add support for MultiplexedSession class

f6674b9

alkatrivedi force-pushed the class-multiplexed-session branch from 04b8f76 to f6674b9 Compare November 19, 2024 10:12

alkatrivedi added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Nov 19, 2024

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

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 19, 2024

surbhigarg92 reviewed Nov 20, 2024

View reviewed changes

Merge branch 'main' into class-multiplexed-session

dc38eb5

alkatrivedi force-pushed the class-multiplexed-session branch 9 times, most recently from 25cec8e to 5101675 Compare November 25, 2024 08:58

refactor: MultiplexedSession class

99a18ae

alkatrivedi force-pushed the class-multiplexed-session branch from 5101675 to 99a18ae Compare November 25, 2024 10:27

Merge branch 'main' into class-multiplexed-session

d7f0752

alkatrivedi force-pushed the class-multiplexed-session branch from b978f8c to cda3ed3 Compare November 25, 2024 10:47

surbhigarg92 reviewed Nov 28, 2024

View reviewed changes

alkatrivedi force-pushed the class-multiplexed-session branch from cea4617 to 2815e24 Compare November 28, 2024 11:02

alkatrivedi added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Nov 29, 2024

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

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 29, 2024

alkatrivedi force-pushed the class-multiplexed-session branch 2 times, most recently from c57a1be to e8935b4 Compare November 29, 2024 03:36

alkatrivedi added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Nov 29, 2024

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

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 29, 2024

harshachinta reviewed Dec 2, 2024

View reviewed changes

alkatrivedi force-pushed the class-multiplexed-session branch 2 times, most recently from ed7e9b6 to 625787b Compare December 2, 2024 19:12

alkatrivedi added the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 4, 2024

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

alkatrivedi added the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 5, 2024

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

refactor

4584d5e

alkatrivedi force-pushed the class-multiplexed-session branch from e6b1c24 to 4584d5e Compare December 6, 2024 05:31

alkatrivedi and others added 2 commits December 6, 2024 05:32

Merge branch 'googleapis:main' into class-multiplexed-session

94f90ed

refactor

5c2e5f6

surbhigarg92 reviewed Dec 6, 2024

View reviewed changes

src/multiplexed-session.ts Outdated Show resolved Hide resolved

surbhigarg92 reviewed Dec 6, 2024

View reviewed changes

harshachinta approved these changes Dec 6, 2024

View reviewed changes

refactor

2844dcf

harshachinta mentioned this pull request Dec 9, 2024

chore: add support for class MultiplexedSession #2201

Merged

	* @throws {Error} In case the multiplexed session creation will get fail, and an error occurs within `_createSession`,
	* @throws {Error} If the multiplexed session creation fails in `_createSession`, the error is caught and ignored. This is because the currently active multiplexed session has a 30-day expiry, providing the maintainer with four opportunities (one every 7 days) to refresh the active session.

	* In case of error, an error will get emitted along with the erorr event.
	* In case of error, an error will get emitted along with the error event.

Conversation

alkatrivedi commented Nov 13, 2024

Uh oh!

generated-files-bot bot commented Nov 18, 2024

Uh oh!

snippet-bot bot commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

No region tags are edited in this PR.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

harshachinta Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alkatrivedi Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harshachinta commented Dec 6, 2024

Uh oh!

alkatrivedi commented Dec 9, 2024

snippet-bot bot commented Nov 18, 2024 •

edited

Loading

harshachinta Dec 2, 2024 •

edited

Loading

alkatrivedi Dec 6, 2024 •

edited

Loading