CSHARP-4347: Add log messages to Server selection spec #1103
Conversation
JamesKovacs left a comment
JamesKovacs left a comment There was a problem hiding this comment.
Feedback and questions for your consideration.
| _rapidHeartbeatTimer.Change(TimeSpan.Zero, _minHeartbeatInterval); | ||
| } | ||
| | ||
| _serverSelectionEventLogger.LogAndPublish(new ClusterEnteredSelectionQueueEvent( |
There was a problem hiding this comment.
I was going to ask about allocating an object even if logging isn't enabled, but I see all the events are structs and thus won't incur heap allocation overhead. Very clever!
| _description, | ||
| selector, | ||
| -1, | ||
| EventContext.OperationName)); |
There was a problem hiding this comment.
Why do we return an operationId of null for SelectServer but -1 for SelectServerAsync?
| _server.Description, | ||
| TimeSpan.FromSeconds(1), | ||
| -1, | ||
| EventContext.OperationName)); |
There was a problem hiding this comment.
Same as above. Why -1 for async but null for sync?
There was a problem hiding this comment.
Thanks, should be null.
| e.ClusterDescription.ToString(), | ||
| "Server selection failed", | ||
| "exception123")); | ||
| //e.Exception?.ToString())); |
There was a problem hiding this comment.
Debug code exception123 should be replaced with commented impl on line 91.
| | ||
| // private methods | ||
| private IDisposable BeginOperation() => EventContext.BeginOperation(null, "listDatabases"); | ||
| |
There was a problem hiding this comment.
Missing calls to BeginOperation in Execute and ExecuteAsync.
There was a problem hiding this comment.
Above comment needs addressing.
There was a problem hiding this comment.
Thanks, BeginOperation removed.
| break; | ||
| | ||
| case "tags": | ||
| case "tagSets": |
There was a problem hiding this comment.
This looks unrelated. Move to another ticket?
JamesKovacs left a comment
JamesKovacs left a comment There was a problem hiding this comment.
One comment was missed.
| | ||
| // private methods | ||
| private IDisposable BeginOperation() => EventContext.BeginOperation(null, "listDatabases"); | ||
| |
There was a problem hiding this comment.
Above comment needs addressing.
2 participants
No description provided.