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

Fix cutting of response when traffic splits an edge into three parts #2943

Merged
merged 5 commits into from
Mar 17, 2021

Conversation

danpat
Copy link
Member

@danpat danpat commented Mar 17, 2021

Issue

When traffic splits an edge into three parts, we were looking at the wrong field to determine if the third segment needs to exist. Edges are not often split this many times, and when the bug occurred, it resulted a zero-length geometry, so not a huge impact.

Tasklist

  • Add tests
  • Add #fixes with the issue number that this PR addresses
  • Update the docs with any new request parameters or changes to behavior described
  • Update the changelog

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

@danpat danpat requested a review from purew March 17, 2021 03:54

// Regression is when breakpoint2 is set to 255,
// but speed3 is not set to UNKNOWN - we would incorrectly
// emit an additional 0-length slice in the API response
Copy link
Contributor

Choose a reason for hiding this comment

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

For my understanding, the case of speed3 or congestion3 being set while breakpoint2 is 255 is still an invalid state, but you're making the code more defensive to gracefully handle such an invalid state?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah - the old logic allowed for doing something in that case (breakpoint=255, but still emitting the third split). The code now correctly does not make the third split if breakpoint=255, so set values in speed3/congestion3 will be properly ignored.

Copy link
Contributor

@purew purew left a comment

Choose a reason for hiding this comment

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

LGTM

@kevinkreiser
Copy link
Member

feel free to ignore the non required windows build, theres a problem with CI rather than the code

@danpat danpat merged commit 7128246 into master Mar 17, 2021
@danpat danpat deleted the danpat_cutting_bug branch March 17, 2021 13:57
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.

3 participants