Comments on data-export branch

Justin Harris jharris at redhat.com
Mon Jul 19 16:17:31 UTC 2010


----- "Ajay Kumar" <ajaykumarns at gmail.com> wrote:

> Please find my comments/suggestions below:
> 
> 1.RulesImporter.java{#42} new BufferedReader(..) is instantiated and
> not used after that(perhaps forgot to clean it up?)
> 
> 2. LinkedList used instead of ArrayList - This is more of a general
> comment. I have noticed that LinkedList is used in a lot of places
> within candlepin and it is my understanding that LinkedList is very
> slow than ArrayList and most folks use ArrayList unless there is high
> memory constraint.

This really depends on the use case - there is a reason that both implementations
are included.  If all you are really doing is adding to a collection and then looping
over the whole thing, then LinkedList is perfectly reasonable and probably preferred, as
adding elements to a LinkedList can be more efficient overall if you exceed the size of the
underlying array in an ArrayList, as it will need to allocate a new array and copy all of the
elements over.

However, if you need to access elements by their index, then by all means use an ArrayList, because
afaik the LinkedList implementation has to traverse the list from the head in order to access the
element.

> 
> 3, logged.debug/info used without guard statements. Again, this is
> more of a general statement(lot of occurrences within candlepin).
> logging without guard statements makes the code run slower. I suggest
> that we migrate our logging statements to slf4j framework. Migrating
> to slf4j gives us the following advantages
>    i) You don't have to use guard statements at all.
>            for example, if you want to log
>                       if(logger.isDebugEnabled())
>                         logger.debug("loading entitlements for
> consumer: " + consumer.getId()) //log4j
>                       }
>            In slf4j, you write.
>                        logger.debug("loading entitlements for
> consumer: {}", consumer.getId()) //slf4j
> 
>         Note that the log statements look cleaner and also you don't
> have to write guard statements. If debug is not enabled, slf4j does
> not perform template substitution. So no performance impact.
> 
>    ii) slf4j is not a separate logging framework. It delegates log
> calls to either log4j/java's util.logger/ logback etc... So, we can
> continue to configure our logging formats in log4j / change the
> backend logging framework anything else.
>    iii) No extra jars to add - hibernate already uses slf4j.
>    iv) template substitution.
>     and more...
>  what say you?
> 
> 4. Can we make the 'buf len' Exporter#addFilesToArchive and
> Importer#extractArchive configurable? I have personally seen
> performance benefits when the buf len is more than 1024. Again this
> depends on the underlying file system etc.. Perhaps should we move
> all
> zip handling code into separate utility classes?(they are private
> methods right now)
> 
> 5. I am seeing this pattern lot of times within our code {More of a
> general statement.. applies to all code within candlepin}
>          mapping id to entity. Converting Set<Entity> => Map<Id,
> Entity>. Because all the domain objects inherit from
> AbstractHibernateObject,(all inherit getId from this class) can we
> have a general utility method to take of this mapping ?
> 
> 6. Should Importer#loadExport be renamed to Importer#loadImport?
> 
> 7. In my personal opinion the
>         i) *Exporter.java classes seem to be just delegating call to
> jackson's ObjectMapper#writeValue. Do we really need these classes? I
> can understand using these classes if jackson's classes were not
> exposed but rather used within these classes.(encapsulating
> dependency)
>   ii) *Importer.java seem to just delegate to jackson's
> ObjectMapper#readValue and set id to null. Again, do we need this
> abstraction? In my opinion, the store method should be moved to
> curator level as it is replicating store functionality.
> 
> 8. I also believe Exporter and Importer.java seem to bear lot of
> heavy
> weight... In my opinion,
> 
>        *) We could have an OwnerImporter/Exporter which would take
> care of importing all entities within an owner and
>               -> handing down consumer import/export to
> ConsumerImporter/Exporter...
>                    -> handing down Entitlement import/export to
> EntitlementImporter/Exporter
> 
>     and so on.. something like a chain of responsibilities.
> 
> 9. Again this is more of a general comment. I have seen that we are
> using rules engine within the code to restrict access/etc.. In my
> opinion rules implementation could be moved into a separate AOP
> layer/code.
> 
> 10. The existing AOP code which uses annotation to restrict access to
> db does not seem to cache the class -> annotation relationship. I
> believe if this is done, we will see a lot of performance
> improvements.
> 
> On Fri, Jul 2, 2010 at 2:24 PM, Bryan Kearney <bkearney at redhat.com>
> wrote:
> > Exporting:
> >
> > - I assume that signing ill be added to the export, and verification
> to
> > the import?
> > - Does the use of process builder / tmp directories mean we can not
> run
> > in a J2EE container?
> > - Where do you make the split between putting code into hte
> exporter
> > methods and the exporter?
> > - I see "MKT" around the code. Can we do a "hasProductCert" check
> instead.
> > - Are hte product ids stored, or re-generated?
> > - We will need the cert and json for the entitlement.
> > - Can we throw an export eventin there?
> >
> > Importing:
> > - Consumer becomes owner.
> > - I assume a second path will be to look for updates.. this code
> appears
> > to be create only.l
> > _______________________________________________
> > candlepin mailing list
> > candlepin at lists.fedorahosted.org
> > https://fedorahosted.org/mailman/listinfo/candlepin
> >
> 
> 
> 
> -- 
> with regards
>      Ajay Kumar.N.S
> http://www.linkedin.com/in/ajaykumarns
> _______________________________________________
> candlepin mailing list
> candlepin at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/candlepin



More information about the candlepin mailing list