6:52 p.m.
I have also been looking at this today, and there are quite a few things in
the code base that worry me about its quality.
1) JdbcRepository seems to save jobs to the database but never seems to
actually load them again or remove them? [1]
2) Some things seem to be implemented in a very inefficient manner, using
lists when a map or a set would be more appropriate. For example
i<https://github.com/jberet/jsr352/blob/master/jberet-core/src/main/ja...
AbstractJobRepository all jobs are stored in a list, and as a result every
operation on the repo is O(n) on the number of jobs. A map would be a far
more suitable data structure here. This will be a real problem is a
customer is ever trying to scale to even a moderately sized number of jobs
and job instances.
3) Thread safety
Almost all objects in the code base are mutable (i.e. no use of final), and
with a few exceptions most of the code is not synchronized. From what I can
see not much thought has been given to thread safety, and looking through
the code I think there are quite a few places where there are the potential
to have threading issues. e.g. In [2], where a list that is being modified
concurrently is returned to the caller. The caller cannot safely use the
list, as it may be modified by another thread as it is being iterated.
There are other places where I think there are the potential for races,
however I don't know the code well enough to be sure.
4) It looks like it has been designed as a standalone project to be
embedded into a deployment, and no thought has been given to how to
actually integrate it into Wildfly. I know David already mentioned the
statics issue, but this is a big problem. e.g. only one jberet.properties
will be loaded, so if two applications have different properties files then
one will leak into the other app, depending on the current TCCL when the
BatchConfig class is first accessed.
Stuart
[1]
https://github.com/jberet/jsr352/blob/master/jberet-core/src/main/java/or...
[2]
https://github.com/jberet/jsr352/blob/master/jberet-core/src/main/java/or...
On Wed, Jul 24, 2013 at 6:09 PM, David M. Lloyd <david.lloyd(a)redhat.com>wrote:
On initial review of JBeret we have noticed a number of issues that
need
to be addressed. The culmination amounts to a series of questions and
observations here:
#1) Why did we not choose to just use the RI? In other words, what
benefit do we get from JBeret that is not also in the RI? In other,
other words, why should we *use* this code instead of the RI at this
point in time?
#2) Why does JBeret duplicate facilities already present in the WildFly
code base and deployer chain - e.g. annotation indexing, reflection
indexing, thread management, parsing facilities, etc.?
#3) Specific to algorithmic complexity - it appears that jobs are keyed
by ID, yet accessed using a sequential search [1] - this does not scale
well to large numbers of jobs. Is there no better approach?
#4) JAXB seems to be being used to parse XML, which is a departure from
all of our other services which expect parsing to be done during
deployment processing in a more efficient manner. Is there any better
way we can integrate this, preferably not using JAXB?
#5) There are a number of resources present that seem inappropriate for
the production JAR [2] [3]. Is this intentional?
#6) This code base makes extensive use of static state, including static
fields that seem not to be adequately protected for thread-safety, and
at least one static thread pool [4]. This needs to be fixed, as these
kinds of things make embedding difficult or impossible.
[1]
https://github.com/jberet/jsr352/blob/master/jberet-core/src/main/java/or...
[2]
https://github.com/jberet/jsr352/tree/master/jberet-core/src/main/resourc...
[3]
https://github.com/jberet/jsr352/blob/master/jberet-core/src/main/resourc...
[4]
https://github.com/jberet/jsr352/blob/master/jberet-core/src/main/java/or...
--
- DML
_______________________________________________
wildfly-dev mailing list
wildfly-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/wildfly-dev