Liron Aravot has posted comments on this change.
Change subject: core: Use storage helper to fetch (related to BZ882825)
......................................................................
Patch Set 1: (6 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ISCSIStorageHelper.java
Line 19
Line 20
Line 21
Line 22
Line 23
seems like this change was added to the wrong patch, not related to this one
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageHelperBase.java
Line 124: public boolean isConnectSucceeded(Map<String, String> returnValue,
Line 125: List<storage_server_connections> connections) {
Line 126: return true;
Line 127: }
Line 128:
method name isn't correct - the method filters connections by type.
Line 129: public static Map<StorageType, List<storage_server_connections>>
filterLUNsByStorageType(LUNs lun) {
Line 130: Map<StorageType, List<storage_server_connections>>
storageConnectionsForStorageTypeMap =
Line 131: new HashMap<StorageType,
List<storage_server_connections>>();
Line 132: for (storage_server_connections lunConnections :
lun.getLunConnections()) {
Line 125: List<storage_server_connections> connections) {
Line 126: return true;
Line 127: }
Line 128:
Line 129: public static Map<StorageType, List<storage_server_connections>>
filterLUNsByStorageType(LUNs lun) {
please use EnumMap instead
Line 130: Map<StorageType, List<storage_server_connections>>
storageConnectionsForStorageTypeMap =
Line 131: new HashMap<StorageType,
List<storage_server_connections>>();
Line 132: for (storage_server_connections lunConnections :
lun.getLunConnections()) {
Line 133: StorageType storageType =
lunConnections.getstorage_type();
Line 127: }
Line 128:
Line 129: public static Map<StorageType, List<storage_server_connections>>
filterLUNsByStorageType(LUNs lun) {
Line 130: Map<StorageType, List<storage_server_connections>>
storageConnectionsForStorageTypeMap =
Line 131: new HashMap<StorageType,
List<storage_server_connections>>();
general questions about this change
the map will contain only iscsi storage type as key always , do we plan to have
another connection type for lun (local and fcp doesn't have connections)? what
will be the use of that map if it always has only ISCSI type?
why do we need connection list in LUN and also to create the map each time? why
not to replace the list of connection in LUN entity with map and do this when
loading the connections? right now it seems to me that we will have to repeat
it every time and will need to map always
Line 132: for (storage_server_connections lunConnections :
lun.getLunConnections()) {
Line 133: StorageType storageType =
lunConnections.getstorage_type();
Line 134: if
(!storageConnectionsForStorageTypeMap.containsKey(storageType)) {
Line 135: storageConnectionsForStorageTypeMap.put(storageType,
new ArrayList<storage_server_connections>());
Line 130: Map<StorageType, List<storage_server_connections>>
storageConnectionsForStorageTypeMap =
Line 131: new HashMap<StorageType,
List<storage_server_connections>>();
Line 132: for (storage_server_connections lunConnections :
lun.getLunConnections()) {
Line 133: StorageType storageType =
lunConnections.getstorage_type();
Line 134: if
(!storageConnectionsForStorageTypeMap.containsKey(storageType)) {
why arraylist?
Line 135: storageConnectionsForStorageTypeMap.put(storageType,
new ArrayList<storage_server_connections>());
Line 136: }
Line 137:
storageConnectionsForStorageTypeMap.get(storageType).add(lunConnections);
Line 138: }
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/StorageHelperBaseTest.java
Line 26:
Line 27: @Test
Line 28: public void getLunConnectionsForISCSI() {
Line 29: LUNs lun = new LUNs();
Line 30: ArrayList<storage_server_connections> connections = new
ArrayList<storage_server_connections>();
can be exported to a method instead of repeat...
Line 31: connections.add(new storage_server_connections("Some LUN
connection",
Line 32: "id",
Line 33: "iqn",
Line 34: "password",
--
To view, visit http://gerrit.ovirt.org/10141
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ida526dd9e08bc9881e65a529cb1b00aee0786af1
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Liron Aravot <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
