Vered Volansky has posted comments on this change. Change subject: core: Add storage space thresholds support ......................................................................
Patch Set 35: (7 comments) 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 Done 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 Done 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? There's a test that executes the command. There, if there's no user-set values for thresholds, configuration values are loaded. This was absent from previous patches since this was checked in REST before, but was moved here at yours and Juan's request. 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? There's a test that executes the command. There, if there's no user-set values for thresholds, configuration values are loaded. This was absent from previous patches since this was checked in REST before, but was moved here at yours and Juan's request. 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 a Done 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 commen OK, though I do think that as a reviewer your job is to question the code, hence you assume this is a bug, but as a reader you probably would have just understood it. 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? Probably some auto-shit. Will move back. 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
