-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
refactor transport socket merge logic #36619
Conversation
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
// Now check if this matches if existing cluster's transport socket. | ||
if c.GetTransportSocket() != nil { | ||
if cpValueCast.GetTransportSocket().Name == c.GetTransportSocket().Name { | ||
tsmPatch = c.GetTransportSocket() | ||
} |
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 would suggest, although in theory c.GetTransportSocket() != nil
and ·len(c.GetTransportSocketMatches()) > 0
can not be true simultanously
if cpValueCast.GetTransportSocket() != nil {
if len(c.GetTransportSocketMatches()) > 0 {
} else if c.GetTransportSocket() != nil{
}
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.
Ok. Changed.
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
break | ||
} | ||
} | ||
if tsmPatch == nil && len(c.GetTransportSocketMatches()) > 0 { |
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.
len(c.GetTransportSocketMatches()) > 0
checked in L81
// There is a name mismatch, so we cannot do a deep merge. Instead just replace the transport socket | ||
// This means either there is a name mismatch or cluster does not have transport socket matches/transport socket. | ||
// We cannot do a deep merge. Instead just replace the transport socket | ||
if tsmPatch == nil { |
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.
Isn't this shoube under else if c.GetTransportSocket() != nil
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.
It really does not matter. But refactored a bit more to simplify further. PTAL.
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
No functional change. Just refactors merge transport logic to simplify if conditions and also when the cluster does not have transport socket avoids proto.Merge call.