Community
Participate
Working Groups
Created attachment 171482 [details] Patch to show errorMsg argument Currently if the Proxy sends a Error event an error dialog is shown. But this error dialog doesn't show the error event errorMsg argument. The patch fixes this and uses it for the PBS proxy to send error message if the pbs commands can't be executed correctly.
Is it possible for the proxy to use an IProxyRuntimeStartupErrorEvent rather than modifying IRuntimeErrorStateEvent? This event already provides a message that is propagated to the UI, so I think this would be preferrable. The proxy could test for the existence of the commands on startup and generate this event if they fail. There are a number of problems with your patch, including the API changes and that the message is not optional so all resource managers would need to be updated. Thanks.
(In reply to comment #1) > Is it possible for the proxy to use an IProxyRuntimeStartupErrorEvent rather > than modifying IRuntimeErrorStateEvent? This event already provides a message > that is propagated to the UI, so I think this would be preferrable. The proxy > could test for the existence of the commands on startup and generate this event > if they fail. Yes for the case that the commands are missing it can be done at start-up and the patch is not required. But a missing command is only one of many possible sources of errors. Most other reasons (e.g. invalid xml, out of memory, ..) can't be avoided by checking them on start-up. Thus it would be very good if the proxy is able to send an error after the start-up. But I can check for the existence of the commands on start-up as a temporary solution. This sounds like a good idea. I'll work on that tonight. > There are a number of problems with your patch, including the API changes and > that the message is not optional so all resource managers would need to be > updated. I didn't review/test the patch much. So I wouldn't be surprised if it contains errors. I didn't want to spend time on it after I realized it can't go into 4.0 because it changes to much. The message is supposed to be optional (can be null) and I think it works with null. It also compiles with all RMs. So the other RM don't need to be changed. Is a API change possible for 4.0.1 if the PTP mailing-list agrees?
I think we need to improve the possibilities for the proxy to communicate errors to the client little bit more systematically. It should be possible to send: - Fatal error (shown as error - shut down proxy) - Error (shown as error - proxy stays running) - Warning shown as error - proxy stays running) The errors should be shown in a dialog. The warning in the "Error Log" view. I think for the RM it is very important to clearly communicate errors. It is quite likely that errors occur because executing external programs and parsing output is inherent risky. Thus a good way to report those errors is important.
(In reply to comment #2) > Is a API change possible for 4.0.1 if the PTP mailing-list agrees? An API change is not possible for a bugfix release. Is an API change necessary in this case though? We already have a "message" protocol type that provides error level, code, and string (see MessageAttributes), and is implemented all the way to AbstructRuntimeResourceManager. However, there is no implementation above this. It would just be necessary to decide on how to handle each message level as per comment #3 and provide the implementation.
(In reply to comment #4) > (In reply to comment #2) > > Is a API change possible for 4.0.1 if the PTP mailing-list agrees? > > An API change is not possible for a bugfix release. > > Is an API change necessary in this case though? We already have a "message" > protocol type that provides error level, code, and string (see > MessageAttributes), and is implemented all the way to > AbstructRuntimeResourceManager. However, there is no implementation above this. > It would just be necessary to decide on how to handle each message level as per > comment #3 and provide the implementation. I don't see how using the MessageAttributes class would help to avoid an API change. The problem is that IRuntimeErrorStateEvent doesn't has a method to get the attributes. Either as String as implemented in the patch or as MessageAttributes.
Rather than sending a proxy ERROR_STATE event, you could send a proxy MESSAGE event with type ERROR and the appropriate message. These are available as attributes from the IRuntimeMessageEvent. The proxy ERROR_STATE event (which results in an IRuntimeErrorStateEvent) is meant for situations where the runtime system has detected an unrecoverable error and should be shut down. If that's the situation in this case, then you could also issue one of these in addition to the message.
I looked at the error message handling in PBSProxyRuntimeServer. It seems to me that the eclipse framework allows to poll the user with dialogs and effectively lock him out by a combination of Error Dialog Modality and the poll cycle length of 2 seconds. A major problem (or not) is that the ErrorEvent will cause the GUI to generate an Error Dialog. Then, if an ErrorEvent is used for anything which is NOT fatal, the possibility exists, that the same error will occur in the next loop cycle of the main event handler of the proxy and cause any ErrorDialog to be spawned repeatedly. Even if the GUI could be improved to block any ErrorDialogs if another one is already open, the question arises whether the maximum time of 2 seconds is enough to allow the user any interaction with the GUI. My favorite solution would be to declare a policy regarding error reporting (with ideas of ,https://bugs.eclipse.org/bugs/show_bug.cgi?id=316225, comment 3.) If an error is FATAL: generate an error event and shut down the server If an error is NOT fatal: generate a message event with type error If an error is NOT fatal and low priority: generate a message event with type warning AbstractProxyServer defines the state "SUSPENDED". This has not been implemented and I don't know how well this mode is supported on the GUI site. But in the long run, a "suspended" mode might be used for errors which are not fatal, but require the user to take action. I'll go through the error handling in the PBS Proxy tonight and will ensure the above mentioned error reporting policy. For the future this issue has to be aware to anyone writing proxy code ...
Created attachment 174895 [details] Replaces non-fatal Errors (ErrorEvents) by ProxyRuntimeMessageEvent
I'm not sure that a message event is displayed anywhere. We need to implement the above policy. FATAL messages could generate an ErrorStateEvent. Other messages need to be converted to multistatus and displayed in an error dialog (to handle multiple messages) or something.
(In reply to comment #9) > I'm not sure that a message event is displayed anywhere. We need to implement > the above policy. FATAL messages could generate an ErrorStateEvent. Other > messages need to be converted to multistatus and displayed in an error dialog > (to handle multiple messages) or something. Do you suggest to extend the GUI by a list view for messages of type ProxyRuntimeMessageEvent? I.e. the GUI would accumulate any messages in that list?
(In reply to comment #10) > (In reply to comment #9) > > I'm not sure that a message event is displayed anywhere. We need to implement > > the above policy. FATAL messages could generate an ErrorStateEvent. Other > > messages need to be converted to multistatus and displayed in an error dialog > > (to handle multiple messages) or something. > > Do you suggest to extend the GUI by a list view for messages of type > ProxyRuntimeMessageEvent? I.e. the GUI would accumulate any messages in that > list? I think a message view would be very useful.
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > I'm not sure that a message event is displayed anywhere. We need to implement > > > the above policy. FATAL messages could generate an ErrorStateEvent. Other > > > messages need to be converted to multistatus and displayed in an error dialog > > > (to handle multiple messages) or something. > > > > Do you suggest to extend the GUI by a list view for messages of type > > ProxyRuntimeMessageEvent? I.e. the GUI would accumulate any messages in that > > list? > > I think a message view would be very useful. I don't see why we we need any new GUI view. The standard Eclipse error reporting does exactly what we need as far as I can see. By StatusManager.getManager().handle(new Status(IStatus.ERROR, CurrentPlugin.PLUGIN_ID, "Error Message"), StatusManager.SHOW); we can show a dialog which lists one or many error messages (it automatically switches to the multiple error view as the 2nd message is handled) and with LOG instead of SHOW we can display warning in the ErrorLog view. I don't think it is a good idea to show errors only in the Error Log view because it is too easy to overlook an error there. See also: http://wiki.eclipse.org/Platform_UI_Error_Handling Also I think that a RuntimeSubmitJobErrorEvent instead of a RuntimeMessageEvent should be used in terminateJob.
Created attachment 174935 [details] Patch to show Messages This patch shows the messages from the proxy. Is this how we want to implement it? In AbstractRuntimeResourceManager everything but handleEvent(IRuntimeMessageEvent e) is just clean-up of the code and can be ignored. In PBSProxyRuntimeServer a test error message is added to simulate the case that for each update a error is shown. Also the terminateJob error message is corrected (but not tested)
(In reply to comment #13) > Created an attachment (id=174935) [details] > Patch to show Messages > > This patch shows the messages from the proxy. Is this how we want to implement > it? > > In AbstractRuntimeResourceManager everything but > handleEvent(IRuntimeMessageEvent e) is just clean-up of the code and can be > ignored. > > In PBSProxyRuntimeServer a test error message is added to simulate the case > that for each update a error is shown. Also the terminateJob error message is > corrected (but not tested) This approach is an improvement in that it reduces the amount of windows popping up when an error repeatedly occurs. However, it doesn't resolve the more severe problem of the GUI being blocked due to the small polling cycle length of 2 seconds. A dedicated list view for messages would resolve this issue. Critical errors could be indicated by alarming colors instead of an error dialog.
(In reply to comment #12) > I don't see why we we need any new GUI view. The standard Eclipse error > reporting does exactly what we need as far as I can see. By > StatusManager.getManager().handle(new Status(IStatus.ERROR, > CurrentPlugin.PLUGIN_ID, "Error Message"), StatusManager.SHOW); > we can show a dialog which lists one or many error messages (it automatically > switches to the multiple error view as the 2nd message is handled) and with LOG > instead of SHOW we can display warning in the ErrorLog view. I think the view would be useful as it would keep a record of errors that have occurred, much like the Error Log view, but I don't have a problem doing both: displaying the error in a dialog, and logging it to the Error Log view. Providing our own view would allow us to customize how the messages are displayed, such as using a colors for the different levels, but I don't want to create additional work if it's not really necessary. > > I don't think it is a good idea to show errors only in the Error Log view > because it is too easy to overlook an error there. > > See also: http://wiki.eclipse.org/Platform_UI_Error_Handling > > Also I think that a RuntimeSubmitJobErrorEvent instead of a RuntimeMessageEvent > should be used in terminateJob. Definitely not. A RuntimeSubmitJobErrorEvent should only be sent if a job *submission* fails. It has nothing to do with jobs once they have been submitted.
(In reply to comment #14) > This approach is an improvement in that it reduces the amount of windows > popping up when an error repeatedly occurs. However, it doesn't resolve the > more severe problem of the GUI being blocked due to the small polling cycle > length of 2 seconds. I'm not sure what you mean. The GUI is not blocked. The dialog is not modal. The user can e.g. shutdown the proxy while the dialog is open. Or do you mean being blocked in some other way? > A dedicated list view for messages would resolve this issue. Critical errors > could be indicated by alarming colors instead of an error dialog. The error log view does provide different colors for errors (red) or warning (yellow). The problem is the view might not be visible (closed, hidden tab or minimized). (In reply to comment #15) > I think the view would be useful as it would keep a record of errors that have > occurred, much like the Error Log view, but I don't have a problem doing both: > displaying the error in a dialog, and logging it to the Error Log view. > Providing our own view would allow us to customize how the messages are > displayed, such as using a colors for the different levels, but I don't want to > create additional work if it's not really necessary. So far I don't see the advantage of a the view. The customized might also not be visible and I'm not sure we need more flexibility than the Error Log view. > > Also I think that a RuntimeSubmitJobErrorEvent instead of a RuntimeMessageEvent > > should be used in terminateJob. > > Definitely not. A RuntimeSubmitJobErrorEvent should only be sent if a job > *submission* fails. It has nothing to do with jobs once they have been > submitted. Sorry. Yes I copy-pasted the wrong event. My patch uses RuntimeTerminateJobErrorEvent.
(In reply to comment #16) > (In reply to comment #14) > > This approach is an improvement in that it reduces the amount of windows > > popping up when an error repeatedly occurs. However, it doesn't resolve the > > more severe problem of the GUI being blocked due to the small polling cycle > > length of 2 seconds. > I'm not sure what you mean. The GUI is not blocked. The dialog is not modal. > The user can e.g. shutdown the proxy while the dialog is open. Or do you mean > being blocked in some other way? I stand corrected! It's does not block - It's just annoying since it keeps popping up. But that's a feature not a bug, since an error is supposed to be annoying , right? Hence, this approach might solve this issue! Good Job.
committed to HEAD on Jul 24th.
