Skip to content
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

Improve ping API a bit by returning failure in case of only failure #2813

Merged
merged 1 commit into from
Jun 12, 2016

Conversation

Kubuxu
Copy link
Member

@Kubuxu Kubuxu commented Jun 6, 2016

Part of #2803
License: MIT
Signed-off-by: Jakub Sztandera [email protected]

@Kubuxu Kubuxu added the need/review Needs a review label Jun 7, 2016

ctx, cancel := context.WithTimeout(ctx, kPingTimeout*time.Duration(numPings))
defer cancel()
pings, err := n.Ping.Ping(ctx, pid)
if err != nil {
log.Debugf("Ping error: %s", err)
outChan <- &PingResult{Text: fmt.Sprintf("Ping error: %s", err)}
outChan <- &PingResult{
Text: fmt.Sprintf("Ping error: %s", err)}
Copy link
Member

Choose a reason for hiding this comment

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

lets explicitly set Success false here (for clarity to the reader), and then put the closing bracket on its own line.

@Kubuxu
Copy link
Member Author

Kubuxu commented Jun 8, 2016

Updated.

@whyrusleeping whyrusleeping merged commit ecbca7e into master Jun 12, 2016
@whyrusleeping whyrusleeping deleted the feature/ping-api branch June 12, 2016 01:40
@jbenet
Copy link
Member

jbenet commented Aug 27, 2016

Not quite sure what this does-- the PR title doesn't tell me, and the code is not clear why-- maybe add some comments why some return success and one fail?

@Kubuxu
Copy link
Member Author

Kubuxu commented Aug 27, 2016

We have boolean filed Success which as boolean fields default to false, was implying failure in NDJSON messages that didn't fail. Now, we explicitly set success to true on all of them and to false if there is failure.

@jbenet jbenet mentioned this pull request Aug 28, 2016
58 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants