----- "Ajay Kumar" ajaykumarns@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?)
- 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'thave 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?
- 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)
- 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 ?
Should Importer#loadExport be renamed to Importer#loadImport?
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.
- 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 takecare 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.
- 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.
- 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@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@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@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/candlepin