-
Notifications
You must be signed in to change notification settings - Fork 533
SPEC-222: SDAM Monitoring #746
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| topologyId: "42" | ||
| address: "a:27017" | ||
| previousDescription: | ||
| address: "127.0.0.1:27017" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This address surprises me. Where does it come from? Why would a driver have that as the default address? I'd expect instead that the previousDescriptions's address would be the same as the newDescription's address, but with type "Unknown" (as it is).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I had read in the SDAM spec as well about the default for the server description but reading it again I don't see it... So maybe leave it out? I think some implementations will default with an "empty" description and some might default with the address provided and type unknown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The address should be "a:27017" in previousDescription and type "Unknown". I don't see how it makes sense to have an empty address for a server description changed event. The address is part of the top-level event metadata, so both descriptions should share it.
|
@jyemin Updated per the comments, I've reduced the fields to the important ones and fixed the defaults. |
| previousDescription: | ||
| topologyType: "Unknown" | ||
| setName: null | ||
| servers: [ "a:27017" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have "b:27017" as well.
It should also be an array of ServerDescription instead of array of string
|
Replaced by: #834 |
No description provided.