<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jul 24, 2013 at 8:14 PM, Cheng Fang <span dir="ltr">&lt;<a href="mailto:cfang@redhat.com" target="_blank">cfang@redhat.com</a>&gt;</span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  
    
  
  <div bgcolor="#FFFFFF" text="#000000"><div class="im">
    <br>
    <div>On 7/24/13 12:52 PM, Stuart Douglas
      wrote:<br>
    </div>
    <blockquote type="cite">
      <div dir="ltr">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.
        <div><br>
        </div>
        <div>1) JdbcRepository seems to save jobs to the database but
          never seems to actually load them again or remove them? [1]</div>
      </div>
    </blockquote></div>
    JdbcRepository is still work in progress, and is currently not used
    yet.<div class="im"><br>
    <blockquote type="cite">
      <div dir="ltr">
        <div><br>
        </div>
        <div>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 <a href="https://github.com/jberet/jsr352/blob/master/jberet-core/src/main/java/org/jberet/repository/JdbcRepository.java#L36" target="_blank">i</a>n
          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. <br>
        </div>
      </div>
    </blockquote></div>
    See my reply to previous message.  Initially I did implement it as a
    map, but didn&#39;t like duplicating id as the key so changed it to
    list.  I don&#39;t expect the number of jobs to be that large, or access
    to jobs to be a hot spot.  But I&#39;m open to switch it since the
    feedback so far has favored a mapping lookup.<div class="im"><br>
    <blockquote type="cite">
      <div dir="ltr">
        <div><br>
        </div>
        <div>3) Thread safety</div>
        <div>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&#39;t know the code well enough to be sure. <br>
        </div>
      </div>
    </blockquote></div>
    If most of the code is synchronized, I would also be worried<span><span> ;-)     </span></span>.  But I
    agreeed, thread safety is the area we need to look more closely as
    we integrate to WildFly.  In {2], what&#39;s your recommendation? to
    always return a new list the the caller, which seems a bit wasteful.</div></blockquote><div><br></div><div><br></div><div>You must either return a new list or use a concurrent list as the backing data structure.</div>
<div><br></div><div>Stuart</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div class="im"><br>
    <blockquote type="cite">
      <div dir="ltr">
        <div><br>
        </div>
        <div>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. <br>
        </div>
      </div>
    </blockquote></div>
    jberet.properties is only for standalone distro.  For running in
    WildFly, all configuration will be included in subsystem
    configuration.<br>
    <br>
    Appreciate all the feedback!<span class="HOEnZb"><font color="#888888"><br>
    <br>
    Cheng</font></span><div><div class="h5"><br>
    <blockquote type="cite">
      <div dir="ltr">
        <div><br>
        </div>
        <div>Stuart</div>
        <div><br>
        </div>
        <div>[1] <a href="https://github.com/jberet/jsr352/blob/master/jberet-core/src/main/java/org/jberet/repository/JdbcRepository.java#L36" target="_blank">https://github.com/jberet/jsr352/blob/master/jberet-core/src/main/java/org/jberet/repository/JdbcRepository.java#L36</a></div>

        <div>[2] <a href="https://github.com/jberet/jsr352/blob/master/jberet-core/src/main/java/org/jberet/runtime/JobExecutionImpl.java#L134" target="_blank">https://github.com/jberet/jsr352/blob/master/jberet-core/src/main/java/org/jberet/runtime/JobExecutionImpl.java#L134</a></div>

        <div><br>
        </div>
      </div>
      <div class="gmail_extra"><br>
        <br>
        <div class="gmail_quote">On Wed, Jul 24, 2013 at 6:09 PM, David
          M. Lloyd <span dir="ltr">&lt;<a href="mailto:david.lloyd@redhat.com" target="_blank">david.lloyd@redhat.com</a>&gt;</span>
          wrote:<br>
          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On initial
            review of JBeret we have noticed a number of issues that
            need<br>
            to be addressed.  The culmination amounts to a series of
            questions and<br>
            observations here:<br>
            <br>
            #1) Why did we not choose to just use the RI?  In other
            words, what<br>
            benefit do we get from JBeret that is not also in the RI?
             In other,<br>
            other words, why should we *use* this code instead of the RI
            at this<br>
            point in time?<br>
            <br>
            #2) Why does JBeret duplicate facilities already present in
            the WildFly<br>
            code base and deployer chain - e.g. annotation indexing,
            reflection<br>
            indexing, thread management, parsing facilities, etc.?<br>
            <br>
            #3) Specific to algorithmic complexity - it appears that
            jobs are keyed<br>
            by ID, yet accessed using a sequential search [1] - this
            does not scale<br>
            well to large numbers of jobs.  Is there no better approach?<br>
            <br>
            #4) JAXB seems to be being used to parse XML, which is a
            departure from<br>
            all of our other services which expect parsing to be done
            during<br>
            deployment processing in a more efficient manner.  Is there
            any better<br>
            way we can integrate this, preferably not using JAXB?<br>
            <br>
            #5) There are a number of resources present that seem
            inappropriate for<br>
            the production JAR [2] [3].  Is this intentional?<br>
            <br>
            #6) This code base makes extensive use of static state,
            including static<br>
            fields that seem not to be adequately protected for
            thread-safety, and<br>
            at least one static thread pool [4].  This needs to be
            fixed, as these<br>
            kinds of things make embedding difficult or impossible.<br>
            <br>
            [1]<br>
            <a href="https://github.com/jberet/jsr352/blob/master/jberet-core/src/main/java/org/jberet/repository/AbstractRepository.java#L50" target="_blank">https://github.com/jberet/jsr352/blob/master/jberet-core/src/main/java/org/jberet/repository/AbstractRepository.java#L50</a><br>

            [2]<br>
            <a href="https://github.com/jberet/jsr352/tree/master/jberet-core/src/main/resources/sql" target="_blank">https://github.com/jberet/jsr352/tree/master/jberet-core/src/main/resources/sql</a><br>
            [3]<br>
            <a href="https://github.com/jberet/jsr352/blob/master/jberet-core/src/main/resources/jobXML.xjb" target="_blank">https://github.com/jberet/jsr352/blob/master/jberet-core/src/main/resources/jobXML.xjb</a><br>
            [4]<br>
            <a href="https://github.com/jberet/jsr352/blob/master/jberet-core/src/main/java/org/jberet/util/ConcurrencyService.java#L22" target="_blank">https://github.com/jberet/jsr352/blob/master/jberet-core/src/main/java/org/jberet/util/ConcurrencyService.java#L22</a><br>

            <span><font color="#888888"><br>
                --<br>
                - DML<br>
                _______________________________________________<br>
                wildfly-dev mailing list<br>
                <a href="mailto:wildfly-dev@lists.jboss.org" target="_blank">wildfly-dev@lists.jboss.org</a><br>
                <a href="https://lists.jboss.org/mailman/listinfo/wildfly-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/wildfly-dev</a><br>
              </font></span></blockquote>
        </div>
        <br>
      </div>
      <br>
      <fieldset></fieldset>
      <br>
      <pre>_______________________________________________
wildfly-dev mailing list
<a href="mailto:wildfly-dev@lists.jboss.org" target="_blank">wildfly-dev@lists.jboss.org</a>
<a href="https://lists.jboss.org/mailman/listinfo/wildfly-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/wildfly-dev</a>
</pre>
    </blockquote>
    <br>
  </div></div></div>

<br>_______________________________________________<br>
wildfly-dev mailing list<br>
<a href="mailto:wildfly-dev@lists.jboss.org">wildfly-dev@lists.jboss.org</a><br>
<a href="https://lists.jboss.org/mailman/listinfo/wildfly-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/wildfly-dev</a><br>
<br></blockquote></div><br></div></div>