-
Notifications
You must be signed in to change notification settings - Fork 68
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
CLOUDP-65729: Add sharded cluster support to Ops Manager #217
Conversation
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.
Just a couple of comments, lgtm as far as I can see. I don't have the full context so I may have missed things
in.Processes = append(in.Processes[:k], in.Processes[k+1:]...) | ||
out[i].ProcessConfigs[j] = newReplicaSetProcessConfig(m, p) | ||
out[i].addToMongoURI(p) | ||
in.Processes = removeProcess(in.Processes, k) |
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 there a reason to modify the in
argument?
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.
since this is such a complex search (O(n*m*o)
) I'm trying to speed up search by deleting visited processes
I want at some point benchmark this to see how else can we improve it
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.
ah got it
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 changed the name from in and added a comment about how this method is dangerous
|
||
if constrain.Check(ver) { | ||
return zero, nil | ||
func patchSharding(out *opsmngr.AutomationConfig, s *opsmngr.ShardingConfig) { |
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.
So this doesn't modify existing sharding configs -- is that not supported? Might be worth putting a comment for the function
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 just didn't want to add test for this and manual test takes all my resources, I've added a TODO with a CLOUDP to test it later
internal/convert/rs_config.go
Outdated
|
||
type patcher func(*ProcessConfig, string) *opsmngr.Process | ||
|
||
// path is a generic replica set patcher, you'll need to provide a function that |
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.
[np] missing rest of comment
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.
not only that the method name had a typo, fixed
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.
@robcarlan-mlab RFAL
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.
lgtm!
Proposed changes
Jira ticket: CLOUDP-65729
Checklist
make fmt
and formatted my codeFurther comments
This adds support to list and create sharded clusters, update is still untested
Open to suggestion on naming specially of
RSConfig
struct