Hi Erik,
3) You are right. For some reason I thought you meant a GUI.
4 & 6) I think its a great idea it will simplify the user's code. Actually
let me take a crack
at it. So the 2 changes would be as follows: 1) The user provides a map
with instancename -> instanceobject
and 2) the evaluator will silently ignore instances that are not imported
inside the policy
7) I agree XML for the customexpressions is more elegant than properties,
I also agree about externalizing all the in-built expressions
(acplparsermap) as an XML. Looking
forward to your patch.
8) Good point I will add the privileged blocks whereever required
9 & 13) Will look into the logging and cleaning up code
10) I think most of the places we do have statements like this:
if(logger.isLoggable(Level.FINE))
logger.fine(Thread.currentThread().getName()+" iterating over instances "
);
Is that not sufficient or have we missed putting the if in some places?
11) As a matter of fact in our original ANT build we had it built as a
seperate jar but then while migrating to maven
in the interest of time we put it all together. Will work on seperating it
out
12) I suppose this is the maven convention? Again in the interest of time
I didn't change the folder structure, do you see a
compelling reason for this? I noticed some other projects like Apache Muse
weren't following it either
14) I personally don't prefer the Apache Geronimo style but if everyone
feels strongly about this I am OK with changing it.
15) Please do provide a patch.
Again we appreciate your in depth feedback!
Neeraj
3)
>NRJ: The factory is a good idea. What do you have in mind in terms of a
>user interface?
I just thought in rationalizing the current APIs and make all user exposed
methods interface based.
If I understand correctly, there are two kinds of API: User API, Service
Provider Implementation API
User API is the user view to imperius
SPI API is for instance the datastore interfaces, mostly the one in the
external
packages.
4)
>NRJ- There is a direct correlation between the arguments passed to the
>evaluatePolicy method and the imported classes/instances within the
>policy.
>For e.g.
>
>If the import statement in the policy is as follows:
>
>Import Class a : a1,a2;
>Import Class b: b1,b2;
>
>Then the evaluatePolicy method call would have inputMap as the parameter
>
>where
>Map inputMap = ["a" --> instanceInfoListForA]
> ["b" --> instanceInfoListForB ]
>
>where
>instanceInfoListForA = [instanceInfoA1 , instanceInfoA2]
>instanceInfoLIstForB = [instanceInfoB1 , instanceInfoB2 ]
IMO non declared instanceinfo objects could be silently ignored. e.g.
<source>
Map inputMap = ["a" --> instanceInfoListForA]
["b" --> instanceInfoListForB ]
["c" --> instanceInfoListForC ]
where
instanceInfoListForA = [instanceInfoA1 , instanceInfoA2]
instanceInfoLIstForB = [instanceInfoB1 , instanceInfoB2, instanceInfoB3 ]
instanceInfoLIstForC = [instanceInfoC1 ]
</source>
B3 and C1 could be silently ignored since the policy has:
>Import Class a : a1,a2;
>Import Class b: b1,b2;
I can create a patch if you agree.
6) Simplified User API by hiding InstanceInfo?
e.g.
Map map = new HashMap();
map.put("a1", myobjecta1);
map.put("a2", myobjecta2);
map.put("b1", myobjectb1);
map.put("b2", myobjectb2);
spl.executePolicy(name,map);
Internally, PolicyManager.executePolicy can create InstanceInfo.
7) Properties Loader (customexpressions): Currently custom expressions are
configured in a property file placed at the user.dir or splhome. I propose
the
following:
- change properties to xml format
- default naming of the file would change from
customexpressions.properties to
imperius.xml
- imperius.xml would be loaded by default from the locations: cur.dir,
user.dir
and splhome.
- allow configuring customexpressions via the SPLPolicyRuleProvider
interface.
The operation void registerExpressions(InputStream xml) is added to the
SPLPolicyRuleProvider.
<imperius>
<configuration>
<spl-expression class-name="sample.SendMail"/>
<spl-expression class-name="sample.ExecuteCommand"/>
</configuration>
<imperius>
- Externalize ACPLParserMap expressions registration to another XML file:
org.apache.imperius.spl.Expressions.xml
I can also provide a patch.
8) Access to protected resources. System.getProperty, Class.forName, etc
None of the access to these resources happen in privilege blocks, so
imperius
won't work in secured environment.
9) Cosmetic: There are plenty of messages output to system.out. These
should be
moved to log.
10) Logging code should be enclosed within a check condition:
e.g.
if( logger.isDebugEnabled() )
{
logger.debug(xxxx)
}
11) Samples in the JavaSPL should be moved to its own module. I propose
javaspl-samples
12) Unit tests should be moved to /src/test/
12) Sources should be moved from /src/ to /src/java/ or /src/main/
13) Cleanup code: Remove commented out code; Remove empty methods such as
static
mains..; make classes private when possible
14) Code Formatting: use of Apache Geronimo coding style?
15) Prevent nullpointerexceptions. in some cases I got NPE, so need to
prevent
these situations. I propose to create unit tests for the test cases I have
and
provide a patch.
Regards
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
"Those are my principles. If you don't like them I have others."
Neeraj Joshi
Autonomic Computing Policy Development
Tivoli, IBM
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
