-
Notifications
You must be signed in to change notification settings - Fork 135
Stability fixes #461
Stability fixes #461
Conversation
35019f3
to
22896ba
Compare
sj, err = rg.loadFromMaster(rip, port) | ||
return sj, err | ||
if sj.Leader != "" { | ||
// FIXME(prozlach): Still, we are assuming that sj.Leader is a correct |
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 think we'll crash here? Right?
Is there any way we can add tests to confirm this robustness? @drewkerrigan |
@sargun I added some simple validation in records.leaderIP and added an error to the return. Invalid states should now return an error back to the caller to deal with. |
LGTM. @jdef |
return "", errors.New("Invalid leader address: " + leader) | ||
} | ||
hostPort := nameAddressPair[1] | ||
hostPortPair := strings.Split(hostPort, ":") |
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.
use net.SplitHostPort
added some nit-pick comments. look similar to another PR that I saw a while back. if this is just an updated version of that PR, would you please xref? |
@@ -170,7 +170,7 @@ func (rg *RecordGenerator) ParseState(c Config, masters ...string) error { | |||
// find master -- return if error | |||
sj, err := rg.findMaster(masters...) | |||
if err != nil { | |||
logging.Error.Println("no master") | |||
logging.Error.Println("Failed to fetch state.json") |
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.
Why don't we print the error here, when on line 210 we print the error?
A couple more nits. |
cd1e0dc
to
9b6c91b
Compare
@sargun nits addressed, squash? |
lgtm, please squash |
Improve state.json fetch logic error messages Make findMaster method more robust loadWrap method should not assume "Leader" field presence in reply adding error content to log lines, removing unecessary log line added validation and tests for leaderIP
9b6c91b
to
353fa8d
Compare
No description provided.