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

Clarify ping RPC expected behavior #85

Merged
merged 1 commit into from
Sep 8, 2022

Conversation

bstoll
Copy link
Contributor

@bstoll bstoll commented Jun 29, 2022

While looking into a test to verify ping behavior, I became unsure of what is expected in the PingResponse stream. It looks like it would be valid to return individual packets OR summary statistics OR both the individual packets and the summary.

This change is to clarify that summary statistics are always expected, and individual packet metrics should be returned (ie, it's probably not reasonable for a flood request).

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2586440683

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%

Totals Coverage Status
Change from base Build 2503260217: 0.0%
Covered Lines: 0
Relevant Lines: 0

💛 - Coveralls

@hellt
Copy link
Contributor

hellt commented Jun 30, 2022

Since there is no option in the PingRequest to ask for a flood ping, shall we assume that we also always request single ping stats to be returned?
The way I read the spec is that both single ping responses and summary responses are expected

@bstoll
Copy link
Contributor Author

bstoll commented Jun 30, 2022

A flood ping is specified by setting "interval" to -1.

@hellt
Copy link
Contributor

hellt commented Jun 30, 2022

my bad @bstoll
I can't read

@marcushines marcushines merged commit ec7a8ab into openconfig:master Sep 8, 2022
@bstoll bstoll deleted the ping-behavior branch November 18, 2022 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants