Comments on data-export branch

Ajay Kumar ajaykumarns at gmail.com
Mon Jul 19 16:53:31 UTC 2010


On Mon, Jul 19, 2010 at 12:17 PM, Justin Harris <jharris at redhat.com> wrote:
>
> ----- "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.
>

I totally agree with you on this regard. I would like to restate that
I am not advocating for using ArrayList everywhere :). Consider the
file
Principal.java:
We use LinkedList for 'roles'. In my personal opinion, I don't believe
we would need to use LinkedList here...

Or

Consider EntitlementDto.java#144.
Here we could use ArrayList as we know how many elements are going to
be added to the list
List<BigInteger> lst = new ArrayList<BigInteger>(certificates.size())

>>
>> 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
> _______________________________________________
> 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



More information about the candlepin mailing list