chore: refactor efm plugins#662
Conversation
| /* | ||
| Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
|
|
There was a problem hiding this comment.
monitor_connection_context.test.ts might need some updates too. could also be a good place to add tests for ContextPoolImpl, HostMonitorImpl, ConnectionContextImpl
| .catch((error: any) => { | ||
| throw error; | ||
| }); | ||
| const raceResult = await Promise.race([this.waitForUnhealthy(context), methodFunc()]); |
There was a problem hiding this comment.
does race ensure that we cancel or clean up the one that does not finish first? or are we relying on stop monitoring to kill anything eventually?
There was a problem hiding this comment.
waitForUnhealthy should complete on its own eventually and the finally block should clean up methodFunc
| export interface ConnectionContext { | ||
| readonly failureDetectionIntervalMillis: number; | ||
| readonly failureDetectionCount: number; | ||
| readonly expectedActiveMonitoringStartTimeNano: number; |
There was a problem hiding this comment.
i think we put nano times as bigint in the other parts of the wrapper
There was a problem hiding this comment.
The util method getCurrentTimeNano returns the current nanos as a Number.
I will create a separate PR going over all the nano variables in the codebase and make it consistent.
| /** | ||
| * Manages active and pending monitoring contexts grouped by host. | ||
| */ | ||
| export interface ContextPool { |
There was a problem hiding this comment.
i'm curious if this is as needed for node, as node is single threaded it shouldn't have the same concurrency concerns - could there be a simpler solution for this?
There was a problem hiding this comment.
That's a good point. Although I do wonder if it is even worthwhile introducing the extra complexity. The context pool implementation in JDBC didn't have significant performance impact either.
2 participants
Summary
Description
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.