feat: increase sessions in the pool in batches#963
Conversation
Codecov Report
@@ Coverage Diff @@
## master #963 +/- ##
==========================================
- Coverage 98.25% 98.25% -0.01%
==========================================
Files 21 21
Lines 20327 20338 +11
Branches 1077 1079 +2
==========================================
+ Hits 19973 19983 +10
Misses 351 351
- Partials 3 4 +1
Continue to review full report at Codecov.
|
|
Sorry @olavloite, I ran out of time to take a proper look at this. So it'll be next week before I get to this. Hope that it's ok. |
skuruppu
left a comment
There was a problem hiding this comment.
Thanks for this change @olavloite. I just had one minor question.
Another thought is the possibility of initializing the client with some sessions. As I understand it, default min sessions is 0 so I think we would wait for the first request before attempting to create sessions. Your change helps since at least now we won't be creating one session at a time when your default min sessions is zero. But maybe we can set min to something like 10 so the pool opens with something. Thoughts?
| ? this.options.incStep | ||
| : DEFAULTS.incStep!; | ||
| let writes = 0; | ||
| // Only create 1 write session at a time. |
There was a problem hiding this comment.
Is there a specific reason for this? Are we worried that we might overdo the number of write sessions and go over the write fraction?
There was a problem hiding this comment.
The reasoning behind this is to keep it consistent with the current behavior of the node session pool, which treats the configured fraction for write sessions slightly different from what the Java and Go pools do. The basic working (at the moment) is:
- Use the write fraction to determine the number of write sessions to create during the initial filling of the session pool. This means that if you configure
min=100andwrite=0.2you'll end up with an initial session pool with 20 write sessions. - The write fraction is not used to determine whether any further write sessions should be created when additional sessions are needed. Instead, write sessions are only created on-demand when the user application requests one, and there is none available in the pool. That is what is happening here.
- A session that has been marked as a write session, either during the initial filling of the pool, or later because it was created on-demand, will remain a write session during the course of its lifetime. This means that the node session pool will maintain a number of write sessions, even if a write fraction of 0.0 (the default) has been configured, if the user application requires write sessions.
The last point is something we should consider a breaking change if we want to change it, but it is probably better not to change it at all. It is a very nice way of auto-configuring the number of write sessions in the pool.
#1031 further optimizes the way the pool maintains write sessions, but retains the behavior from point 3 above.
Yes, I do think it would be good to set a non-zero default value for |
|
@skuruppu I figured out why the system tests (and emulator tests) were failing, and the reason behind it might also be a reason that it might not be a good idea to change the default for database2 = instance.database(generateName('database'));
const [, database2CreateOperation] = await database2.create({
schema: `
CREATE TABLE Albums (
AlbumId STRING(1024) NOT NULL,
AlbumTitle STRING(1024) NOT NULL,
) PRIMARY KEY(AlbumId)`,
});
await database2CreateOperation.promise();The first line will however not only get a reference to the database, but it will also try to open a session pool for it. As the database at that moment does not yet exist, filling the session pool with sessions will fail. That is not a problem as long as the default for |
|
@olavloite thanks for looking into changing the min sessions to be non-zero. That makes sense. The pool is initialized in the constructor but as you said, the database may not exist at that point. So it makes sense to not change that here then. I think we can get this working by moving the pool creation logic (conditionally) but that's for another time. |
|
I'd be happy for you to merge this in when you're ready. |
…odejs-spanner into increase-sessions-in-batches
Increase the number of sessions in the pool in batches of 25 (configurable) for better performance and less RPCs.
This also adds four benchmarks for to verify the working of the above change, and for comparison with other supported platforms. The simulated execution times and network latency is not intended to be 100% equal to a production situation, but to simulate a realistic and repeatable situation that can be used for multiple platforms.