----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13771/#review25650 -----------------------------------------------------------
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java <https://reviews.apache.org/r/13771/#comment50147> 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? plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java <https://reviews.apache.org/r/13771/#comment50142> Nice change here - the old substring / calculate length stuff looks quite hacky! plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsTunnelManagerImpl.java <https://reviews.apache.org/r/13771/#comment50143> This comment implies there's an issue here, but doesn't suggest a fix - do you have a fix for it? server/src/com/cloud/configuration/ConfigurationManagerImpl.java <https://reviews.apache.org/r/13771/#comment50144> 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? server/src/com/cloud/network/NetworkManagerImpl.java <https://reviews.apache.org/r/13771/#comment50149> 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. server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java <https://reviews.apache.org/r/13771/#comment50150> Not sure what this comment means - is it meant to be left in? utils/src/com/cloud/utils/net/NetUtils.java <https://reviews.apache.org/r/13771/#comment50146> "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. This mostly looks good, but I had a few questions (inline), mostly around comments which seemed confusing. This 200 line patch was significantly easier to review than the previous 10k line version. :) - Dave Cahill 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 > >
