CT 1440 Fix code to emit ConnectionReused event#6605
Conversation
JCZuurmond
left a comment
JCZuurmond
left a comment
There was a problem hiding this comment.
Hi @gshank, thanks for cleaning up this code and fixing the reused connection message. I added some comments.
| Creates a connection for this thread if one doesn't already | ||
| exist, and will rename an existing connection.""" | ||
|
|
||
| conn_name: str = "master" if name is None else name |
There was a problem hiding this comment.
Type hint should be : str | None or : Optional[str]
There was a problem hiding this comment.
It can't be Optional[str] because the logging events are defined in protobufs which don't have Optional[str].
| # Get a connection for this thread | ||
| conn = self.get_if_exists() | ||
|
|
||
| if conn and conn.name == conn_name and conn.state == "open": |
There was a problem hiding this comment.
Do we only consider a connection to be reused if the name changed? Reading the code I would expect this to also be a connection reused.
There was a problem hiding this comment.
It's currently not considered reused. Of course the code is so strangely factored that it's a bit hard to tell how it ought to be used.
| if conn.state != "open": | ||
| conn.handle = LazyHandle(self.open) | ||
| if conn.name != conn_name: | ||
| orig_conn_name: str = conn.name or "" |
There was a problem hiding this comment.
Why switch to an empty string here? I would expect the same type being used everywhere either str | None or str
There was a problem hiding this comment.
I misread the type above
gshank
force-pushed
the
ct-1440-connection_reused
branch
from
f74d9bf to
ef36c91
Compare
3 participants
resolves #6168
Description
The code in the adapter "set_connection_name" method was not correctly emitting a ConnectionReused event. Refactor code to make it more understandable and emit the right logging event.
Checklist
changie newto create a changelog entry