Juan Hernandez has posted comments on this change. Change subject: core: Add storage space thresholds support ......................................................................
Patch Set 24: (3 comments) https://gerrit.ovirt.org/#/c/35277/24/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Line 2769: <xs:element name="wipe_after_delete" type="xs:boolean" minOccurs="0"/> Line 2770: <xs:element name="import" type="xs:boolean" minOccurs="0"/> Line 2771: <xs:element name="is_attached" type="xs:boolean" minOccurs="0"/> Line 2772: <xs:element name="low_space_threshold" type="xs:int" minOccurs="0"/> Line 2773: <xs:element name="critical_space_threshold" type="xs:int" minOccurs="0"/> What are the units for these? Can you add a comment explaining what is the unit? Is int (32 bits) enough for this? In other entities, like disks, we use "xs:long" for sizes. Line 2774: </xs:sequence> Line 2775: </xs:extension> Line 2776: </xs:complexContent> Line 2777: </xs:complexType> https://gerrit.ovirt.org/#/c/35277/24/backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml File backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml: Line 2302: optionalArguments This operation doesn't create a new storage domain, it just attaches an existing storage domain to the data center. Si I think that in this context the thresholds aren't valid parameters, not even optional. https://gerrit.ovirt.org/#/c/35277/24/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/StorageDomainMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/StorageDomainMapper.java: Line 63: } Line 64: if (model.isSetCriticalSpaceThreshold()) { Line 65: entity.setCriticalSpaceThreshold((model.getCriticalSpaceThreshold())); Line 66: } else if (entity.getCriticalSpaceThreshold() == -1) { Line 67: entity.setCriticalSpaceThreshold(Config.<Integer>getValue(ConfigValues.CriticalSpaceThreshold)); This logic deciding to assing default values should be in the backend, not in the RESTAPI. Line 68: } Line 69: return entity; Line 70: } Line 71: -- To view, visit https://gerrit.ovirt.org/35277 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I19621dfc770c69003d731a7593d037d7d4040a82 Gerrit-PatchSet: 24 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vered Volansky <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Idan Shaby <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: Vered Volansky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
