Vered Volansky has posted comments on this change. Change subject: core: Add storage space thresholds support ......................................................................
Patch Set 28: (4 comments) Replied to new comments. All other comments were already replied-to in patchset 23, and addressed, even if not yet submitted. The latest patchset was half-work submitted due to Ori's request. In any case, I did reply to these comments the first time they were entered, please see my answers there. https://gerrit.ovirt.org/#/c/35277/28/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)); > Same formatting comment Well, if this is removed from here, like both you and Juan suggested, there's not format issue left. Line 68: } Line 69: return entity; Line 70: } Line 71: https://gerrit.ovirt.org/#/c/35277/28/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.getLowSpaceThreshold()) { Line 436: type = AuditLogType.IRS_DISK_SPACE_LOW; Line 437: } Line 438: if (freeDiskInGB < domainFromDb.getCriticalSpaceThreshold()) { //This one takes in case both thresholds are met > This comment does not seem right - you can skip the first if and only hit t Yes, but the comment refers to what happens when both conditions are met. It should focus the reader on the fact that even if the two thresholds are met, only the second message gets in, which is indeed the desired behaviour. In other words, someone not familiar with this code might think there's a bug here, and the comment states this is the desired behaviour, rather than a bug. Line 439: type = AuditLogType.IRS_DISK_SPACE_LOW_ERROR; Line 440: } Line 441: } Line 442: https://gerrit.ovirt.org/#/c/35277/28/packaging/dbscripts/upgrade/03_06_1181_add_thresholds_to_storage_domain_static.sql File packaging/dbscripts/upgrade/03_06_1181_add_thresholds_to_storage_domain_static.sql: Line 1: select fn_db_add_column('storage_domain_static', 'low_space_threshold', 'INTEGER NOT NULL DEFAULT 0'); Line 2: select fn_db_add_column('storage_domain_static', 'critical_space_threshold', 'INTEGER NOT NULL DEFAULT 0'); Line 3: Line 4: UPDATE storage_domain_Static > please have "Static" with a lower-case s Done Line 5: SET low_space_threshold=cast(( Line 6: SELECT option_value Line 7: FROM vdc_options Line 8: WHERE option_name='LowSpaceThreshold' AND version='general') AS INTEGER), Line 5: SET low_space_threshold=cast(( Line 6: SELECT option_value Line 7: FROM vdc_options Line 8: WHERE option_name='LowSpaceThreshold' AND version='general') AS INTEGER), Line 9: critical_space_threshold=cast(( > There's a redundant space here before "critical" Done Line 10: SELECT option_value Line 11: FROM vdc_options -- 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: 28 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
