4964 add information about lacp connection status for lag ports#5040
4964 add information about lacp connection status for lag ports#5040
Conversation
| * @param logicalPortNumber the switch | ||
| * | ||
| */ | ||
| @ApiOperation(value = "Read all LACP status on specific switch", response = LacpStatusResponse.class) |
There was a problem hiding this comment.
Seems like description should be Read LACP status on specific port of the switch
Also, as long as you don't allow ports list or range as an input, you should return LacpStatusResponse, not List
|
|
||
| @Override | ||
| @Property(LOGICAL_PORT_NUMBER_PROPERTY) | ||
| public abstract void setLogicalPortNumber(int logicalPortNumber); |
There was a problem hiding this comment.
It seems to me, that using primitives make this field effectively mandatory non-null. Is that so for all cases here that they always have some value? Or is it possible that some of the ints or booleans are not set (are null)?
| assertTrue(lacpPartner.isPresent()); | ||
|
|
||
| lacpPartner = lacpPartnerRepository.findBySwitchIdAndLogicalPortNumber(TEST_SWITCH_ID_2, LOGICAL_PORT_NUMBER_1); | ||
| assertTrue(lacpPartner.isPresent()); |
There was a problem hiding this comment.
I think isPresent is not enough. If the repo returns a random result for all the calls, this test passes, but that's not ok. Please add more specific assertions.
| file: 023-add-lacp-reply-into-lag-class.yaml | ||
| - include: | ||
| relativeToChangelogFile: true | ||
| file: 025-add-lacp-partner-class.yaml |
There was a problem hiding this comment.
I'm not sure when migration 024 will be merged so please change your migration name to 024
| @@ -0,0 +1,39 @@ | |||
| /* Copyright 2017 Telstra Open Source | |||
| System.exit(handleLaunchException(e)); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Why did you move main method from the end of the file?
There was a problem hiding this comment.
Its formatted by IntelliJ, Updated.
| } | ||
|
|
||
| private void createLacpSpout(TopologyBuilder builder) { | ||
| declareKafkaSpout(builder, topologyConfig.getKafkaLacpTopic(), LACP_SPOUT_ID); |
There was a problem hiding this comment.
You forgot to add LACP_SPOUT_ID into input of RouterBolt. It should be
private void createRouterBolt(TopologyBuilder builder, PersistenceManager persistenceManager) {
RouterBolt routerBolt = new RouterBolt(persistenceManager, ZooKeeperSpout.SPOUT_ID);
declareBolt(builder, routerBolt, ROUTER_BOLT_ID)
.shuffleGrouping(CONNECTED_DEVICES_SPOUT_ID)
.shuffleGrouping(LACP_SPOUT_ID)
.allGrouping(ZooKeeperSpout.SPOUT_ID);
}
| * <code>SwitchConnectedDeviceFrame.UNIQUE_INDEX_PROPERTY</code> | ||
| */ | ||
| public static String createMessageKey(LacpInfoData data) { | ||
| return String.format("%s_%s_arp", data.getSwitchId(), data.getLogicalPortNumber()); |
There was a problem hiding this comment.
your key has postfix arp but it should be lacp
| /** | ||
| * This key is needed to balance load on Packet Bolt. If you see that some packet bolts have high load, and | ||
| * some have low load, try to extend this key. Maximum extension is equal to | ||
| * <code>SwitchConnectedDeviceFrame.UNIQUE_INDEX_PROPERTY</code> |
There was a problem hiding this comment.
Last sentence should be removed because Lacp DB frame has no UNIQUE_INDEX_PROPERTY
5 participants
No description provided.