-
Notifications
You must be signed in to change notification settings - Fork 13
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
Implement isMaster with tests for replica and non-replica scenario #60
Implement isMaster with tests for replica and non-replica scenario #60
Conversation
@@ -0,0 +1,5 @@ | |||
initialization | |||
initializeWith: aCollection |
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.
Is it a collection or dictionary?
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.
More particularly a dictionary, but when somebody presses on "create" in the debugger (in a DNU), the parameter has this name. This was defined in some method of the hierarchy, not sure if it's on purpose or a bug.
@@ -0,0 +1,5 @@ | |||
replica set | |||
urlString |
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.
How is this used? Can you think of a better name?
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 intent was to differenciate from the cases when a ZnUrl is useful vs this string version of the url that always comes in the responses. May be host
?
Looks good to me. |
isMaster | ||
|
||
| reply | | ||
reply := self command: (OrderedDictionary new at: #ismaster put: 1; yourself). |
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.
What's the reason to use OrderedIdentityDictionary in many other places?
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.
Frankly I don't know, a reason may be to allow repeated String keys but I don't know in which special cases. I think we should unify how the "JSON dictionaries" (Dictionaries that will be converted to json in a mongo call) are created.
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 created issue #61 to maybe discuss more about this.
MongoDB drivers and clients use isMaster to determine the state of the replica set members and to discover additional members of a replica set. | ||
|
||
|
||
Read more at: https://docs.mongodb.com/v4.0/reference/command/isMaster/ |
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.
In the other PR we are linking to the most recent docs. It's fine to link to the doc used during impl or latest but we should aim to be consistent.
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.
Good catch. What do you think is the best approach?
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.
Link to the document at the time of the implementation. One can go to newer versions (if it exists) then but not the other way around.
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 will take it into account for next time.
Do you consider it's good overall? |
Yes. I consider this good over all. I don't have the power to merge it though. ;) |
Fixes #59 plus few secondary additions: