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

diff reachability: only use start locations present and active in both snapshots #5236

Merged
merged 3 commits into from
Dec 12, 2019

Conversation

anothermattbrown
Copy link
Contributor

No description provided.

@batfish-bot
Copy link

This change is Reviewable

@codecov
Copy link

codecov bot commented Dec 12, 2019

Codecov Report

Merging #5236 into master will decrease coverage by <.01%.
The diff coverage is 91.3%.

@@             Coverage Diff              @@
##             master    #5236      +/-   ##
============================================
- Coverage     73.33%   73.33%   -0.01%     
+ Complexity    31834    31829       -5     
============================================
  Files          2610     2610              
  Lines        128396   128411      +15     
  Branches      15423    15424       +1     
============================================
+ Hits          94163    94168       +5     
- Misses        26794    26804      +10     
  Partials       7439     7439
Impacted Files Coverage Δ Complexity Δ
...reachability/DifferentialReachabilityAnswerer.java 93.54% <91.3%> (-2.2%) 3 <1> (ø)
.../src/main/java/org/batfish/datamodel/flow/Hop.java 53.33% <0%> (-6.67%) 5% <0%> (-1%)
...org/batfish/datamodel/flow/BidirectionalTrace.java 79.54% <0%> (-2.28%) 13% <0%> (-1%)
...col/src/main/java/org/batfish/role/InferRoles.java 89.54% <0%> (-1.37%) 50% <0%> (-1%)
...tfish/representation/cisco/CiscoConfiguration.java 85.48% <0%> (-0.13%) 519% <0%> (-1%)

Copy link
Contributor

@progwriter progwriter left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @anothermattbrown)


projects/question/src/main/java/org/batfish/question/differentialreachability/DifferentialReachabilityAnswerer.java, line 74 at r2 (raw file):

    // requiredTransitNodes can be different in each snapshot. flow must transit any one
    Set<String> requiredTransitNodes =
        Sets.union(

I would have assumed this has to be intersection. must transit same node(s) in both snapshots?

Copy link
Contributor Author

@anothermattbrown anothermattbrown left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @progwriter)


projects/question/src/main/java/org/batfish/question/differentialreachability/DifferentialReachabilityAnswerer.java, line 74 at r2 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

I would have assumed this has to be intersection. must transit same node(s) in both snapshots?

semantics of required transit nodes is that any (i.e. one) of the specified nodes must be transited. we do not enforce that the transited node (can) be the same in both snapshots

Copy link
Contributor

@progwriter progwriter left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


projects/question/src/main/java/org/batfish/question/differentialreachability/DifferentialReachabilityAnswerer.java, line 74 at r2 (raw file):

Previously, anothermattbrown (Matt Brown) wrote…

semantics of required transit nodes is that any (i.e. one) of the specified nodes must be transited. we do not enforce that the transited node (can) be the same in both snapshots

seem counterintuitive but ok

Copy link
Contributor Author

@anothermattbrown anothermattbrown left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


projects/question/src/main/java/org/batfish/question/differentialreachability/DifferentialReachabilityAnswerer.java, line 74 at r2 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

seem counterintuitive but ok

yeah we could do much more powerful waypointing. probably will some day 😉

@anothermattbrown anothermattbrown merged commit 0816203 into master Dec 12, 2019
@anothermattbrown anothermattbrown deleted the matt-diffReachStartLocs branch December 12, 2019 22:25
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.

None yet

3 participants