Skip to content
This repository was archived by the owner on Oct 23, 2024. It is now read-only.

Stability fixes #461

Merged
merged 1 commit into from
Aug 25, 2016
Merged

Stability fixes #461

merged 1 commit into from
Aug 25, 2016

Conversation

sargun
Copy link
Contributor

@sargun sargun commented Aug 23, 2016

No description provided.

sj, err = rg.loadFromMaster(rip, port)
return sj, err
if sj.Leader != "" {
// FIXME(prozlach): Still, we are assuming that sj.Leader is a correct
Copy link
Contributor Author

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?

@sargun
Copy link
Contributor Author

sargun commented Aug 23, 2016

Is there any way we can add tests to confirm this robustness? @drewkerrigan

@drewkerrigan
Copy link
Contributor

@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.

@sargun
Copy link
Contributor Author

sargun commented Aug 24, 2016

LGTM. @jdef

return "", errors.New("Invalid leader address: " + leader)
}
hostPort := nameAddressPair[1]
hostPortPair := strings.Split(hostPort, ":")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use net.SplitHostPort

@jdef
Copy link
Contributor

jdef commented Aug 24, 2016

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?

@drewkerrigan
Copy link
Contributor

@jdef I've addressed your comments, let me know if you see anything else. The PR you're referring to is: #412 - this is an updated version.

@sargun
Copy link
Contributor Author

sargun commented Aug 24, 2016

@jdef Github refused to update the diff correctly on #412.

@@ -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")
Copy link
Contributor Author

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?

@sargun
Copy link
Contributor Author

sargun commented Aug 25, 2016

A couple more nits.

@drewkerrigan
Copy link
Contributor

@sargun nits addressed, squash?

@jdef
Copy link
Contributor

jdef commented Aug 25, 2016

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
@sargun sargun merged commit bfcd401 into master Aug 25, 2016
@sargun sargun deleted the stability-fixes branch August 25, 2016 16:23
@jdef jdef removed the WIP label Aug 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants