Vered Volansky 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 low_space_threshold is percentage, so 0..100. critical_space_threshold is in GB, set initially by us to 5. We don't expect users to touch this, and if so then probably to 0. In any case there's no meaningfull use case for big numbers. Int should be fine for both. 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 2298: body: Line 2299: parameterType: StorageDomain Line 2300: signatures: Line 2301: - mandatoryArguments: {storagedomain.id|name: 'xs:string'} Line 2302: optionalArguments: {storagedomain.storage.low_space_threshold: 'xs:int', storagedomain.storage.critical_space_threshold: 'xs:int'} > This operation doesn't create a new storage domain, it just attaches an exi Ack, will remove. Line 2303: description: add a new storage domain to the data center Line 2304: urlparams: {} Line 2305: headers: Line 2306: Content-Type: {value: application/xml|json, required: true} 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 This was done having seen some other code like this, but this is not the first review saying it should go to the backend, so to the backend it goes. 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 <vvola...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Idan Shaby <ish...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
