feat: efm v2 support #930
Open
feat: efm v2 support #930
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JuanLeee
force-pushed
the
feat/efmv2
branch
2 times, most recently
from
876d0d5 to
5a504a7
Compare
JuanLeee
force-pushed
the
feat/efmv2
branch
9 times, most recently
from
f6e9154 to
9727b6a
Compare
# Conflicts: # aws_advanced_python_wrapper/plugin_service.py # aws_advanced_python_wrapper/utils/atomic.py
karenc-bq
reviewed
Aug 13, 2025
| self._value = value | ||
|
|
||
|
|
||
| class AtomicReference: |
There was a problem hiding this comment.
does it make sense to use generics here instead of Any? might be an overkill
karenc-bq
reviewed
Aug 13, 2025
docs/using-the-python-driver/using-plugins/UsingTheHostMonitoringPlugin.md
Show resolved
Hide resolved
karenc-bq
reviewed
Aug 13, 2025
| from time import perf_counter_ns, sleep | ||
| from typing import TYPE_CHECKING, Any, Callable, ClassVar, Optional, Set | ||
|
|
||
| import psycopg |
There was a problem hiding this comment.
This import shouldn't be here
There was a problem hiding this comment.
This will cause import errors for mysql users, even though efm is not supported for mysql
karenc-bq
reviewed
Aug 14, 2025
|
|
||
| class HostMonitorV2: | ||
| """ | ||
| This class uses a background thread to monitor a particular server with one or more active :py:class:`Connection` |
There was a problem hiding this comment.
Suggested change
| This class uses a background thread to monitor a particular server with one or more active :py:class:`Connection` | |
| This class uses a background thread to monitor a particular server with one or more active :py:class:Connection |
karenc-bq
reviewed
Aug 14, 2025
| logger.warning("HostMonitorV2.MonitorIsStopped", self._host_info.host) | ||
|
|
||
| current_time_ns = perf_counter_ns() | ||
| start_monitoring_time_ns = ((current_time_ns + self._failure_detection_time_ns) // 10**9) * 10**9 |
There was a problem hiding this comment.
Suggested change
| start_monitoring_time_ns = ((current_time_ns + self._failure_detection_time_ns) // 10**9) * 10**9 | |
| start_monitoring_time_ns = current_time_ns + self._failure_detection_time_ns |
Can we create helper methods for these time conversions to make the intent more clear
karenc-bq
reviewed
Aug 14, 2025
| if self.is_stopped: | ||
| logger.warning("HostMonitorV2.MonitorIsStopped", self._host_info.host) | ||
|
|
||
| current_time_ns = perf_counter_ns() |
There was a problem hiding this comment.
Suggested change
| current_time_ns = perf_counter_ns() | |
| current_time_ns = get_current_time_ns() |
karenc-bq
reviewed
Aug 14, 2025
Comment on lines
+199
to
+202
| _THREAD_SLEEP_NANO = 100_000_000 | ||
| _MONITORING_PROPERTY_PREFIX = "monitoring-" | ||
| _QUERY = "SELECT 1" | ||
|
|
There was a problem hiding this comment.
Please check the usage of these. Should use them as static variables instead of self._...
karenc-bq
reviewed
Aug 14, 2025
Comment on lines
+252
to
+254
| queue: Queue = Queue() | ||
| self._new_contexts.compute_if_absent(start_monitoring_time_ns, lambda k: queue) | ||
| queue.put(weakref.ref(context)) |
There was a problem hiding this comment.
Suggested change
| queue: Queue = Queue() | |
| self._new_contexts.compute_if_absent(start_monitoring_time_ns, lambda k: queue) | |
| queue.put(weakref.ref(context)) | |
| compute_if_absent returns the existing queue if key exists | |
| ```suggestion | |
| queue: Optional[Queue] = self._new_contexts.compute_if_absent(start_monitoring_time_ns, lambda k: Queue()) | |
| if queue is not None: | |
| queue.put(weakref.ref(context)) |
karenc-bq
reviewed
Aug 14, 2025
| logger.debug("HostMonitorV2.OpenedMonitoringConnection", self._host_info.url) | ||
| return True | ||
|
|
||
| valid_timeout = ((self._failure_detection_interval_ns - self._THREAD_SLEEP_NANO) // 2) * 10**9 |
There was a problem hiding this comment.
Suggested change
| valid_timeout = ((self._failure_detection_interval_ns - self._THREAD_SLEEP_NANO) // 2) * 10**9 | |
| valid_timeout = ((self._failure_detection_interval_ns - self._THREAD_SLEEP_NANO) // 2) // 10**9 |
There was a problem hiding this comment.
Should we switch all the 10**9 to 1_000_000_000 instead, not sure if the extra calculation has any performance impact
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Adding support for enhanced host monitoring V2 with documentation and tests.
Tested manual E2E tests and integration tests.
Porting from JDBC efm v2 implementation.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.