Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 316671 - PBS: Read model definition from XML and send to client
Summary: PBS: Read model definition from XML and send to client
Status: CLOSED WONTFIX
Alias: None
Product: PTP
Classification: Tools
Component: RM.PBS (show other bugs)
Version: 4.0   Edit
Hardware: PC Windows 7
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: Roland Schulz CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-11 23:27 EDT by Roland Schulz CLA
Modified: 2011-12-21 01:06 EST (History)
4 users (show)

See Also:


Attachments
Implements reading attribute definitions from file and communication to the client during the discovery phase (52.06 KB, patch)
2010-07-31 07:34 EDT, Benjamin Lindner CLA
no flags Details | Diff
Reading of Attribute Definitions and Parser/PBS/Protocol Mappings from text files (203.75 KB, patch)
2010-08-05 23:43 EDT, Benjamin Lindner CLA
no flags Details | Diff
Reading of Attribute Definitions and Parser/PBS/Protocol Mappings from text files (217.05 KB, patch)
2010-08-07 13:42 EDT, Benjamin Lindner CLA
no flags Details | Diff
Reading of Attribute Definitions and Parser/PBS/Protocol Mappings from text files (216.98 KB, patch)
2010-08-16 05:17 EDT, Benjamin Lindner CLA
no flags Details | Diff
Reading of Attribute Definitions and Parser/PBS/Protocol Mappings from text files (217.83 KB, patch)
2010-09-03 18:51 EDT, Benjamin Lindner CLA
no flags Details | Diff
UI modifications to read in attribute defs from XML (126.85 KB, patch)
2010-09-15 17:00 EDT, Albert L. Rossi CLA
no flags Details | Diff
UI modifications to read in attribute defs from XML (2) (137.43 KB, patch)
2010-09-16 12:16 EDT, Albert L. Rossi CLA
no flags Details | Diff
PBS: Read model definition from XML and send to client (217.97 KB, patch)
2010-10-01 21:08 EDT, Benjamin Lindner CLA
g.watson: iplog+
Details | Diff
merged proxy and UI patch (392.18 KB, patch)
2010-10-04 09:21 EDT, Albert L. Rossi CLA
no flags Details | Diff
merged proxy and UI patch (402.63 KB, patch)
2010-10-04 11:20 EDT, Albert L. Rossi CLA
no flags Details | Diff
merged proxy and UI patch (402.63 KB, patch)
2010-10-08 12:03 EDT, Albert L. Rossi CLA
no flags Details | Diff
merged proxy and UI patch with progress bar fix for bug 310193 (421.24 KB, patch)
2010-10-08 15:16 EDT, Albert L. Rossi CLA
no flags Details | Diff
merged proxy and UI patch with progress bar fix for bug 310193 plus combo refresh (413.35 KB, patch)
2010-10-11 11:26 EDT, Albert L. Rossi CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roland Schulz CLA 2010-06-11 23:27:57 EDT
Currently the model definition (including the attribute definitions) is coded into the Java code of the Proxy and Client. It would be better to store it in an XML file and send it to the client during the ModelDef Proxy Protocol phase.
Comment 1 Roland Schulz CLA 2010-07-10 01:15:54 EDT
Ben,
what is the status on this?
Comment 2 Benjamin Lindner CLA 2010-07-10 13:26:23 EDT
(In reply to comment #1)
> Ben,
> what is the status on this?

Currently the model definition is read from a custom formatted text file and sent to the Client. No improvements have been made w/ respect to correctness and xml formatting. Also, it is not clear how the GUI handles the data.
Comment 3 Roland Schulz CLA 2010-07-11 00:01:31 EDT
Could you attach a patch against CVS to this bug? 

I was assuming that the the property name you send in the Model Def Phase is shown as the name in the properties view. 

This events are handled in ModelManager. If you want to check that they are received correctly.

BTW: I noticed that their are no attribute definitions on the client side anymore. Al did you remove them? I'm surprised that it even works without.
Comment 4 Albert L. Rossi CLA 2010-07-11 07:54:55 EDT
(In reply to comment #4).

Roland, could you be more specific? I'm not sure what you mean.  The client uses the definitions in pbs.core.
Comment 5 Albert L. Rossi CLA 2010-07-11 08:16:30 EDT
Continued from last comment.

Roland, if you mean the reception of the attributes sent by the proxy, this is what I was talking about earlier.  We had decided not to implement this for the first version; the attribute definitions have always been taken from a hard-coded list (map) on the client side.  The dynamic population of the map is what needs to be added.
Comment 6 Roland Schulz CLA 2010-07-11 14:21:05 EDT
(In reply to comment #5)
> Continued from last comment.
> 
> Roland, if you mean the reception of the attributes sent by the proxy, this is
> what I was talking about earlier.  We had decided not to implement this for the
> first version; the attribute definitions have always been taken from a
> hard-coded list (map) on the client side.  The dynamic population of the map is
> what needs to be added.

What I meant was: I couldn't find these hard-coded attribute definitions anymore. The class PBSNodeAttributes and PBSQueueAttributes is still there but it isn't used anymore. PBSJobAttributes is still used but only for the script template code.
Comment 7 Albert L. Rossi CLA 2010-07-11 17:36:11 EDT
OK.  At some point way at the beginning of our coding effort we decided that there shouldn't be multiple definitions of shared hard-coded names, so if there were definitions in the UI plugin, I may have moved them to core.  But the code in the UI plugin has never made use of anything except the PBSJobAttributes.

If other parts of the UI use the other definitions, the refactoring of the code may have taken care of that by pointing them to core (?).
Comment 8 Albert L. Rossi CLA 2010-07-11 17:38:01 EDT
Re: the last comment.  I guess not (i.e., the refactoring taking care of it), since you say the other categories are not used.  Were they ever?
Comment 9 Benjamin Lindner CLA 2010-07-31 07:34:20 EDT
Created attachment 175647 [details]
Implements reading attribute definitions from file and communication to the client during the discovery phase

new features: 
- introduced intermediate class ProxyRuntimeServer between AbstractProxyRuntimeServer and PBSProxyRuntimeServer, implements: checkEnvironment() and detectAttributes()
- during DISCOVERY: check the environment for pbs command line tools and read attribute definitons from custom formatted text file and communicates this to the client via MODEL_DEF events.

remarks:
- as of now, the file pbs_attribute_definitions.txt has to be copied manually to the remote site. This should be automated.
- currently the patch doesn't change the functionality of the code, but implements the required changes for future customization.

status:
The model attribute definitions are hard-coded in package org.eclipse.ptp.core.elements.attributes; currently the pbsproxyruntimeserver has its own version of a hard-coded pbs attribute defintions list which is compatible with the model definitions in the ptp core. For this, the pbs attribute definition classes have a mapping feature. 

I intent to adjust the pbs proxy server class "controller" to work with a separate copy of custom Attribute definitions (as read from file) and an additional Mapping class (will also be read from file), which links PBS Attribute Definitions to target Model Definitions. Currently, this functionality is hardcoded into the PBS Attribute Definitions classes and the mapping is not very explicit/clear.
Splitting this into two separate files will enhance the understanding of the process and add flexibility, since the Remote PBS Proxy then decides on the Attribute Definitions it provides AND how they link to Model Definitions of the resource manager.
Comment 10 Roland Schulz CLA 2010-07-31 14:58:06 EDT
Please follow the developer guidelines (e.g activate API Tooling, externalize String) before you submit patches. See: http://wiki.eclipse.org/PTP/developer_guidelines

It is not safe to call Process.exec without reading stdout and stderr (even if you just discard it). If the buffer overruns it dead-locks.

What is the checkEnviroment procedure good for? The parse calls in initServer already check that all commands are executable and the output can be parsed. Calling "which" checks less. Not sure whether this is helpful (not saying it is not - just don't see what this adds).

(In reply to comment #9)
> remarks:
> - as of now, the file pbs_attribute_definitions.txt has to be copied manually
> to the remote site. This should be automated.
This file should be part of the pbs_proxy.jar. Why did you not use XML for this file?
Comment 11 Benjamin Lindner CLA 2010-08-01 06:55:35 EDT
(In reply to comment #10)
> Please follow the developer guidelines (e.g activate API Tooling, externalize
> String) before you submit patches. See:
> http://wiki.eclipse.org/PTP/developer_guidelines
Sure. 

> It is not safe to call Process.exec without reading stdout and stderr (even if
> you just discard it). If the buffer overruns it dead-locks.
Good to know. 

> What is the checkEnviroment procedure good for? The parse calls in initServer
> already check that all commands are executable and the output can be parsed.
> Calling "which" checks less. Not sure whether this is helpful (not saying it is
> not - just don't see what this adds).
I implemented the checkEnvironment a while ago. At that time the initServer didn't have a parse() check (AFAIK). Hence the idea of checkEnvironment() might be obsolete by now. I'll take it out, since initServer() is taking care of it.

Currently, I am not clear on who should take ownership of the detected PBS Attributes. In principle, this could be completely moved to the controller, since I don't see it being used at any other instance. However, then the controller should be queried by the proxyserver during the discovery phase, in order to communicate the detected attributes to the client side. But the implementation of runStateMachine is in abstractproxyruntimeserver() which doesn't know anything about controllers.

Either controllers should be implemented an abstraction level higher, or the abstractproxyruntimeserver should have a pointer to the list of attributes it is supposed to communicate. That means that the attributes have to live in the data space of (pbs)proxyruntimeserver. 

> (In reply to comment #9)
> > remarks:
> > - as of now, the file pbs_attribute_definitions.txt has to be copied manually
> > to the remote site. This should be automated.
> This file should be part of the pbs_proxy.jar. Why did you not use XML for this
> file?
Text based is just an intermediate solution, b/c it allows to edit any data more efficiently. XML will be added later, its just a matter of swapping parsers.

How do I add the data files to pbs_proxy.jar?
Comment 12 Roland Schulz CLA 2010-08-01 12:45:26 EDT
(In reply to comment #11)
> Currently, I am not clear on who should take ownership of the detected PBS
> Attributes. In principle, this could be completely moved to the controller,
> since I don't see it being used at any other instance. However, then the
> controller should be queried by the proxyserver during the discovery phase, in
> order to communicate the detected attributes to the client side. But the
> implementation of runStateMachine is in abstractproxyruntimeserver() which
> doesn't know anything about controllers.
> 
> Either controllers should be implemented an abstraction level higher, or the
> abstractproxyruntimeserver should have a pointer to the list of attributes it
> is supposed to communicate. That means that the attributes have to live in the
> data space of (pbs)proxyruntimeserver. 

I would add a function to controller which returns the list of attributes in a ready to send fashion, the same way as parse does. I would then add an abstract method to abstractproxyruntimeserver which sends the attributes. It is implemented in pbsproxyruntimeserver and thus has access to the controller instances. This would make it most similar to how it is handled so far for the events coming from parse. But feel free to do it different if you think it can be done better.

> > (In reply to comment #9)
> > > - as of now, the file pbs_attribute_definitions.txt has to be copied manually
> > > to the remote site. This should be automated.
> > This file should be part of the pbs_proxy.jar. Why did you not use XML for this
> > file?
> 
> How do I add the data files to pbs_proxy.jar?
It has to be added to build.xml.
Comment 13 Dieter Krachtus CLA 2010-08-01 14:44:52 EDT
(In reply to comment #12)

> can you help with how to add a text file to the jar. See last comment of
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=316671
>
> Thanks
> Roland


This is pretty straight forward by adjusting the ANT jar target (e.g. <fileset file="about.html"/>). 

I edited the build.xml to show you how to do it. Notice that the file 'about.html' is added to pbs_proxy.jar
Comment 14 Benjamin Lindner CLA 2010-08-05 23:43:58 EDT
Created attachment 175998 [details]
Reading of Attribute Definitions and Parser/PBS/Protocol Mappings from text files 

Thanks. I included any files into the package pbs_proxy.jar. They are now synched everytime there is a change.

This patch now contains functional changes. Any hard-coded PBS Attribute Definitions are abandoned. The definitions and all necessary mappings (keys and values) are now in text files. I spend some initial effort on getting some of the mapping right, but more refinement is needed. Especially mapping all parser outputs to some pbs attribute definition.
The mapping is now very explicit. There are two stages:
1) mapping of parsed command line tool output to unique PBS Attributes. 
2) mapping of unique PBS Attributes to a corresponding meaning within the PTP Protocol (found in PTP core attributes)

Previously this mapping was hard-coded and intermingled. The current approach w/ two stages should make things more clear for future extensions. 

The code in the current state might break some other code, like job submission. For this, testing is needed and adjustments should be made.

Still missing is the implementation of XML parsers and the definition of the attributes in XML. But I'd give that a lower priority than getting the mapping right in the first place. Especially the completion of the set of required and possible PBS Attributes needs consideration.
Comment 15 Benjamin Lindner CLA 2010-08-07 13:42:27 EDT
Created attachment 176093 [details]
Reading of Attribute Definitions and Parser/PBS/Protocol Mappings from text files 

I enhanced the mapping to tread the IDs as regular expressions w/ case_ignorant. This enables advances mapping features w/ if...else... logic and allows to catch case case changes , e.g. job_owner -> Job_Owner

I also added copyright statement changes to any file which was edited. 

The patch should be ready for a submission to the head.
Comment 16 Benjamin Lindner CLA 2010-08-16 05:17:49 EDT
Created attachment 176659 [details]
Reading of Attribute Definitions and Parser/PBS/Protocol Mappings from text files

fixed build path problem

the pbs wizard is currenlty broken, since it requires to read the attribute definitions from the file. But the wizard is executed on the client side. Hence, we'd need a consistent way of locating the definitions file on the client side. 

job termination works. 
job submission is not tested yet. 

I recommend to test the current functionality, fix the wizard and submit the patch to the head. 
Then we can open another bug, w/ the XML parsing feature request.
Comment 17 Roland Schulz CLA 2010-08-16 22:23:16 EDT
Greg, Al,

I think I remember we said (a few months ago) the best solution for the PBS attributes is:
- They are store in a file
- Read only by the proxy
- Send in the Model-Def. phase

I think we meant this also for the launch. Because it has been a while and I haven't written down anything (sorry!), I'm not sure anymore.

Thus I have the following questions:
- Should only those attributes used in the communication (jobs, nodes, ...) be send in the ModelDef phase (and thus read by the proxy) or also the launch attributes only used by the client? 
- If so, what is the reason?
- If so, do we need to extend the ModelDef phase for the launch attributes or can we send those as normal ModelDef attributes?

I hope one of you remembers what we discussed back then so that we don't have to discuss it again.

Roland
Comment 18 Greg Watson CLA 2010-08-17 10:03:56 EDT
(In reply to comment #17)
> Greg, Al,
> 
> I think I remember we said (a few months ago) the best solution for the PBS
> attributes is:
> - They are store in a file
> - Read only by the proxy
> - Send in the Model-Def. phase
> 
> I think we meant this also for the launch. Because it has been a while and I
> haven't written down anything (sorry!), I'm not sure anymore.
> 
> Thus I have the following questions:
> - Should only those attributes used in the communication (jobs, nodes, ...) be
> send in the ModelDef phase (and thus read by the proxy) or also the launch
> attributes only used by the client? 
> - If so, what is the reason?
> - If so, do we need to extend the ModelDef phase for the launch attributes or
> can we send those as normal ModelDef attributes?
> 
> I hope one of you remembers what we discussed back then so that we don't have
> to discuss it again.
> 
> Roland

I think the reason for generating attributes on the server side was to allow server configuration changes to be easily propagated to the client without requiring plugin updates, etc. The down-side to this is that the client will only know the attributes when the proxy is running, which would mean that it's not possible to create a launch configuration without starting the proxy. To overcome this, you'll need to cache the attributes on the client somehow. Another problem is how you present the attributes to the user. If the client doesn't have any knowledge of the attributes, then you're probably restricted to only displaying them in a table in the launch configuration. However, there may be some pre-defined attributes that you want to display in a particular manner that is more meaningful to the user, so this would need to be built into the launch configuration.

There is no distinction between attributes for the model and attributes for the launch, so both can be sent during the model def phase. However it sounds like you may need to distinguish between them so that you can populate the launch configuration with only the launch attributes. If this is the case, then there are two options: split the model def phase into a model attribute phase followed by a launch attribute phase; or add a flag to the attribute definition indicating what it is used for (there is already a "visible" flag, but you'd probably need to add another one).
Comment 19 Albert L. Rossi CLA 2010-08-17 11:18:29 EDT
It appears that we will then need to rethink the entire interface, since it was posited on  the notion of "configurability".  If there is no pre-knowledge of standard attributes, then what is there to configure?

It seems to me that we need to distinguish between the "universal" PBS attributes (standard, found everywhere), and possibly customized ones.  The latter certainly can be dropped into some table widget.

However, if this doesn't seem right, then we do need another discussion.

I hope to be on the call for 08/19, but we may want to keep this discussion aside for another venue.
Comment 20 Roland Schulz CLA 2010-08-18 00:05:13 EDT
I don't think for PBS it is possible to discover what launch configurations are available on a specific server. Thus the current implementation where the user can configure for a specific RM what launch configurations should be displayed.

I think one of the reasons we discussed before, was that we want to make it easy to extend the current Java Proxy to other RMs which do allow to discover the available attributes. But I'm not sure whether this is sufficient of a reason if it makes since more difficult for the GUI.
Comment 21 Benjamin Lindner CLA 2010-09-03 18:51:56 EDT
Created attachment 178204 [details]
Reading of Attribute Definitions and Parser/PBS/Protocol Mappings from text files

update for the most recent version of head

for large cluster the regular expression mechanism currently poses a performance barrier. This can cause large delays between updates. I will work on fixing this.
Comment 22 Albert L. Rossi CLA 2010-09-10 16:30:09 EDT
Roland, Benjamin,

We failed to discuss how the client is going to know about the mapping of an API attribute name to the batch script flag.  I think this needs to be included in the attribute definition event somehow.

What I'm talking about is:

Account_Name

becomes

-A

Currently the fact that the template holds this information is not a problem because it knows both ahead of time.  That doesn't really make sense any more.
Comment 23 Benjamin Lindner CLA 2010-09-10 17:56:38 EDT
So what you are suggesting is to define a new mapping. This time from an attribute in the PBSXXX_XXX landscape (definitions.txt) to a valid entry in the batch script.

Then a simple file would do it:

PBSKEY ID :: Substitution text

The user then only has to supply a list of entries like:

PBSKEY ID :: VALUE

The Substitution text would have to be scanned for MARKERS which are then replaced by the anticipated value.

e.g. walltime:

PBSKEY ID = PBSJOB_RESOURCE_LIST__WALLTIME
Value = 2:00:00

then the entry in the mapping file would be:
PBSKEY ID :: #PBS -l walltime=MARKER

then the code would do a strsubst. on MARKER and replace it with the value.

A different problem is where that mapping should be initially located. Like with the definitions.txt. With the definitions a good solution would be to do the initial connection during setting up the Resource Manager connection. I don't see how the above mapping should be incorporated into PBS definitions. After all, it's a mapping. 

Ideally a PBS attribute would know about how it's supposed to be represented in different contexts. Currently a PBS attribute only knows about how it's represented in one context (the UI), or at least its attribute fields are targeted towards this. It doesn't know how it would look like when in Parser representation or when in Model representation (even the IDs change when switching the context). 
This knowledge currently resides with the individual parsers or with the controller on the proxy side, which have to translate the representation by means of the various mapping files. 

I don't see a short term solution which effectively extents the protocol discovery phase to allow for fancy attributes. We'd need to redesign the meaning of an attribute or introduce something like a generic attribute, which we then can use to transport additional information (e.g. mapping instructions between different contexts) from the proxy to the client.


(In reply to comment #22)
> Roland, Benjamin,
> 
> We failed to discuss how the client is going to know about the mapping of an
> API attribute name to the batch script flag.  I think this needs to be included
> in the attribute definition event somehow.
> 
> What I'm talking about is:
> 
> Account_Name
> 
> becomes
> 
> -A
> 
> Currently the fact that the template holds this information is not a problem
> because it knows both ahead of time.  That doesn't really make sense any more.
Comment 24 Greg Watson CLA 2010-09-10 18:25:04 EDT
The original comment for this bug was the desire to store the (static) model definition in XML and send to the client during the model definition phase of the protocol. This has also been discussion somewhat in bug 309343. I think we can assume that we're going to move in this direction as soon as Randy finishes the new model design. Since this will be XML, there will be nothing to stop us adding RM-specific elements to the XML schema, which could include information such as the mappings described in the previous comment, other information you needed to build the UI, or anything else. These elements would be interpreted by the particular RM type and ignored by everyone else.

e.g.

<rm type="PBS">
  <map attrId="Account_Name" flag="-A">
  ...
</rm>

Since it will still be some time before this will be completed, I would suggest the following interim steps:

1. Define the XML schema that provides the elements/attributes you require
2. Have the proxy generate the XML file on the server side during it's startup.
3. Modify the server launcher so that it retrieves and caches the file once the proxy is started.
4. Parse the file and generate the appropriate information.

You could even include a static version of the XML in the plugin in the interim to save work (although 2 & 4 will probably still be required at some point.) Once the model & protocol changes have been implemented, you would only need to remove step 3 and use the information provided during the model definition phase.
Comment 25 Albert L. Rossi CLA 2010-09-13 09:12:39 EDT
In response to Benjamin and Greg,

I guess my original comment needed more clarification.  The issue is not how to achieve the mapping.  The template on the client side currently has lines in it like:

#PBS -A @Account_Name@
#PBS -c @Checkpoint@
#PBS -C @directive@
#PBS -e @Error_Path@

etc.

In essence, the template, which is used to construct the UI widgets, is its own "mapping" file.  But any mapping file on the client side has the same problem: it needs to know about all the possible PBS job attributes ahead of time; that sort of renders the dynamic nature of the attribute definition moot.  I realize we continue to go around in circles here, but to my thinking, if the design is to provide the client with attribute definitions, then the client should also be provided with the mapping it needs to construct a batch script to send back to the proxy.  So the question was precisely where to store that information, which I believe should be somehow associated with the proxy.

Greg's temporary solution is acceptable, but would mean we would have to push forward with making the proxy use XML and we would need to come up with a schema now.  I don't know how much work that will take.

What if we used Greg's idea of having the proxy send over an additional bit of info in the form of a special "attribute"?  That is, define an attribute "PBS_FLAGS", with its contents a series of name-value pairs, "Account_Name=-A", etc.  The proxy can get these from a regular file for now.  The client can use that attribute to construct the template from the other job attributes.
Comment 26 Greg Watson CLA 2010-09-13 09:27:27 EDT
(In reply to comment #25)

> Greg's temporary solution is acceptable, but would mean we would have to push
> forward with making the proxy use XML and we would need to come up with a
> schema now.  I don't know how much work that will take.
> 
> What if we used Greg's idea of having the proxy send over an additional bit of
> info in the form of a special "attribute"?  That is, define an attribute
> "PBS_FLAGS", with its contents a series of name-value pairs, "Account_Name=-A",
> etc.  The proxy can get these from a regular file for now.  The client can use
> that attribute to construct the template from the other job attributes.

I would suggest avoiding any extra work that will just need to be discarded in the future, and "special attributes" seem to be in this category. Why not just have the client read a local XML file with this information for now? The next step could be to have the proxy generate the XML, and the final step would be to send the XML as part of the model definition phase.

You might get the impression by now that I'm pushing the XML approach, which is true. I'd like to get the XML schema sorted out sooner rather than later as I think we're going to need to implement it soon anyway. I'd prefer to spend resources on that rather than on other options that will just be replaced by it int he future anyway.
Comment 27 Albert L. Rossi CLA 2010-09-13 09:56:39 EDT
(In reply to comment #26)

Yes, the XML schema is ultimately the way to go.  

At this point, then, the client doesn't even need to worry about the model definition (or indeed anything sent over by the proxy).  We just need to abstract out the class that does the work of converting attribute definitions into the PBS template file so the interface doesn't change when we switch to using the model definition, which is at any rate a good design decision.  

Agreed.
Comment 28 Albert L. Rossi CLA 2010-09-15 17:00:47 EDT
Created attachment 178983 [details]
UI modifications to read in attribute defs from XML

Apply 178204 first.
Comment 29 Albert L. Rossi CLA 2010-09-15 17:09:36 EDT
This patch actually does a number of things.

1.  It has now made the non-nls string messages consistently those displayed by the UI and thus subject to translation; all internal string constants have been pulled together into an interface.

2.  Templates are now saved and fetched from the Resource Manager Configuration, rather than stored directly on disk.

3.  Attribute data, which is a superset of attribute defintions, is abstracted out into an interface with currently one implementation as XML.  The object serializes and deserializes, but without strict validation as this is done currently in the absence of an XSD (which I assume we will eventually agree on).

4.  The UI makes use of a "converter" which takes this Attribute data and parses it into defintions, flags and tooltips; the converter also generates a base template (which contains all the valid attributes mapped to their appropriate qsub flags).  The XML implementation of the converter uses a static plugin resource, all_job_attributes.xml, to build this data structure.  The batch template manager is hardcoded to use this class, but when we switch to deriving this data from the model definition, the new implementation will be swapped in.

TODO:

a) Fix the <tooltip> text to use CDATA.
b) Add some missing copyright headers.

I have not yet decided exactly how the RM connection test will be done, but that is the next step.
Comment 30 Albert L. Rossi CLA 2010-09-16 12:16:53 EDT
Created attachment 179043 [details]
UI modifications to read in attribute defs from XML (2)

Replaces 178983.

Tool-tip display regularized.  Copyright headers updated.  

Moved some code from Launch Tab to Wizard for proper encapsulation.

Changed behavior of base template naming (each resource manager config has the base template named the same way; this is to avoid renaming, because the RM wizard page where the name is given to the RM is not available at the point where the base template needs to be created; but since templates now map clearly to RM configurations, this should not be a problem).

The next patch will provide the connection test.
Comment 31 Albert L. Rossi CLA 2010-09-16 14:16:01 EDT
I'm working on a prototype for using the IAttributeDefinition events during model definition, but I wanted to ask one thing:  is the client guaranteed to have received all such definition events prior to the receipt of the IResourceManagerChangeEvent which says "STARTED"?  Otherwise, how do I know when the definition phase is complete?

Also, it would be nice if the AttributeDefinitionManager allowed one to get all the definitions, without specifically asking for them by name.  I realize you are protecting this class from being written to or corrupted, but we could always return the keySet or the valueSet as an array.  Are there objections to my adding such a method?  Otherwise, I need to replicate the map in the PBSResourceManager.
Comment 32 Albert L. Rossi CLA 2010-09-16 15:15:59 EDT
A few more considerations.  After fooling with this for a little bit, I think that, apart the handiness of having a connection test, this is really not the correct logic for being able to create the templates from model defs.  This is because the reconnection needs to be done in other places than in wizard (unless we fully prohibit a launch/run configuration from being displayed if the connection is down).  So, I think for the moment, rather than trying to add the generalized test connection function, I will simply try to find the various points where the templates need to be displayed, and make them all do a check to see what the state of the RM is, and if it is down, start it, then access its AttributeDefinitionManager.
Comment 33 Greg Watson CLA 2010-09-16 17:42:14 EDT
(In reply to comment #31)
> I'm working on a prototype for using the IAttributeDefinition events during
> model definition, but I wanted to ask one thing:  is the client guaranteed to
> have received all such definition events prior to the receipt of the
> IResourceManagerChangeEvent which says "STARTED"?  Otherwise, how do I know
> when the definition phase is complete?
> 
> Also, it would be nice if the AttributeDefinitionManager allowed one to get all
> the definitions, without specifically asking for them by name.  I realize you
> are protecting this class from being written to or corrupted, but we could
> always return the keySet or the valueSet as an array.  Are there objections to
> my adding such a method?  Otherwise, I need to replicate the map in the
> PBSResourceManager.

Hmm. Well no model events are sent from the proxy until the model definition phase has finished and the client has issued a start events command. So, yes, you could use this. However, perhaps it would be better to provide a listener that is called back when the state machine changes state?
Comment 34 Greg Watson CLA 2010-09-16 17:45:33 EDT
(In reply to comment #32)
> A few more considerations.  After fooling with this for a little bit, I think
> that, apart the handiness of having a connection test, this is really not the
> correct logic for being able to create the templates from model defs.  This is
> because the reconnection needs to be done in other places than in wizard
> (unless we fully prohibit a launch/run configuration from being displayed if
> the connection is down).  So, I think for the moment, rather than trying to add
> the generalized test connection function, I will simply try to find the various
> points where the templates need to be displayed, and make them all do a check
> to see what the state of the RM is, and if it is down, start it, then access
> its AttributeDefinitionManager.

I don't think automatically starting the RM is a good idea. The user may have shut it down for some reason, and may not want it to start if they, for example, accidentally click on a launch configuration. At a minimum there should be a dialog that asks the user if they actually want to start it, but then you still need to deal with the situation if they say no.
Comment 35 Roland Schulz CLA 2010-09-16 19:27:18 EDT
I think Dave mentioned that on the last call that it would be more user friendly if it is done automatically and the user does not have to click some additional button. I agree. And if we just start the RM temporary (thus start and immediate stop it again) I don't see why this could be a problem for the user in the case you describe.
Comment 36 Albert L. Rossi CLA 2010-09-17 09:02:28 EDT
Well, my concern is not whether in the "Edit" wizard page for the RM we provide an option button or do it automatically; my concern is whether or not to update the template every time it gets exposed.  If we do that, then I need to restart the (potentially disconnected) RM even from the Launch Tab view.

And, it seems I am actually running into problems restarting the RM.  I must be doing something wrong, but when I start/shutdown once, the second time I call start, I do not get the "STARTED" event and the listener makes the whole thing hang.

BTW, I forgot to include a change to one of the interfaces in org.eclipse.ptp.rm.pbs.core in the above patches (sorry).  I'll post a new one soon, as I work out exactly what to do about this connection issue.
Comment 37 Greg Watson CLA 2010-09-17 10:32:24 EDT
(In reply to comment #35)
> I think Dave mentioned that on the last call that it would be more user
> friendly if it is done automatically and the user does not have to click some
> additional button. I agree. And if we just start the RM temporary (thus start
> and immediate stop it again) I don't see why this could be a problem for the
> user in the case you describe.

In my view it's not a good idea to do things that the user doesn't expect. If the host is unavailable, for example, then user is going to get a dialog that the connection failed. Also, starting the PBS RM takes some time, so the UI will be blocked while this happening. The launch configuration already does this kind of thing in a couple of places, and I'm in the process of taking the code out.

I think a better idea would be to obtain and cache the information when the RM is started. If the RM is stopped when the user opens the launch configuration, the cached information could be used, perhaps with a message indicating this fact.
Comment 38 Albert L. Rossi CLA 2010-09-17 10:38:12 EDT
(In reply to comment #37)
> (In reply to comment #35)
> > I think Dave mentioned that on the last call that it would be more user
> > friendly if it is done automatically and the user does not have to click some
> > additional button. I agree. And if we just start the RM temporary (thus start
> > and immediate stop it again) I don't see why this could be a problem for the
> > user in the case you describe.
> 
> In my view it's not a good idea to do things that the user doesn't expect. If
> the host is unavailable, for example, then user is going to get a dialog that
> the connection failed. Also, starting the PBS RM takes some time, so the UI
> will be blocked while this happening. The launch configuration already does
> this kind of thing in a couple of places, and I'm in the process of taking the
> code out.
> 
> I think a better idea would be to obtain and cache the information when the RM
> is started. If the RM is stopped when the user opens the launch configuration,
> the cached information could be used, perhaps with a message indicating this
> fact.
Yes, that's the direction I was going to go in, which is now feasible since the association of the template with the RM configuration is clean.  The only issue is the first time you define a Resource Manager.  If you don't connect, you won't see anything by way of template and consequently the Launch Tab will be blank.

On the other front, I think the reason the restart was not working is due to that nasty progress bar thing (there is another bug for this ... I don't think we ever fully resolved it).  If after connecting/stopping, I also stop the hung progress bar, and then restart, everything seems to be fine.  So this hung progress bar definitely needs to be fixed.
Comment 39 Greg Watson CLA 2010-09-17 10:45:32 EDT
(In reply to comment #38)
> Yes, that's the direction I was going to go in, which is now feasible since the
> association of the template with the RM configuration is clean.  The only issue
> is the first time you define a Resource Manager.  If you don't connect, you
> won't see anything by way of template and consequently the Launch Tab will be
> blank.

In this situation, you could display a message saying "please start the resource manager to load the configuration information" or some such, rather than a blank page.
 
> 
> On the other front, I think the reason the restart was not working is due to
> that nasty progress bar thing (there is another bug for this ... I don't think
> we ever fully resolved it).  If after connecting/stopping, I also stop the hung
> progress bar, and then restart, everything seems to be fine.  So this hung
> progress bar definitely needs to be fixed.

I'll take a look.
Comment 40 Albert L. Rossi CLA 2010-09-17 11:14:34 EDT
(In reply to comment #39)
> (In reply to comment #38)
> > Yes, that's the direction I was going to go in, which is now feasible since the
> > association of the template with the RM configuration is clean.  The only issue
> > is the first time you define a Resource Manager.  If you don't connect, you
> > won't see anything by way of template and consequently the Launch Tab will be
> > blank.
> 
> In this situation, you could display a message saying "please start the
> resource manager to load the configuration information" or some such, rather
> than a blank page.

Yes.

> 
> > 
> > On the other front, I think the reason the restart was not working is due to
> > that nasty progress bar thing (there is another bug for this ... I don't think
> > we ever fully resolved it).  If after connecting/stopping, I also stop the hung
> > progress bar, and then restart, everything seems to be fine.  So this hung
> > progress bar definitely needs to be fixed.
> 
> I'll take a look.

The culprit is line 385 in org.eclipse.ptp.remote.launch.core.AbstractRemoteServerRunner:

while (!fRemoteProcess.isCompleted() && !subMon.isCanceled()) {


it seems that the process.exitValue never returns without illegal thread state exceptions being thrown.

I understand why you do this ... if you just do waitFor, that blocks, and the user has no option of cancelling the monitor.

But do we want to allow the user to cancel the progress bar here?  Doing so should imply process.destroy ...

If so, a secondary thread probably needs to be set up.

This needs a different bug report.
Comment 41 Albert L. Rossi CLA 2010-09-17 12:12:53 EDT
OK, tracing it further down.

Neither exitValue nor waitFor ever seems to return (true) on the RemoteProcess, which I assume is the SSHRemoteProcess.

In order for this to happen, though, the composed KillableExecution needs to return wasFinished() as true, and for that to happen, the ExecutionObserver needs to have !isRunning on the composed exec to be true; this depends on the channel being closed.  But that won't happen until notifyFinish is called, which won't happen unless !isRunning is true ...

I may have missed something here, but there seems to be a closed loop here preventing this from ever reporting an exited process.  

????
Comment 42 Albert L. Rossi CLA 2010-09-17 12:28:24 EDT
(In reply to comment #41)
> OK, tracing it further down.
> 
> Neither exitValue nor waitFor ever seems to return (true) on the RemoteProcess,
> which I assume is the SSHRemoteProcess.
> 
> In order for this to happen, though, the composed KillableExecution needs to
> return wasFinished() as true, and for that to happen, the ExecutionObserver
> needs to have !isRunning on the composed exec to be true; this depends on the
> channel being closed.  But that won't happen until notifyFinish is called,
> which won't happen unless !isRunning is true ...
> 
> I may have missed something here, but there seems to be a closed loop here
> preventing this from ever reporting an exited process.  
> 
> ????

No, I guess the root cause is the the JSch channel is never closed.
Comment 43 Albert L. Rossi CLA 2010-09-17 12:31:41 EDT
(In reply to comment #42)
> (In reply to comment #41)
> > OK, tracing it further down.
> > 
> > Neither exitValue nor waitFor ever seems to return (true) on the RemoteProcess,
> > which I assume is the SSHRemoteProcess.
> > 
> > In order for this to happen, though, the composed KillableExecution needs to
> > return wasFinished() as true, and for that to happen, the ExecutionObserver
> > needs to have !isRunning on the composed exec to be true; this depends on the
> > channel being closed.  But that won't happen until notifyFinish is called,
> > which won't happen unless !isRunning is true ...
> > 
> > I may have missed something here, but there seems to be a closed loop here
> > preventing this from ever reporting an exited process.  
> > 
> > ????
> 
> No, I guess the root cause is the the JSch channel is never closed.

That is, is there some other route to closing the channel I've missed?  If not, then my original statement is correct ... channel is released only under condition of the channel having already been closed.

Not sure how to fix this yet.
Comment 44 Albert L. Rossi CLA 2010-09-17 13:41:49 EDT
Sorry to keep reporting these issues on this bug ...

The run method in ExecutionObserver never exits, even when I cancel the progress bar.  So something is broken in the state checking for the remote process.

Let me see if I can't find that other bug and add these last comments to it ...
Comment 45 Albert L. Rossi CLA 2010-09-23 12:35:40 EDT
After thinking some more about how the PBS UI works, it now seems to me that the template configuration should not be a wizard page in the ResourceManager setup, but should be an option available from the Launch Tab.  That way every time the user wants to add or modify custom templates, she doesn't have to shut down the RM.  This will also simplify the creation of the base template.  If the user has not started the resource manager, a warning will be issued.  Otherwise, the base template is simply regenerated from the current model definition attributes.  This way, the template is sure to be up to date.
Comment 46 Roland Schulz CLA 2010-09-23 13:50:51 EDT
(In reply to comment #45)
> After thinking some more about how the PBS UI works, it now seems to me that
> the template configuration should not be a wizard page in the ResourceManager
> setup, but should be an option available from the Launch Tab.  That way every
> time the user wants to add or modify custom templates, she doesn't have to shut
> down the RM.  This will also simplify the creation of the base template.  If
> the user has not started the resource manager, a warning will be issued. 
> Otherwise, the base template is simply regenerated from the current model
> definition attributes.  This way, the template is sure to be up to date.

No objections. Only think that the advantage is small thus it should only be done if it is not too complicated to change.
Comment 47 Greg Watson CLA 2010-09-23 14:02:29 EDT
(In reply to comment #45)
> After thinking some more about how the PBS UI works, it now seems to me that
> the template configuration should not be a wizard page in the ResourceManager
> setup, but should be an option available from the Launch Tab.  That way every
> time the user wants to add or modify custom templates, she doesn't have to shut
> down the RM.  This will also simplify the creation of the base template.  If
> the user has not started the resource manager, a warning will be issued. 
> Otherwise, the base template is simply regenerated from the current model
> definition attributes.  This way, the template is sure to be up to date.

Sounds like a good approach.
Comment 48 Albert L. Rossi CLA 2010-09-29 19:06:03 EDT
Roland, Ben

I am getting ready to post the patch with the latest version of the UI, but I've got a couple of little things I need to take care of.  One has to do with getting the queues from a running proxy.  This seemed to be working until today.  It is very probable I did something to change this behavior, but before I started messing with that part of the code, I wanted to make sure that no updates to HEAD have been made recently that would affect this.

Thanks, Al
Comment 49 Roland Schulz CLA 2010-09-29 20:05:41 EDT
(In reply to comment #48)
> Roland, Ben
> 
> I am getting ready to post the patch with the latest version of the UI, but
> I've got a couple of little things I need to take care of.  One has to do with
> getting the queues from a running proxy.  This seemed to be working until
> today.  It is very probable I did something to change this behavior, but before
> I started messing with that part of the code, I wanted to make sure that no
> updates to HEAD have been made recently that would affect this.
> 
> Thanks, Al

no we haven't committed anything which could effect this.
Comment 50 Greg Watson CLA 2010-09-29 22:37:23 EDT
me either
Comment 51 Albert L. Rossi CLA 2010-09-29 22:49:17 EDT
(In reply to comment #50)
> me either

Greg,

could this be related to the proxy slowness Roland noted a couple of weeks ago?  If I wait long enough, then refresh, the queues are there.  I takes almost 3 minutes for the queue lists to populate.  The machines seem to display immediately.

Al
Comment 52 Greg Watson CLA 2010-09-29 23:01:56 EDT
That bug was fixed some time ago. If you've updated from head recently you shouldn't have that problem.
Comment 53 Roland Schulz CLA 2010-09-29 23:09:27 EDT
(In reply to comment #52)
> That bug was fixed some time ago. If you've updated from head recently you
> shouldn't have that problem.

There were two performance problems introduced in the same week. As Greg mentioned he has fixed the one in the protocol some time ago.

@Ben: Have you fixed the performance problem in your new proxy mapping code? If so does the latest patch already include the version with the fixed performance?
Comment 54 Benjamin Lindner CLA 2010-09-30 05:23:32 EDT
(In reply to comment #53)
> (In reply to comment #52)
> > That bug was fixed some time ago. If you've updated from head recently you
> > shouldn't have that problem.
> 
> There were two performance problems introduced in the same week. As Greg
> mentioned he has fixed the one in the protocol some time ago.
> 
> @Ben: Have you fixed the performance problem in your new proxy mapping code? If
> so does the latest patch already include the version with the fixed
> performance?

I haven't looked at it throughout the last 1-2 weeks. At that time the performance of the proxy mapping still seemed to be a problem w/ machines with large number of nodes. For testing a cluster < 1000 nodes should be used. 
My initial guess that the regular expression mechanisms might be a problem should be ruled out, since I introduced a "memory" into the mapping, which bypasses the evaluation after the first lookup and just does a plain map-based retrieval.

I'll take a look at other potential bottlenecks till saturday. Then I hopefully have an answer.
Comment 55 Benjamin Lindner CLA 2010-10-01 21:04:15 EDT
(In reply to comment #54)
> (In reply to comment #53)
> > (In reply to comment #52)
> > > That bug was fixed some time ago. If you've updated from head recently you
> > > shouldn't have that problem.
> > 
> > There were two performance problems introduced in the same week. As Greg
> > mentioned he has fixed the one in the protocol some time ago.
> > 
> > @Ben: Have you fixed the performance problem in your new proxy mapping code? If
> > so does the latest patch already include the version with the fixed
> > performance?
> 
> I haven't looked at it throughout the last 1-2 weeks. At that time the
> performance of the proxy mapping still seemed to be a problem w/ machines with
> large number of nodes. For testing a cluster < 1000 nodes should be used. 
> My initial guess that the regular expression mechanisms might be a problem
> should be ruled out, since I introduced a "memory" into the mapping, which
> bypasses the evaluation after the first lookup and just does a plain map-based
> retrieval.
> 
> I'll take a look at other potential bottlenecks till saturday. Then I hopefully
> have an answer.

I updated my portion of the patch which resolves the performance problem. The performance is now back to where it was. Here's what happened:
The mapping from Parser to unique PBS Attributes was transparent, i.e. each "key/value" pair the parser saw was added as an attribute to respective Element attributes. This caused the "status" attribute of the nodes to trigger a complete update of the respective Node Element, each time the field changed, which effectively resulted in updating all nodes at each polling cycle (2 sec). (The profiler showed that almost all time is spend in generating the protocol transmission string.)

The problem is now temporarily solved: The Parser's default behavior is now to NOT add attributes, which are not explicitly mentioned in the Parser2PBS Mapping. This in combination with NOT monitoring the "status" field of the nodes yields the old behavior. 

The code is not tested for remote connections. If performance problems arise, I recommend commenting out the "jobs" field for nodes in PBSAttributes/Parser2PBS-KeyMap-nodes.txt, since this field might change from time to time, causing a complete update of a node.
Comment 56 Benjamin Lindner CLA 2010-10-01 21:08:03 EDT
Created attachment 180095 [details]
PBS: Read model definition from XML and send to client

Reading of Attribute Definitions and Parser/PBS/Protocol Mappings from text files
Comment 57 Albert L. Rossi CLA 2010-10-04 09:21:05 EDT
Created attachment 180162 [details]
merged proxy and UI patch

I am attaching a patch which merges the proxy changes with the latest UI changes.  From now on, please use this patch and not the preceding ones, as I have eliminated all the UI changes made in Ben's patches.

The optimization does reduce the amount of time it takes to populate the list of queues, though there is still a slight delay, so the user is going to have to refresh the view at least once, or wait to open the view for about 15-20 seconds.

Also, when launching a job, I see an error message:

!ENTRY org.eclipse.ptp.core 4 4 2010-10-04 08:06:33.208
!MESSAGE AbstractProxyRuntimSystem: invalid attribute for definition: java.lang.IllegalArgumentException: No enum const class org.eclipse.ptp.core.elements.attributes.JobAttributes$State.Q

I'm not sure what is going on there.

This may not be the final UI patch, as I am still trying to resolve the issue of the mouse focus.  It seems that the auto-detect mechanism which the Application and Debug tabs apply to validate the remote paths using remotetools code, in the case that the actual RM proxy has not been launched, continues to steal the mouse focus so that the first click -- anywhere on the Launch tab -- is not entirely received (mouseUp gets lost).  I'm not yet sure how to deal with this.  Could we not disable this automatic checking, and only do it when the resource manager proxy is actually running?  If not, I'm open to suggestions ...

Al
Comment 58 Albert L. Rossi CLA 2010-10-04 09:38:24 EDT
Two additional notes.

1.  Along with the server progress bar hang, I still note the job-launch progress bar gets stuck, and not even at 100%, but at "submitting", while the Job viewer does report the job state changes from Q to R, and then the job is removed.  So this behavior is still broken.

2.  Would it be kinder to generate, along with the base template, a standard short template as well (the way I originally did)?  This will ultimately break the server-side encapsulation of attribute knowledge, but I would assume for PBS that queue, account name, job name, nodes, and walltime will always be there (we are already breaking it by looking for "destination" and putting that attribute at the top of the list).
Comment 59 Greg Watson CLA 2010-10-04 09:42:33 EDT
(In reply to comment #57)

> This may not be the final UI patch, as I am still trying to resolve the issue
> of the mouse focus.  It seems that the auto-detect mechanism which the
> Application and Debug tabs apply to validate the remote paths using remotetools
> code, in the case that the actual RM proxy has not been launched, continues to
> steal the mouse focus so that the first click -- anywhere on the Launch tab --
> is not entirely received (mouseUp gets lost).  I'm not yet sure how to deal
> with this.  Could we not disable this automatic checking, and only do it when
> the resource manager proxy is actually running?  If not, I'm open to
> suggestions ...

Disabling this checking unless the RM is running sounds like a good idea. I think currently it will try to open the connection automatically, but this can lead to problems if the host is down, etc.
Comment 60 Benjamin Lindner CLA 2010-10-04 09:57:11 EDT
(In reply to comment #57)
> Created an attachment (id=180162) [details]
> merged proxy and UI patch
> 
> I am attaching a patch which merges the proxy changes with the latest UI
> changes.  From now on, please use this patch and not the preceding ones, as I
> have eliminated all the UI changes made in Ben's patches.
> 
> The optimization does reduce the amount of time it takes to populate the list
> of queues, though there is still a slight delay, so the user is going to have
> to refresh the view at least once, or wait to open the view for about 15-20
> seconds.
> 
> Also, when launching a job, I see an error message:
> 
> !ENTRY org.eclipse.ptp.core 4 4 2010-10-04 08:06:33.208
> !MESSAGE AbstractProxyRuntimSystem: invalid attribute for definition:
> java.lang.IllegalArgumentException: No enum const class
> org.eclipse.ptp.core.elements.attributes.JobAttributes$State.Q
> 
> I'm not sure what is going on there.

The error is probably due to the fact that the PBS Attribute value "Q" (as supplied by the parser) is not mapped to an entry in the protocol. 
Adding 
PBSJOB_STATE :: Q :: COMPLETED
to the file
PBS2Protocol-ValueMap.txt
should do the job.
Comment 61 Albert L. Rossi CLA 2010-10-04 10:09:54 EDT
(In reply to comment #60)
> (In reply to comment #57)
> > Created an attachment (id=180162) [details] [details]
> > merged proxy and UI patch
> > 
> > I am attaching a patch which merges the proxy changes with the latest UI
> > changes.  From now on, please use this patch and not the preceding ones, as I
> > have eliminated all the UI changes made in Ben's patches.
> > 
> > The optimization does reduce the amount of time it takes to populate the list
> > of queues, though there is still a slight delay, so the user is going to have
> > to refresh the view at least once, or wait to open the view for about 15-20
> > seconds.
> > 
> > Also, when launching a job, I see an error message:
> > 
> > !ENTRY org.eclipse.ptp.core 4 4 2010-10-04 08:06:33.208
> > !MESSAGE AbstractProxyRuntimSystem: invalid attribute for definition:
> > java.lang.IllegalArgumentException: No enum const class
> > org.eclipse.ptp.core.elements.attributes.JobAttributes$State.Q
> > 
> > I'm not sure what is going on there.
> 
> The error is probably due to the fact that the PBS Attribute value "Q" (as
> supplied by the parser) is not mapped to an entry in the protocol. 
> Adding 
> PBSJOB_STATE :: Q :: COMPLETED
> to the file
> PBS2Protocol-ValueMap.txt
> should do the job.

Ben,

I added

PBSJOB_STATE :: Q :: QUEUED

(I assume you meant "QUEUED", since C corresponds to "COMPLETED") to the PBS2Protocol-ValueMap.txt file, but I'm still getting the same error message.  The error is not treated as fatal, but it still appears in the events.

Al
Comment 62 Benjamin Lindner CLA 2010-10-04 10:16:48 EDT
(In reply to comment #61)
> (In reply to comment #60)
> > (In reply to comment #57)
> > > Created an attachment (id=180162) [details] [details] [details]
> > > merged proxy and UI patch
> > > 
> > > I am attaching a patch which merges the proxy changes with the latest UI
> > > changes.  From now on, please use this patch and not the preceding ones, as I
> > > have eliminated all the UI changes made in Ben's patches.
> > > 
> > > The optimization does reduce the amount of time it takes to populate the list
> > > of queues, though there is still a slight delay, so the user is going to have
> > > to refresh the view at least once, or wait to open the view for about 15-20
> > > seconds.
> > > 
> > > Also, when launching a job, I see an error message:
> > > 
> > > !ENTRY org.eclipse.ptp.core 4 4 2010-10-04 08:06:33.208
> > > !MESSAGE AbstractProxyRuntimSystem: invalid attribute for definition:
> > > java.lang.IllegalArgumentException: No enum const class
> > > org.eclipse.ptp.core.elements.attributes.JobAttributes$State.Q
> > > 
> > > I'm not sure what is going on there.
> > 
> > The error is probably due to the fact that the PBS Attribute value "Q" (as
> > supplied by the parser) is not mapped to an entry in the protocol. 
> > Adding 
> > PBSJOB_STATE :: Q :: COMPLETED
> > to the file
> > PBS2Protocol-ValueMap.txt
> > should do the job.
> 
> Ben,
> 
> I added
> 
> PBSJOB_STATE :: Q :: QUEUED
> 
> (I assume you meant "QUEUED", since C corresponds to "COMPLETED") to the
> PBS2Protocol-ValueMap.txt file, but I'm still getting the same error message. 
> The error is not treated as fatal, but it still appears in the events.
> 
> Al

from what I see the Protocol doesn't define "QUEUED" as a valid enum state. 
Thus without changing PTP core protocol attribute definitions we're left with mapping anything to what is already defined w/ respect to the protocol.
The fact that you're getting the same error message is surprising. At least you should get a complaint that "QUEUED" is not a valid enum state. 

I'll take a look at it till tomorrow.
Comment 63 Albert L. Rossi CLA 2010-10-04 10:19:39 EDT
(In reply to comment #62)
> (In reply to comment #61)
> > (In reply to comment #60)
> > > (In reply to comment #57)
> > > > Created an attachment (id=180162) [details] [details] [details] [details]
> > > > merged proxy and UI patch
> > > > 
> > > > I am attaching a patch which merges the proxy changes with the latest UI
> > > > changes.  From now on, please use this patch and not the preceding ones, as I
> > > > have eliminated all the UI changes made in Ben's patches.
> > > > 
> > > > The optimization does reduce the amount of time it takes to populate the list
> > > > of queues, though there is still a slight delay, so the user is going to have
> > > > to refresh the view at least once, or wait to open the view for about 15-20
> > > > seconds.
> > > > 
> > > > Also, when launching a job, I see an error message:
> > > > 
> > > > !ENTRY org.eclipse.ptp.core 4 4 2010-10-04 08:06:33.208
> > > > !MESSAGE AbstractProxyRuntimSystem: invalid attribute for definition:
> > > > java.lang.IllegalArgumentException: No enum const class
> > > > org.eclipse.ptp.core.elements.attributes.JobAttributes$State.Q
> > > > 
> > > > I'm not sure what is going on there.
> > > 
> > > The error is probably due to the fact that the PBS Attribute value "Q" (as
> > > supplied by the parser) is not mapped to an entry in the protocol. 
> > > Adding 
> > > PBSJOB_STATE :: Q :: COMPLETED
> > > to the file
> > > PBS2Protocol-ValueMap.txt
> > > should do the job.
> > 
> > Ben,
> > 
> > I added
> > 
> > PBSJOB_STATE :: Q :: QUEUED
> > 
> > (I assume you meant "QUEUED", since C corresponds to "COMPLETED") to the
> > PBS2Protocol-ValueMap.txt file, but I'm still getting the same error message. 
> > The error is not treated as fatal, but it still appears in the events.
> > 
> > Al
> 
> from what I see the Protocol doesn't define "QUEUED" as a valid enum state. 
> Thus without changing PTP core protocol attribute definitions we're left with
> mapping anything to what is already defined w/ respect to the protocol.
> The fact that you're getting the same error message is surprising. At least you
> should get a complaint that "QUEUED" is not a valid enum state. 
> 
> I'll take a look at it till tomorrow.

Yes, you are right, the error reports "QUEUED" as invalid.

What is going to happen, though, to the internal state in the UI when both Q and C are mapped to COMPLETED ?  That doesn't seem right.
Comment 64 Albert L. Rossi CLA 2010-10-04 10:23:15 EDT
Well, as far as I can tell, the mapping only has an impact on which icon is displayed for job state.  But the red icon really should be for completed to stopped.
Comment 65 Greg Watson CLA 2010-10-04 10:40:25 EDT
(In reply to comment #64)
> Well, as far as I can tell, the mapping only has an impact on which icon is
> displayed for job state.  But the red icon really should be for completed to
> stopped.

Most model elements have two attributes: state and status. The state attribute is pre-defined, and has pre-defined icons associated with it. The status attribute is a string and can be used to represent any additional RM-specific status that is required. There is an extension point called "runtimeModelPresentation" which requires an implementation of the IRuntimeModelPresentation interface. This interface provides methods that map an element to an image or text string for display in the UI. An RM can provide an implementation that maps the status attribute to it's own icons. You can see an example of this in the SLURM RM.
Comment 66 Albert L. Rossi CLA 2010-10-04 11:20:44 EDT
Created attachment 180178 [details]
merged proxy and UI patch
Comment 67 Albert L. Rossi CLA 2010-10-04 11:23:29 EDT
The new merged patch now includes

1. The change suggested by Ben (for now; I'll look at the other STATUS interface mentioned by Greg);

2. The addition to SDMPage.java of a check to see that the RM state is STARTED before validating the path.  It turns out it was only the debugger that was doing this auto-check, not the Application Tab.  The flag for the valid path is simply set to true in the case the RM is not running.
Comment 68 Roland Schulz CLA 2010-10-08 10:58:20 EDT
Ben please add the following declaration as a comment for you patch.

I, ..., declare that:

1) This code contains no cryptography.

2) The code has been developed from scratch (i.e. without incorporating content
from elsewhere or relying on the intellectual property of others).

3) All code in the contribution is licensed under the EPL.
Comment 69 Albert L. Rossi CLA 2010-10-08 12:03:47 EDT
Created attachment 180498 [details]
merged proxy and UI patch
Comment 70 Benjamin Lindner CLA 2010-10-08 14:51:29 EDT
I , Benjamin Lindner, declare that:
1) This code contains no cryptography.
2) The code has been developed from scratch (i.e. without incorporating content
from elsewhere or relying on the intellectual property of others).
3) All code in the contribution is licensed under the EPL.


(In reply to comment #68)
> Ben please add the following declaration as a comment for you patch.
> 
> I, ..., declare that:
> 
> 1) This code contains no cryptography.
> 
> 2) The code has been developed from scratch (i.e. without incorporating content
> from elsewhere or relying on the intellectual property of others).
> 
> 3) All code in the contribution is licensed under the EPL.
Comment 71 Benjamin Lindner CLA 2010-10-08 14:55:37 EDT
(In reply to comment #69)
> Created an attachment (id=180498) [details]
> merged proxy and UI patch

As a final comment on this bug. 
The last performance problem is caused by the design of the 
org.eclipse.ptp.proxy.event.AbstractProxyEvent.toString()
method

This method is not scalable,i.e. it performs bad for large number of attributes with significant content. I suggest to open a separate bug report for this.
Comment 72 Albert L. Rossi CLA 2010-10-08 15:16:21 EDT
Created attachment 180515 [details]
merged proxy and UI patch with progress bar fix for bug 310193
Comment 73 Roland Schulz CLA 2010-10-08 18:21:52 EDT
Comment on attachment 180095 [details]
PBS: Read model definition from XML and send to client

attachment 180515 [details] replaces attachment 180095 [details]. But it  can't be marked obsolete yet, because 180515 needs an IP review first.
Comment 74 Greg Watson CLA 2010-10-08 18:35:08 EDT
(In reply to comment #72)
> Created an attachment (id=180515) [details]
> merged proxy and UI patch with progress bar fix for bug 310193

Is this the patch I need to open a CQ for?
Comment 75 Roland Schulz CLA 2010-10-08 18:39:30 EDT
(In reply to comment #74)
> (In reply to comment #72)
> > Created an attachment (id=180515) [details] [details]
> > merged proxy and UI patch with progress bar fix for bug 310193
> 
> Is this the patch I need to open a CQ for?

yes. Is this correct that we should have the CQ for Ben's patch and not Al's (which includes Ben's)? 
I hadn't wrote you yet, because I haven't had time yet, to look over the code once more. If you want to go ahead, please do so, and we fix minor issues (e.g. missing documentation) afterwards. If you want to wait, I'll email you (probably Monday) when I have reviewed it.
Comment 76 Greg Watson CLA 2010-10-08 18:57:27 EDT
i'll wait until monday
Comment 77 Albert L. Rossi CLA 2010-10-08 20:20:15 EDT
(In reply to comment #76)
> i'll wait until monday

Would it be possible for you to resubmit for the necessary review the first patch without the UI changes Ben had originally made?  It would make things simpler for me then to create my own patch on top of that.

Al
Comment 78 Roland Schulz CLA 2010-10-08 20:36:24 EDT
(In reply to comment #77)
> (In reply to comment #76)
> > i'll wait until monday
> 
> Would it be possible for you to resubmit for the necessary review the first
> patch without the UI changes Ben had originally made?  It would make things
> simpler for me then to create my own patch on top of that.
> 
> Al

I'm not sure what you mean. You don't have to create a patch on top of the submitted patch. The patch is submitted, and as soon as it is accepted we'll commit it. Then you can merge that into your workspace. If CVS is not completely stupid it detects that the some of the changes (Ben's part) are identical and auto-merges it. Then you can commit your changes. Am I expecting to much from CVS? (BTW: Git is great! ;-) )
Comment 79 Albert L. Rossi CLA 2010-10-09 11:36:31 EDT
(In reply to comment #78)
> (In reply to comment #77)
> > (In reply to comment #76)
> > > i'll wait until monday
> > 
> > Would it be possible for you to resubmit for the necessary review the first
> > patch without the UI changes Ben had originally made?  It would make things
> > simpler for me then to create my own patch on top of that.
> > 
> > Al
> 
> I'm not sure what you mean. You don't have to create a patch on top of the
> submitted patch. The patch is submitted, and as soon as it is accepted we'll
> commit it. Then you can merge that into your workspace. If CVS is not
> completely stupid it detects that the some of the changes (Ben's part) are
> identical and auto-merges it. Then you can commit your changes. Am I expecting
> to much from CVS? (BTW: Git is great! ;-) )

Not really what I need to do, then  I need to

1.  Make a patch with only my stuff;
2.  revert my workspace to HEAD's current space
3.  Apply Ben's patch.
4.  Apply my patch.

Otherwise, Ben's original UI stuff will clobber mine, or give me conflicts.

I thought I'd just save myself the trouble, cause like programmers are supposed to be, I'm basically lazy.  But no problem.
Comment 80 Albert L. Rossi CLA 2010-10-09 18:38:30 EDT
> 
> Not really what I need to do, then  I need to
> 
> 1.  Make a patch with only my stuff;
> 2.  revert my workspace to HEAD's current space
> 3.  Apply Ben's patch.
> 4.  Apply my patch.
> 
> Otherwise, Ben's original UI stuff will clobber mine, or give me conflicts.
> 
> I thought I'd just save myself the trouble, cause like programmers are supposed
> to be, I'm basically lazy.  But no problem.

Actually, that won't work either, because my patch would be diff'd against the current state of CVS HEAD.  So I guess the only way to do this is to save my own plugins somewhere else until the first patch is actually committed to HEAD.
Comment 81 Albert L. Rossi CLA 2010-10-11 11:26:32 EDT
Created attachment 180604 [details]
merged proxy and UI patch with progress bar fix for bug 310193 plus combo refresh

This merged patch also takes care of refreshing the queue list until the model definition event with the queues arrives.
Comment 82 Greg Watson CLA 2010-10-12 11:55:37 EDT
I still haven't opened a CQ on this. Please let me know which patch requires review.
Comment 83 Albert L. Rossi CLA 2010-10-12 11:59:18 EDT
(In reply to comment #82)
> I still haven't opened a CQ on this. Please let me know which patch requires
> review.
Greg,

the first patch (PBS: Read model definiton...) is Ben's original one.  The latter is all of Ben's original stuff, minus his UI changes, plus my UI and proxy modifications.  So it is the first, I imagine.

al
Comment 84 Greg Watson CLA 2010-10-12 12:30:10 EDT
Ok, I've opened a CQ on the first patch
Comment 85 Benjamin Lindner CLA 2010-10-12 13:22:22 EDT
(In reply to comment #84)
> Ok, I've opened a CQ on the first patch

Although I have activated baseline support there might be a few things which don't follow the recommendations of the developer guidelines. I wanted to double check this. Can I fix anything while the CQ is in progress?
Comment 86 Greg Watson CLA 2010-10-28 10:53:15 EDT
This contribution has been approved. Please apply and close bug when appropriate.
Comment 87 Roland Schulz CLA 2011-12-21 01:06:09 EST
Has been made obsolete by new RM framework.