[GE dev] Review for CR 6675570

pollinger harald.pollinger at sun.com
Tue Jun 2 12:14:43 BST 2009


Hi Michael,

in this case, I think it's not necessary to create a new status - I'm 
not sure what the side effects are, so it would be to risky for this 
small fix.

Please use the STATUS_EDENIED2HOST for the second error, too. If I 
understand this error right, it's also a "Access denied" error.

Regards,
Harald

mpospisil wrote:
> Hello Harald,
> I have had a look at the changes that you suggested and the 
> notify_about_kill() function returns two errors. The second error is 
> that comproc with id progname_id (a variable to the function) not known 
> on host (another variable to the function). The problem with this is 
> that the valid values for AN_status defined in sge_answer.h does not 
> include anything about such an error.
> Do you think it is necessary to create a new entry in the enumeration 
> for this error, or can I just use the unknown error value STATUS_EUNKNOWN?
> 
> Michael
> 
> mpospisil wrote:
> 
>> Hello Harald,
>> thanks for looking at my changes, see my comments below.
>>
>> pollinger wrote:
>>
>>  
>>
>>> Hi Michael,
>>>
>>> so you just moved the "answer_list_add()" into the if and the else 
>>> block. In the "if" block, which is called in case the 
>>> host_notify_about_kill() function returns an error, you changed the 
>>> status to "STATUS_EDENIED2HOST". That's all, right?
>>>
>>>
>>> There are three things:
>>>
>>> 1. In the if statement, please write to what you are comparing. This 
>>> would be "if (.... != 0)" in this case. But:
>>>
>>>
>>>    
>>>
>> OK
>>
>>  
>>
>>> 2. The host_notify_about_kill() function returns two different error 
>>> values. Shouldn't both be forwarded to the answer list? I don't know if 
>>> there are two different states defined that would be suitable to reflect 
>>> the return values of nost_notify_about_kill(), but if there are, it 
>>> would be a good idea to use them.
>>>
>>>
>>>    
>>>
>> I did not know that host_notify_about_kill() is returning two different 
>> errors. I will have a look if there are different states that could 
>> reflect the error message received from host_notify_about_kill() 
>> function. This sounds like a good idea.
>>
>>  
>>
>>> 3. Shouldn't the ANSWER_QUALITY in the if block be ANSWER_QUALITY_ERROR?
>>> What effect does the ANSWER_QUALITY have?
>>>
>>>
>>>    
>>>
>> Of course it should be ANSWER_QUALITY_ERROR, this is a problem of 
>> copying the other line and just changing the status.
>>
>> I will try to make these modifications and resend my updates.
>>
>> Michael
>>
>>  
>>
>>> Regards,
>>> Harald
>>>
>>>
>>>
>>>
>>>
>>> Index: sge_host_qmaster.c
>>> ===================================================================
>>> RCS file: /cvs/gridengine/source/daemons/qmaster/sge_host_qmaster.c,v
>>> retrieving revision 1.127
>>> diff -u -p -r1.127 sge_host_qmaster.c
>>> --- sge_host_qmaster.c	19 May 2009 11:32:06 -0000	1.127
>>> +++ sge_host_qmaster.c	21 May 2009 13:57:07 -0000
>>> @@ -1128,12 +1128,13 @@ notify(sge_gdi_ctx_class_t *ctx, lListEl
>>>       if (host_notify_about_kill(ctx, lel, kill_jobs)) {
>>>          INFO((SGE_EVENT, MSG_COM_NONOTIFICATION_SSS, action_str,
>>>                (execd_alive ? "" : MSG_OBJ_UNKNOWN), hostname));
>>> +         answer_list_add(&(task->answer_list), SGE_EVENT, 
>>> STATUS_EDENIED2HOST, ANSWER_QUALITY_INFO);
>>>       } else {
>>>          INFO((SGE_EVENT, MSG_COM_NOTIFICATION_SSS, action_str,
>>>                (execd_alive ? "" : MSG_OBJ_UNKNOWN), hostname));
>>> +         answer_list_add(&(task->answer_list), SGE_EVENT, STATUS_OK, 
>>> ANSWER_QUALITY_INFO);
>>>       }
>>> -      DPRINTF((SGE_EVENT));
>>> -      answer_list_add(&(task->answer_list), SGE_EVENT, STATUS_OK, 
>>> ANSWER_QUALITY_INFO);
>>> +      DPRINTF((SGE_EVENT));
>>>    }
>>>
>>>    if (kill_jobs) {
>>>
>>> ------------------------------------------------------
>>> http://gridengine.sunsource.net/ds/viewMessage.do?dsForumId=39&dsMessageId=198252
>>>
>>> To unsubscribe from this discussion, e-mail: [dev-unsubscribe at gridengine.sunsource.net].
>>>
>>>
>>>    
>>>
>> ------------------------------------------------------
>> http://gridengine.sunsource.net/ds/viewMessage.do?dsForumId=39&dsMessageId=198285
>>
>> To unsubscribe from this discussion, e-mail: [dev-unsubscribe at gridengine.sunsource.net].
>>  
>>
> 
> ------------------------------------------------------
> http://gridengine.sunsource.net/ds/viewMessage.do?dsForumId=39&dsMessageId=200182
> 
> To unsubscribe from this discussion, e-mail: [dev-unsubscribe at gridengine.sunsource.net].

------------------------------------------------------
http://gridengine.sunsource.net/ds/viewMessage.do?dsForumId=39&dsMessageId=200344

To unsubscribe from this discussion, e-mail: [dev-unsubscribe at gridengine.sunsource.net].



More information about the gridengine-users mailing list