Allon Mureinik has posted comments on this change.
Change subject: core: Add scan domain query (revised)
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(13 inline comments)
Some general comments/questions:
1. How is this query going to be used? Currently, this patch offers a backend
query that is not called by any user or automatic flow. Will this be introduced
in subsequent patches?
2. Can you add a link to the corresponding VDSM patch?
3. Please don't change change-ids between revisions - it makes it quite hard to
review like this. There's nothing wrong with having a couple of dozen revisions
for the same patch.
[also, see some implementation comments inline]
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ScanDomainQuery.java
Line 21: * @author Ricky Hopper
Line 22: *
Line 23: * @param <P>
Line 24: */
Line 25: public class ScanDomainQuery<P extends ScanDomainParameters> extends
QueriesCommandBase<P> {
You can use StorageDomainQueryParametersBase, no need for a new parameter
class, IMHO.
Line 26:
Line 27: private static final Log logger =
LogFactory.getLog(ScanDomainQuery.class);
Line 28:
Line 29: public ScanDomainQuery(P parameters) {
Line 36: VDSBrokerFrontend vdsBroker =
getBackend().getResourceManager();
Line 37: VDSReturnValue imagesListResult =
vdsBroker.RunVdsCommand(VDSCommandType.GetImagesList, new
GetImagesListVDSCommandParameters(getParameters().getDomainId(),
DbFacade.getInstance()
Line 38: .getStoragePoolDAO()
Line 39: .getAllForStorageDomain(
Line 40:
getParameters().getDomainId()).get(0).getId()));
why not just pass domainId and poolId in the parameter object?
Line 41: List<Guid> imagesList = (List<Guid>)
imagesListResult.getReturnValue();
Line 42: //fromDao is a list of all disk images on the domain from the
DAO
Line 43: List<DiskImage> fromDao =
getDbFacade().getDiskImageDao().getAllSnapshotsForStorageDomain(getParameters().getDomainId());
Line 44: // then, compare the list of all images on the domain with the
list oVirt recognizes
Line 37: VDSReturnValue imagesListResult =
vdsBroker.RunVdsCommand(VDSCommandType.GetImagesList, new
GetImagesListVDSCommandParameters(getParameters().getDomainId(),
DbFacade.getInstance()
Line 38: .getStoragePoolDAO()
Line 39: .getAllForStorageDomain(
Line 40:
getParameters().getDomainId()).get(0).getId()));
Line 41: List<Guid> imagesList = (List<Guid>)
imagesListResult.getReturnValue();
As Liron suggested in the previous incarnation of this patch, consider
returning a list of DiskImages, not just Guids (since you already loaded them
anyway...)
Line 42: //fromDao is a list of all disk images on the domain from the
DAO
Line 43: List<DiskImage> fromDao =
getDbFacade().getDiskImageDao().getAllSnapshotsForStorageDomain(getParameters().getDomainId());
Line 44: // then, compare the list of all images on the domain with the
list oVirt recognizes
Line 45: // if the ID in imagesList is recognized by oVirt, remove from
list
Line 43: List<DiskImage> fromDao =
getDbFacade().getDiskImageDao().getAllSnapshotsForStorageDomain(getParameters().getDomainId());
Line 44: // then, compare the list of all images on the domain with the
list oVirt recognizes
Line 45: // if the ID in imagesList is recognized by oVirt, remove from
list
Line 46: for (DiskImage image : fromDao) {
Line 47: if (imagesList.contains(image.getId())) {
both contains(Object) and remove(Object) need to iterate the list to find the
given object.
Since remove does not through an exception if it's given an object that is not
part of the list (it simply returns false), the contains() condition is
redundant - you can simply iterate and call remove, ignoring the return value.
Line 48: imagesList.remove(image.getId());
Line 49: }
Line 50: }
Line 51: // log the difference [for testing purposes]
Line 49: }
Line 50: }
Line 51: // log the difference [for testing purposes]
Line 52: if (imagesList.isEmpty()) {
Line 53: logger.info("List is empty."); //$NON-NLS-1$
This is backend code, which is not subject to I18N considerations.
You can remove the NON NLS commend.
Line 54: }
Line 55: else {
Line 56: logger.info("UUIDs found: " + StringUtils.join(imagesList,
",")); //$NON-NLS-1$
Line 57: }
Line 50: }
Line 51: // log the difference [for testing purposes]
Line 52: if (imagesList.isEmpty()) {
Line 53: logger.info("List is empty."); //$NON-NLS-1$
Line 54: }
I'd put the "}" and the "else {" on the same line, but that's really nitpicking.
Feel free to ignore it :-)
Line 55: else {
Line 56: logger.info("UUIDs found: " + StringUtils.join(imagesList,
",")); //$NON-NLS-1$
Line 57: }
Line 58: getQueryReturnValue().setReturnValue(imagesList); // return
difference
Line 52: if (imagesList.isEmpty()) {
Line 53: logger.info("List is empty."); //$NON-NLS-1$
Line 54: }
Line 55: else {
Line 56: logger.info("UUIDs found: " + StringUtils.join(imagesList,
",")); //$NON-NLS-1$
here too.
Line 57: }
Line 58: getQueryReturnValue().setReturnValue(imagesList); // return
difference
Line 59: }
Line 60:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/ScanDomainParameters.java
Line 1: package org.ovirt.engine.core.common.queries;
Line 2:
Line 3: import org.ovirt.engine.core.compat.Guid;
Line 4:
Line 5: public class ScanDomainParameters extends VdcQueryParametersBase {
See comment about the parameter class in the ScanDomainQuery class.
Line 6:
Line 7: private static final long serialVersionUID = -7975065688901687512L;
Line 8:
Line 9: public ScanDomainParameters(Guid sdUUID) {
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/GetImagesListVDSCommandParameters.java
Line 1: package org.ovirt.engine.core.common.vdscommands;
Line 2:
Line 3: import org.ovirt.engine.core.compat.Guid;
Line 4:
Line 5: public class GetImagesListVDSCommandParameters extends
IrsBaseVDSCommandParameters {
You can use StorageDomainIdParametersBase, no need for a new parameter class,
IMHO.
Line 6: public GetImagesListVDSCommandParameters(Guid sdUUID) {
Line 7: super();
Line 8: setStorageDomainId(sdUUID);
Line 9: }
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/GetImagesListVDSCommand.java
Line 20: protected void ExecuteIrsBrokerCommand() {
Line 21: _result =
getIrsProxy().getImagesList(getParameters().getStorageDomainId().toString());
Line 22: ProceedProxyReturnValue();
Line 23: ArrayList<Guid> tempRetValue = new
ArrayList<Guid>(_result.mImageList.length);
Line 24: for (int i = 0; i < _result.mImageList.length; i++) {
Use the foreeach syntax, it's be more readable
Line 25: tempRetValue.add(new Guid(_result.mImageList[i]));
Line 26: }
Line 27: setReturnValue(tempRetValue);
Line 28: }
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/ImagesListReturnForXmlRpc.java
Line 3: import java.util.Map;
Line 4:
Line 5: public final class ImagesListReturnForXmlRpc extends
StatusReturnForXmlRpc {
Line 6: private static final String IMAGES_LIST = "imageslist";
Line 7: // [XmlRpcMissingMapping(MappingAction.Ignore),
XmlRpcMember("isolist")]
I don't get this comment.
is it a leftover?
Line 8: public String[] mImageList;
Line 9:
Line 10: public ImagesListReturnForXmlRpc(Map<String, Object> innerMap) {
Line 11: super(innerMap);
Line 4:
Line 5: public final class ImagesListReturnForXmlRpc extends
StatusReturnForXmlRpc {
Line 6: private static final String IMAGES_LIST = "imageslist";
Line 7: // [XmlRpcMissingMapping(MappingAction.Ignore),
XmlRpcMember("isolist")]
Line 8: public String[] mImageList;
I'd add a getter for this, instead as keep it as public.
Line 9:
Line 10: public ImagesListReturnForXmlRpc(Map<String, Object> innerMap) {
Line 11: super(innerMap);
Line 12: Object[] tempObj = (Object[]) innerMap.get(IMAGES_LIST);
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IrsServerWrapper.java
Line 251: @Override
Line 252: public ImagesListReturnForXmlRpc getImagesList(String sdUUID) {
Line 253: Map<String, Object> xmlRpcReturnValue =
irsServer.getImagesList(sdUUID);
Line 254: ImagesListReturnForXmlRpc wrapper = new
ImagesListReturnForXmlRpc(xmlRpcReturnValue);
Line 255: return wrapper;
I'd inline the return
Line 256: }
--
To view, visit http://gerrit.ovirt.org/8219
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icce8e2cd12b9609ce82dbb7d255c08bdbc6dcc41
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
