Allon Mureinik has posted comments on this change.
Change subject: core: Add scan domain query
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(11 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ScanDomainQuery.java
Line 35:
Line 36: @Override
Line 37: protected void executeQueryCommand() {
Line 38: // first, run getImagesList query into vdsm to get all of the
images on the storage domain - then store in imagesList
Line 39: VDSBrokerFrontendImpl vdsBroker = new VDSBrokerFrontendImpl();
You should use getBackend().getResourceManager() - see GetLunsByVgIdQuery for a
usage example.
Line 40: VDSReturnValue imagesListResult =
vdsBroker.RunVdsCommand(VDSCommandType.GetImagesList, new
GetImagesListVDSCommandParameters(getParameters().getDomainId(),
DbFacade.getInstance()
Line 41: .getStoragePoolDAO()
Line 42: .getAllForStorageDomain(
Line 43:
getParameters().getDomainId()).get(0).getId()));
Line 40: VDSReturnValue imagesListResult =
vdsBroker.RunVdsCommand(VDSCommandType.GetImagesList, new
GetImagesListVDSCommandParameters(getParameters().getDomainId(),
DbFacade.getInstance()
Line 41: .getStoragePoolDAO()
Line 42: .getAllForStorageDomain(
Line 43:
getParameters().getDomainId()).get(0).getId()));
Line 44: ArrayList<Guid> imagesList = (ArrayList<Guid>)
imagesListResult.getReturnValue();
You can downcast to a List, you don't need an ArrayList.
Line 45: // diskImageList is a map of the IDs returned by the vdsm
query to actual Disk objects
Line 46: HashMap<Guid, Disk> diskImageList = new HashMap<Guid, Disk>();
Line 47: for (Guid id : imagesList) {
Line 48: diskImageList.put(id, getDbFacade().getDiskDao().get(id));
Line 42: .getAllForStorageDomain(
Line 43:
getParameters().getDomainId()).get(0).getId()));
Line 44: ArrayList<Guid> imagesList = (ArrayList<Guid>)
imagesListResult.getReturnValue();
Line 45: // diskImageList is a map of the IDs returned by the vdsm
query to actual Disk objects
Line 46: HashMap<Guid, Disk> diskImageList = new HashMap<Guid, Disk>();
define the variable as a Map, not a HashMap.
Also, if it's a map, the name diskImageList is kinda confusing :-)
Line 47: for (Guid id : imagesList) {
Line 48: diskImageList.put(id, getDbFacade().getDiskDao().get(id));
Line 49: }
Line 50: // retArr is the ArrayList which will be returned by this query
Line 45: // diskImageList is a map of the IDs returned by the vdsm
query to actual Disk objects
Line 46: HashMap<Guid, Disk> diskImageList = new HashMap<Guid, Disk>();
Line 47: for (Guid id : imagesList) {
Line 48: diskImageList.put(id, getDbFacade().getDiskDao().get(id));
Line 49: }
I think that instead of getting the disks one by one, you can use
DiskImageDAO.getAllSnapshotsForStorageDomain.
Line 50: // retArr is the ArrayList which will be returned by this query
Line 51: ArrayList<Guid> retArr = new ArrayList<Guid>();
Line 52: List<Disk> allDisks =
DbFacade.getInstance().getDiskDao().getAll();
Line 53: ArrayList<Disk> disksOnSD = new ArrayList<Disk>(); // disks on
the storage domain passed as a parameter
Line 47: for (Guid id : imagesList) {
Line 48: diskImageList.put(id, getDbFacade().getDiskDao().get(id));
Line 49: }
Line 50: // retArr is the ArrayList which will be returned by this query
Line 51: ArrayList<Guid> retArr = new ArrayList<Guid>();
You can define the variable as a List not an ArrayList
Line 52: List<Disk> allDisks =
DbFacade.getInstance().getDiskDao().getAll();
Line 53: ArrayList<Disk> disksOnSD = new ArrayList<Disk>(); // disks on
the storage domain passed as a parameter
Line 54: // filter disks in allDisks into disksOnSD if they are on this
domain
Line 55: for (Disk d : allDisks) {
Line 60: }
Line 61: }
Line 62: // then, compare the list of all images on the domain with the
list oVirt recognizes
Line 63: for (Map.Entry<Guid, Disk> listItem :
diskImageList.entrySet()) {
Line 64: if (!(disksOnSD.contains(listItem.getValue()))) {
If you're doing a bunch of contains operation, you'll be better off using Sets
instead of List.
Line 65: retArr.add(listItem.getKey());
Line 66: }
Line 67: }
Line 68: // log the difference [for testing purposes]
Line 72: }
Line 73: else {
Line 74: for (Guid li : retArr) {
Line 75: logger.info("Returning list element..."); //$NON-NLS-1$
Line 76: logger.info("UUID: " + li.toString()); //$NON-NLS-1$
I fear this will bloat up the log a bit. Consider the following:
logger.info ("Returning list: " + StringUtils.join (retArr, ",");
Line 77: }
Line 78: }
Line 79: logger.info("Done returning ScanDomainQuery list...");
Line 80: getQueryReturnValue().setReturnValue(retArr); // return
difference
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VdcQueryType.java
Line 85: GetAllFloppyImagesListByStoragePoolId(VdcQueryAuthType.User),
Line 86: GetAllDisksByVmId(VdcQueryAuthType.User),
Line 87: GetAllAttachableDisks(VdcQueryAuthType.User),
Line 88: GetAllDisks,
Line 89: ScanDomain,
Is this query strictly an admin query?
Line 90: GetImageByImageId,
Line 91: GetDiskByDiskId,
Line 92: GetImagesByStorageDomainAndTemplate,
Line 93:
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/GetImagesListVDSCommand.java
Line 17: @Override
Line 18: protected void ExecuteIrsBrokerCommand() {
Line 19: _result =
getIrsProxy().getImagesList(getParameters().getStorageDomainId().toString());
Line 20: ProceedProxyReturnValue();
Line 21: java.util.ArrayList<Guid> tempRetValue = new
java.util.ArrayList<Guid>(_result.mImageList.length);
don't use FCQNs.
Also, define the variable as a List, not an ArrayList.
Line 22: for (int i = 0; i < _result.mImageList.length; i++) {
Line 23: tempRetValue.add(new Guid(_result.mImageList[i]));
Line 24: }
Line 25: setReturnValue(tempRetValue);
Line 20: ProceedProxyReturnValue();
Line 21: java.util.ArrayList<Guid> tempRetValue = new
java.util.ArrayList<Guid>(_result.mImageList.length);
Line 22: for (int i = 0; i < _result.mImageList.length; i++) {
Line 23: tempRetValue.add(new Guid(_result.mImageList[i]));
Line 24: }
instead of all of this, you could just have
setReturnValue(Arrays.asList(_result));
Line 25: setReturnValue(tempRetValue);
Line 26: }
Line 27:
Line 28: @Override
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IIrsServer.java
Line 85: GetVmsListReturnForXmlRpc getVmsList(String storagePoolId, String
storageDomainId);
Line 86:
Line 87: StatusOnlyReturnForXmlRpc upgradeStoragePool(String storagePoolId,
String targetVersion);
Line 88:
Line 89: ImagesListReturnForXmlRpc getImagesList(String sdUUID);
I'm assuming this is backed up by a VDSM patch?
Is it merged already? If not, please add a link so we know not to merge the
engine patch until we have a VDSM version that supports it.
--
To view, visit http://gerrit.ovirt.org/8070
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5ada1a6a030c090e872b2eb3f67ee9325f379963
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ricky Hopper <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Eduardo <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Liron Aravot <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
