> On Aug. 28, 2013, 2:05 a.m., Dave Cahill wrote: > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java, > > line 100 > > <https://reviews.apache.org/r/13771/diff/2/?file=345161#file345161line100> > > > > Not an issue, just to jog my memory - we're using static methods on > > BroadcastDomainType instead of member method on the URI because we don't > > know if the URI will have a getValue method, right?
not quite, the static method allows for getting the value when the enumeration value is not known (yet) > On Aug. 28, 2013, 2:05 a.m., Dave Cahill wrote: > > plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsTunnelManagerImpl.java, > > line 248 > > <https://reviews.apache.org/r/13771/diff/2/?file=345173#file345173line248> > > > > This comment implies there's an issue here, but doesn't suggest a fix - > > do you have a fix for it? I am adding to the existing comment here. It implies everything is alright whilst it may not. My fix here may/should resolve the issue. I will change the '(not)' into 'now'. Hope that removes your issue. > On Aug. 28, 2013, 2:05 a.m., Dave Cahill wrote: > > server/src/com/cloud/configuration/ConfigurationManagerImpl.java, line 2013 > > <https://reviews.apache.org/r/13771/diff/2/?file=345175#file345175line2013> > > > > Do you mean "why not have"? This comment seems to add confusion - if it > > should be changed, maybe propose a change? What would we gain from adding a > > None BroadcastDomainType? Is null handling working correctly already? You are right. I will drop this comment. It was expressing my own confusion. > On Aug. 28, 2013, 2:05 a.m., Dave Cahill wrote: > > server/src/com/cloud/network/NetworkManagerImpl.java, line 1904 > > <https://reviews.apache.org/r/13771/diff/2/?file=345179#file345179line1904> > > > > Looks like this changes behavior - if isolatePvlan is null, userNetwork > > now gets its broadcast URI set anyway. > > > > Could you explain why we're making that change? I'm sure it's for a > > reason, I just can't tell why from a quick read. thanks for pointing this out. It is not an issue as it get overwritten in the else {} clause, but it is superfluous at this location. > On Aug. 28, 2013, 2:05 a.m., Dave Cahill wrote: > > server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java, > > line 796 > > <https://reviews.apache.org/r/13771/diff/2/?file=345185#file345185line796> > > > > Not sure what this comment means - is it meant to be left in? this is a question I can not answer. I will remove the TODO part > On Aug. 28, 2013, 2:05 a.m., Dave Cahill wrote: > > utils/src/com/cloud/utils/net/NetUtils.java, line 1381 > > <https://reviews.apache.org/r/13771/diff/2/?file=345186#file345186line1381> > > > > "these pvlan functions should take into account code in > > Networks.BroadcastDomainType" > > > > In what way? As it is, I think this comment is not so helpful. I am warning future maintainers for the relation with the mentioned code. I will try to rephrase to make it more clear. - daan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13771/#review25650 ----------------------------------------------------------- On Aug. 27, 2013, 11:36 a.m., daan Hoogland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13771/ > ----------------------------------------------------------- > > (Updated Aug. 27, 2013, 11:36 a.m.) > > > Review request for cloudstack, Chiradeep Vittal, Dave Cahill, Hugo Trippaers, > and Sheng Yang. > > > Bugs: CLOUDSTACK-4346 > > > Repository: cloudstack-git > > > Description > ------- > > After global search and replace all calls to retrieve ids for networks from > URIs using getHost() should be gone. Creating URI should now all use > appropriate calls as well so maitaining the way uris are built can now be > done centrally. > > > Diffs > ----- > > > plugins/hypervisors/baremetal/src/com/cloud/baremetal/networkservice/BaremetaNetworkGuru.java > 07ee12d > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java > 195cf40 > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java > a156ae6 > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/OvsVifDriver.java > 7038d7e > plugins/hypervisors/ovm/src/com/cloud/ovm/hypervisor/OvmResourceBase.java > 59ba001 > > plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java > 5ab2216 > > plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java > ecdec1e > > plugins/network-elements/bigswitch-vns/src/com/cloud/network/element/BigSwitchVnsElement.java > 54623e9 > > plugins/network-elements/cisco-vnmc/src/com/cloud/network/element/CiscoVnmcElement.java > 3ae6a08 > > plugins/network-elements/f5/src/com/cloud/network/resource/F5BigIpResource.java > 1733712 > > plugins/network-elements/juniper-srx/src/com/cloud/network/resource/JuniperSrxResource.java > 3d3d797 > > plugins/network-elements/nicira-nvp/src/com/cloud/network/element/NiciraNvpElement.java > c7d0884 > > plugins/network-elements/nicira-nvp/src/com/cloud/network/guru/NiciraNvpGuestNetworkGuru.java > ff238ed > > plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsTunnelManagerImpl.java > 36a807f > server/src/com/cloud/api/ApiResponseHelper.java c771431 > server/src/com/cloud/configuration/ConfigurationManagerImpl.java 57dc0b3 > server/src/com/cloud/network/ExternalDeviceUsageManagerImpl.java e91dcfa > server/src/com/cloud/network/ExternalFirewallDeviceManagerImpl.java a934024 > server/src/com/cloud/network/ExternalLoadBalancerDeviceManagerImpl.java > c14d5c7 > server/src/com/cloud/network/NetworkManagerImpl.java 00103e3 > server/src/com/cloud/network/guru/DirectPodBasedNetworkGuru.java 5b87d54 > server/src/com/cloud/network/guru/ExternalGuestNetworkGuru.java 00598dd > server/src/com/cloud/network/guru/GuestNetworkGuru.java b0da42f > server/src/com/cloud/network/guru/PrivateNetworkGuru.java 6521cf4 > server/src/com/cloud/network/guru/PublicNetworkGuru.java d109468 > > server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java > ee0d058 > utils/src/com/cloud/utils/net/NetUtils.java 05b485b > > Diff: https://reviews.apache.org/r/13771/diff/ > > > Testing > ------- > > tested with old style uris in regular networks and vpc based networks as well > as in nicira based networks > test build in nonoss but not all code has probably been touched yet. or at > least I am unsure of that. > > > Thanks, > > daan Hoogland > >
