Add NextHop column to Routes answerer #7838
Conversation
- add `Schema.NEXT_HOP` - populate `COL_NEXT_HOP` using modern `NextHop` route attribute
|
This change is |
There was a problem hiding this comment.
Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion
a discussion (no related file):
self-merge only
Codecov Report
@@ Coverage Diff @@
## master #7838 +/- ##
============================================
- Coverage 74.05% 74.04% -0.01%
- Complexity 42272 42279 +7
============================================
Files 3313 3314 +1
Lines 166526 166590 +64
Branches 19940 19945 +5
============================================
+ Hits 123313 123346 +33
- Misses 33677 33698 +21
- Partials 9536 9546 +10
|
There was a problem hiding this comment.
Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @arifogel and @dhalperi)
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/table/Row.java, line 322 at r1 (raw file):
} public @Nullable NextHop getNextHop(String column) {
why Nullable? should there always be some nexthop?
Code quote:
public @Nullable NextHop getNextHop(String column) {
return (NextHop) get(column, Schema.NEXT_HOP);
}projects/batfish-common-protocol/src/test/java/org/batfish/common/util/NextHopComparatorTest.java, line 20 at r1 (raw file):
@Test public void testCompareClass() {
nit, for this test, would be slightly more readable / easier to maintain if the different NextHop objects were stored in variables instead of repeated for each test
NextHopInterface nhIface = NextHopInterface.of("foo");
...
projects/question/src/main/java/org/batfish/question/routes/RoutesAnswerer.java, line 253 at r1 (raw file):
private static final Comparator<Row> BGP_COMPARATOR = Comparator.<Row, String>comparing(row -> row.getNode(COL_NODE).getName()) .thenComparing(row -> row.getString(COL_VRF_NAME))
is this comparator needed anymore?
Code quote:
private static final Comparator<Row> BGP_COMPARATOR =
Comparator.<Row, String>comparing(row -> row.getNode(COL_NODE).getName())
.thenComparing(row -> row.getString(COL_VRF_NAME))
.thenComparing(row -> row.getPrefix(COL_NETWORK))
.thenComparing(row -> row.getNextHop(COL_NEXT_HOP), NextHopComparator.instance());projects/question/src/main/java/org/batfish/question/routes/RoutesAnswerer.java, line 360 at r1 (raw file):
COL_NEXT_HOP, Schema.NEXT_HOP, "Route's Next Hop IP",
this column is not IP
Code quote:
"Route's Next Hop IP",
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dhalperi and @sfraint)
projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/table/Row.java, line 322 at r1 (raw file):
Previously, sfraint (Spencer Fraint) wrote…
why
Nullable? should there always be some nexthop?
Should be null for some route diffs IIRC.
projects/batfish-common-protocol/src/test/java/org/batfish/common/util/NextHopComparatorTest.java, line 20 at r1 (raw file):
Previously, sfraint (Spencer Fraint) wrote…
nit, for this test, would be slightly more readable / easier to maintain if the different NextHop objects were stored in variables instead of repeated for each test
NextHopInterface nhIface = NextHopInterface.of("foo"); ...
done
projects/question/src/main/java/org/batfish/question/routes/RoutesAnswerer.java, line 253 at r1 (raw file):
Previously, sfraint (Spencer Fraint) wrote…
is this comparator needed anymore?
Why wouldn't it be? It has two usages in answer:
switch (question.getRib()) {
case BGP:
rows.addAll(
getBgpRibRoutes(
dp.getBgpRoutes(),
dp.getBgpBackupRoutes(),
matchingVrfsByNode,
network,
protocolSpec,
expandedBgpRouteStatuses,
question.getPrefixMatchType()));
rows.sort(BGP_COMPARATOR);
break;
case EVPN:
rows.addAll(
getEvpnRoutes(
dp.getEvpnRoutes(),
dp.getEvpnBackupRoutes(),
matchingVrfsByNode,
network,
protocolSpec,
ImmutableSet.of(BEST, BACKUP),
question.getPrefixMatchType()));
rows.sort(BGP_COMPARATOR);
break;
projects/question/src/main/java/org/batfish/question/routes/RoutesAnswerer.java, line 360 at r1 (raw file):
Previously, sfraint (Spencer Fraint) wrote…
this column is not
IP
fixed
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @arifogel and @dhalperi)
projects/question/src/main/java/org/batfish/question/routes/RoutesAnswerer.java, line 253 at r1 (raw file):
Previously, arifogel (Ari Fogel) wrote…
Why wouldn't it be? It has two usages in
answer:switch (question.getRib()) { case BGP: rows.addAll( getBgpRibRoutes( dp.getBgpRoutes(), dp.getBgpBackupRoutes(), matchingVrfsByNode, network, protocolSpec, expandedBgpRouteStatuses, question.getPrefixMatchType())); rows.sort(BGP_COMPARATOR); break; case EVPN: rows.addAll( getEvpnRoutes( dp.getEvpnRoutes(), dp.getEvpnBackupRoutes(), matchingVrfsByNode, network, protocolSpec, ImmutableSet.of(BEST, BACKUP), question.getPrefixMatchType())); rows.sort(BGP_COMPARATOR); break;
Sorry, I meant: is this comparator identical to the MAIN_RIB_COMPARATOR now that nexthop column comparison is consolidated? if so, do we need to keep two separate, identical comparator instances or can we migrate all usage onto the main rib one?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dhalperi and @sfraint)
projects/question/src/main/java/org/batfish/question/routes/RoutesAnswerer.java, line 253 at r1 (raw file):
Previously, sfraint (Spencer Fraint) wrote…
Sorry, I meant: is this comparator identical to the
MAIN_RIB_COMPARATORnow that nexthop column comparison is consolidated? if so, do we need to keep two separate, identical comparator instances or can we migrate all usage onto the main rib one?
Ah gotcha. It's possible we may want to sort by some BGP-rib-specific columns in the future. I'd prefer to investigate and address in a separate PR, since I consider that to be out of scope for this PR.
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dhalperi and @sfraint)
Schema.NEXT_HOPCOL_NEXT_HOPusing modernNextHoproute attribute