Moti Asayag has posted comments on this change.
Change subject: engine: Added 'OpenStack Network' provider proxy
......................................................................
Patch Set 21: (9 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/provider/network/openstack/OpenstackNetworkProviderProxy.java
Line 19: private Provider providerDetails;
Line 20:
Line 21: private QuantumClient quantumClient;
Line 22:
Line 23: public OpenstackNetworkProviderProxy(Provider providerDetails) {
not just simply 'provider' ?
Line 24: this.providerDetails = providerDetails;
Line 25: this.quantumClient = new
QuantumClient(providerDetails.getUrl());
Line 26: }
Line 27:
Line 23: public OpenstackNetworkProviderProxy(Provider providerDetails) {
Line 24: this.providerDetails = providerDetails;
Line 25: this.quantumClient = new
QuantumClient(providerDetails.getUrl());
Line 26: }
Line 27:
why the next following private getters are required ?
Line 28: private Provider getProviderDetails() {
Line 29: return providerDetails;
Line 30: }
Line 31:
Line 38: try {
Line 39: Networks networks = getQuantumClient().execute(new
ListNetworks());
Line 40: return map(networks.getList());
Line 41: } catch (RuntimeException e) {
Line 42: throw new VdcBLLException(VdcBllErrors.ENGINE, new
RuntimeException(e));
isn't wrapping the thrown exception with RuntimeException is redundant ?
also, how about producing a specific error for this failure ?
Line 43: }
Line 44: }
Line 45:
Line 46: /**
Line 44: }
Line 45:
Line 46: /**
Line 47: * Quantum implementation must call {@link
QuantumProviderClient#getAll()} since the root API returns an exception
Line 48: * when accessing it.
perhaps it is worth to query for a specific network (even with non-existing id)
and expect 200 or 420 (network not found) for testing connection so in a large
scale the amount of network won't encumber the engine ?
Line 49: */
Line 50: @Override
Line 51: public void testConnection() {
Line 52: getAll();
Line 51: public void testConnection() {
Line 52: getAll();
Line 53: }
Line 54:
Line 55: private List<Network>
map(List<org.openstack.quantum.model.Network> list) {
maybe providedNetworks or quantumNetworks has more meaning than 'list' ?
Line 56: ArrayList<Network> networks = new
ArrayList<Network>(list.size());
Line 57:
Line 58: for (org.openstack.quantum.model.Network quantumNetwork :
list) {
Line 59: Network network = new Network();
Line 52: getAll();
Line 53: }
Line 54:
Line 55: private List<Network>
map(List<org.openstack.quantum.model.Network> list) {
Line 56: ArrayList<Network> networks = new
ArrayList<Network>(list.size());
The type can be reduced to a List<Network>
Line 57:
Line 58: for (org.openstack.quantum.model.Network quantumNetwork :
list) {
Line 59: Network network = new Network();
Line 60: network.setVmNetwork(true);
Line 58: for (org.openstack.quantum.model.Network quantumNetwork :
list) {
Line 59: Network network = new Network();
Line 60: network.setVmNetwork(true);
Line 61: network.setProvidedBy(new
ProviderNetwork(getProviderDetails().getId(),
quantumNetwork.getId().toString()));
Line 62:
network.setId(Guid.createGuidFromString(quantumNetwork.getId().toString()));
why not to generate the ID ?
Line 63: network.setName(quantumNetwork.getName());
Line 64: networks.add(network);
Line 65: }
Line 66:
Line 59: Network network = new Network();
Line 60: network.setVmNetwork(true);
Line 61: network.setProvidedBy(new
ProviderNetwork(getProviderDetails().getId(),
quantumNetwork.getId().toString()));
Line 62:
network.setId(Guid.createGuidFromString(quantumNetwork.getId().toString()));
Line 63: network.setName(quantumNetwork.getName());
is there a chance quantum name might be longer than engine name?
Line 64: networks.add(network);
Line 65: }
Line 66:
Line 67: return networks;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/provider/ProviderProxyFactory.java
Line 21: * The provider to create the proxy for.
Line 22: * @return The proxy for communicating with the provider
Line 23: */
Line 24: @SuppressWarnings("unchecked")
Line 25: public <P extends ProviderProxy> P create(Provider provider) {
the implementation below doesn't comply with the current implementation of
ProviderProxyFactory.create() as exists in the master branch.
By which criteria will you determine the type of the ProviderProxy to be
created ?
Line 26: return (P) new OpenstackNetworkProviderProxy(provider);
Line 27: }
Line 28:
Line 29: /**
--
To view, visit http://gerrit.ovirt.org/10787
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f9ab9d0e4e8d6bc801c4825605aa435552152c0
Gerrit-PatchSet: 21
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Alona Kaplan <[email protected]>
Gerrit-Reviewer: Lior Vernia <[email protected]>
Gerrit-Reviewer: Livnat Peer <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
