Conversation
pooknull
requested review from
egegunes,
eleo007,
gkech,
hors,
jvpasinatto and
nmarukovich
as code owners
| "github.com/percona/percona-server-mongodb-operator/pkg/psmdb/mongo" | ||
| ) | ||
|
|
||
| func getStatus(ctx context.Context, client mongo.Client) (ReplSetStatus, error) { |
There was a problem hiding this comment.
Why don't we have all the mongo client-related functions together as part of the type Client interface? I understand that we are not committing to the interface segregation rule by doing that, but that interface is already containing everything (almost in terms of functionality).
Also the response type seems related to the generic mongo model and maybe can be moved to the mongo model file.
type ReplSetStatus struct {
...
}
This removes the need to have a utils file completely.
| if err != nil { | ||
| log.Error(err, "Failed to get replset status") | ||
| return nil |
There was a problem hiding this comment.
i wonder if we should ignore all errors or only this node is not a member of replset?
| func CheckState(rs ReplSetStatus, startupDelaySeconds int64, oplogSize int64) error { | ||
| func CheckState(rs mongo.Status, startupDelaySeconds int64, oplogSize int64) error { | ||
| if rs.GetSelf() == nil { | ||
| return errors.New("invalid replset status") |
There was a problem hiding this comment.
is this error message right?
|
|
||
| func CheckState(rs ReplSetStatus, startupDelaySeconds int64, oplogSize int64) error { | ||
| func CheckState(rs mongo.Status, startupDelaySeconds int64, oplogSize int64) error { | ||
| if rs.GetSelf() == nil { |
There was a problem hiding this comment.
Given that on L126 we are using again rs.GetSelf, assigning here to a variable, then performing the nil check and then using it in the remaining function is better since that function is looping through the members and it is not needed for every invocation.
|
|
||
| var d net.Dialer | ||
|
|
||
| addr := cnf.Hosts[0] |
There was a problem hiding this comment.
Should we ensure that hosts are not empty/nil?
| cnf.Timeout = time.Second | ||
| client, err := db.Dial(ctx, cnf) | ||
| if err != nil { | ||
| return nil, nil |
There was a problem hiding this comment.
Why are we swallowing this error?
| return &rs, nil | ||
| }() | ||
| if err != nil || s == nil { | ||
| return err |
There was a problem hiding this comment.
Let's add wrap some context to this error, MongodReadinessCheck already returns multiple errors
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 221 out of 221 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
egegunes
left a comment
egegunes
left a comment
There was a problem hiding this comment.
please check #1917 (comment)
hors
requested review from
bogdanjeler-ev,
mayankshah1607 and
oksana-grishchenko
as code owners
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 223 out of 223 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
commit: ea52e0c |
https://perconadev.atlassian.net/browse/K8SPSMDB-1296
DESCRIPTION
This PR improves readiness probe by verifying the
stateStrfield in thereplSetGetStatusoutput. If it's not possible to execute the command, the readiness probe will not fail, because otherwise it wouldn't be possible to deploy a mongod statefulset. The readiness probe will fail if the value of thestateStris not equal toPrimary,SecondaryorArbiterCHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
compare/*-oc.yml)?Config/Logging/Testability