Allon Mureinik has posted comments on this change. Change subject: core: Add storage space thresholds support ......................................................................
Patch Set 35: Code-Review-1 (7 comments) Minor issues, mainly in tests - see inline. https://gerrit.ovirt.org/#/c/35277/35/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java: Line 258: @Reloadable Line 259: @TypeConverterAttribute(Integer.class) Line 260: @DefaultValueAttribute("10") Line 261: //This value is in percents Line 262: WarningLowSpaceIndicator, There is a redundant indentation at the beginning of this line Line 263: @Reloadable Line 264: @TypeConverterAttribute(Integer.class) Line 265: @DefaultValueAttribute("5") Line 266: //This value is in GB Line 263: @Reloadable Line 264: @TypeConverterAttribute(Integer.class) Line 265: @DefaultValueAttribute("5") Line 266: //This value is in GB Line 267: CriticalSpaceActionBlocker, There is a redundant indentation at the beginning of this line Line 268: @TypeConverterAttribute(String.class) Line 269: @DefaultValueAttribute("") Line 270: MacPoolRanges, Line 271: @TypeConverterAttribute(Boolean.class) https://gerrit.ovirt.org/#/c/35277/35/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainResourceTest.java File backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainResourceTest.java: Line 35: Line 36: @Rule Line 37: public MockConfigRule mcr = new MockConfigRule( Line 38: mockConfig(ConfigValues.CriticalSpaceActionBlocker, CRITICAL_SPACE_THRESHOLD), Line 39: mockConfig(ConfigValues.WarningLowSpaceIndicator, LOW_SPACE_THRESHOLD) Why do we need this? Line 40: ); Line 41: Line 42: public BackendStorageDomainResourceTest() { Line 43: super(new BackendStorageDomainResource(GUIDS[0].toString(), new BackendStorageDomainsResource())); https://gerrit.ovirt.org/#/c/35277/35/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainsResourceTest.java File backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainsResourceTest.java: Line 64: Line 65: @Rule Line 66: public MockConfigRule mcr = new MockConfigRule( Line 67: mockConfig(ConfigValues.CriticalSpaceActionBlocker, CRITICAL_SPACE_THRESHOLD), Line 68: mockConfig(ConfigValues.WarningLowSpaceIndicator, LOW_SPACE_THRESHOLD) Why do we need this? Line 69: ); Line 70: Line 71: protected static final org.ovirt.engine.core.common.businessentities.StorageDomainType TYPES_MAPPED[] = { Line 72: org.ovirt.engine.core.common.businessentities.StorageDomainType.Data, https://gerrit.ovirt.org/#/c/35277/35/backend/manager/modules/restapi/types/src/test/java/org/ovirt/engine/api/restapi/types/StorageDomainMapperTest.java File backend/manager/modules/restapi/types/src/test/java/org/ovirt/engine/api/restapi/types/StorageDomainMapperTest.java: Line 26: Line 27: @Rule Line 28: public MockConfigRule mcr = new MockConfigRule( Line 29: mockConfig(ConfigValues.CriticalSpaceActionBlocker, CRITICAL_SPACE_THRESHOLD), Line 30: mockConfig(ConfigValues.WarningLowSpaceIndicator, LOW_SPACE_THRESHOLD) The ConfigValue access was removed from StorageDomainMapper, hence should also be removed from the test. Line 31: ); Line 32: Line 33: public StorageDomainMapperTest() { Line 34: super(StorageDomain.class, StorageDomainStatic.class, org.ovirt.engine.core.common.businessentities.StorageDomain.class); https://gerrit.ovirt.org/#/c/35277/35/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IrsProxyData.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IrsProxyData.java: Line 434: if (freeDiskInGB != null) { Line 435: if (freePercent < domainFromDb.getWarningLowSpaceIndicator()) { Line 436: type = AuditLogType.IRS_DISK_SPACE_LOW; Line 437: } Line 438: if (freeDiskInGB < domainFromDb.getCriticalSpaceActionBlocker()) { //This one takes in case both thresholds are met Continuing the conversation from revision #28 - I initially read the comment and indeed thought this was a bug, as per the comment. Perhaps the comment could be improved to say something like "if both conditions are met, only IRS_DISK_SPACE_LOW_ERROR will be shown"? Line 439: type = AuditLogType.IRS_DISK_SPACE_LOW_ERROR; Line 440: } Line 441: } Line 442: https://gerrit.ovirt.org/#/c/35277/35/packaging/dbscripts/inst_sp.sql File packaging/dbscripts/inst_sp.sql: Line 19: Line 20: Create or replace FUNCTION inst_add_iso_storage_domain(v_storage_domain_id UUID, v_name VARCHAR(250), v_connection_id uuid, v_connection VARCHAR(250),v_available int, v_used int) Line 21: RETURNS VOID Line 22: AS $procedure$ Line 23: DECLARE Why move this? Line 24: BEGIN Line 25: if not exists (select 1 from storage_server_connections where connection = v_connection) then Line 26: -- Insert storage server connection info Line 27: perform Insertstorage_server_connections(v_connection,cast(v_connection_id as varchar(250)),NULL,NULL,NULL,NULL,1,NULL,NULL,NULL,NULL,NULL,NULL); -- 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: 35 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
