Moti Asayag has posted comments on this change.

Change subject: core: [RFE]Backup Awareness - DB
......................................................................


Patch Set 1:

(2 comments)

https://gerrit.ovirt.org/#/c/39523/1/packaging/dbscripts/engine_backup_history_sp.sql
File packaging/dbscripts/engine_backup_history_sp.sql:

Line 36: RETURNS VOID
Line 37:    AS $procedure$
Line 38: BEGIN
Line 39:     IF v_status = -1 THEN
Line 40:        PERFORM InsertEngineBackupHistory(v_db_name, v_done_at, false, 
v_output_message);
> I don't see any formatting issue here , please elaborate
i guess we see it differently in gerrit due to the tabs width configuration. On 
my setup the tab width is set to 4, So line 140 which starts with "PERFORM..." 
is aligned to the first char of line 39:

  IF v_status... THEN
  PERFORM Insert...
      
    INSERT INTO...
    VALUES...

I'd expect the PERFORM... to start from the same offset as the INSERT INTO...


  IF v_status... THEN
    PERFORM Insert...
      
    INSERT INTO...
    VALUES...
Line 41: 
Line 42:         INSERT INTO audit_log(log_time, log_type_name, log_type, 
severity, message)
Line 43:         VALUES(v_done_at, ENGINE_BACKUP_FAILED, 9026, 9, 
v_output_message);
Line 44: 


Line 39:     IF v_status = -1 THEN
Line 40:        PERFORM InsertEngineBackupHistory(v_db_name, v_done_at, false, 
v_output_message);
Line 41: 
Line 42:         INSERT INTO audit_log(log_time, log_type_name, log_type, 
severity, message)
Line 43:         VALUES(v_done_at, ENGINE_BACKUP_FAILED, 9026, 9, 
v_output_message);
> I think this is really straight forward, those are the codes (log_type) and
I meant that when i reviewed the patches, I'd expect to see the entities, 
constants and so own by their appearance.

As part of the review I should check that 9026 is the suitable type for this 
kind of event, but in order to figure out what 9026 stands for, i need to look 
for it in other patches of this topic (which is less convenient to the 
reviewer).

I'd rather see the AuditLogType.java appears before this patch or as part of 
this patch, so when I review this patch, I know exactly where I should look for 
the mentioned codes.
Line 44: 
Line 45:     ELSIF v_status = 0 THEN
Line 46:         INSERT INTO audit_log(log_time, log_type_name, log_type, 
severity, message)
Line 47:         VALUES(v_done_at, ENGINE_BACKUP_STARTED, 9024, 1, 
v_output_message);


-- 
To view, visit https://gerrit.ovirt.org/39523
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7824eb8a720bd654e94dada987aee19b9624a516
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Eli Mesika <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Martin PeÅ™ina <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
Gerrit-Reviewer: Oved Ourfali <[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

Reply via email to