camel-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ashwin Karpe (JIRA)" <j...@apache.org>
Subject [jira] Commented: (CAMEL-1472) Lucene Component
Date Tue, 05 Jan 2010 05:55:15 GMT

    [ https://issues.apache.org/activemq/browse/CAMEL-1472?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=56745#action_56745
] 

Ashwin Karpe commented on CAMEL-1472:
-------------------------------------

Hi Claus,

I have made the needed changes and attached the new patch and zip files following modification
and appropriate testing.

Please find the updates based on your suggestions

Cheers,

Ashwin...

===============================================
Having the following comments, in a bit of messy order as its type as I looked at the code:
1) Why does JettyConfiguration have a LuceneComponent field? I would assume a xxConfiguration
is a simple object that is like a getter/setter that can easily be configured in Spring XML
but also can do some URL parsing as well. It should likely also have a default no-arg ctr
so you can create it from spring XML more easily. Ah you should probably pass the Component
in the parseURL method so you can use it locally only in this method and leave all the getter/setter
for easy Spring XML config as well.
Ashwin: Done

2) The code for parsing the authority could be a bit smarter, however we only have 2 operations
so I assume it do. 
Ashwin: Done

3) I dont like the default of the indexDir to be target/index which smells like a maven folder
for unit testing
Ashwin: Done. Changed target/index to ./indexDirectory

4) The getMaxHits code could be prettier, eg use that getAndRemoveParameter and then put it
in settings afterards. Then you can use Camel Type convert to get it as a integer and avoid
the ugly JDK converter and type case code
Ashwin: Was unclear on what to do here. Did not change.

5) The operation parameter can be an enum and Camel can auto convert to it from a String (case
insensitve AFAIR). Just a note.
Ashwin: Was unclear on what to do here. Did not change.

6) No need to INFO log creating a producer. camel-core have logging for creating consumers/producers
already. And INFO logging is to high and the logging info does not provide good details anyway.
So please remove it.
Ashwin: Done

7) LuceneEndpoint is preferred to have a default no arg ctr as well and have getter/setter
to configure it so you can create that endpoint easily as well. There are some components
which is not possible like that, eg Jetty, Http etc. But this one is simple enough for that.
Ashwin: Done

8) Why is the endpoint non singleton? Most endpoints can be singleton out of the box
Ashwin: Done. Changed to singleton

9) The debug loggin in LucenIndexer looks like TRACE logging to me. Try to mimimize DEBUG
logging a bit and use TRACE for the very verbose stuff
Ashwin: Done.

10) I wonder if the body cannot be converted to String should the indexer skip the entire
Exchange? Or should some exception be thrown. You can use getMandatoryBody if the body must
exist.
Ashwin: Done

11) LuceneProducer. Remove that INFO logging which has no value. camel-core has logging already
for starting and stopping resource in Camel
Ashwin: Done

12) I dont think you need to store endpoint in LuceneProducer as the super got it already
Ashwin: Done

13) Dont use NPE exception but throw an IllegalArgumentException instead if some parameters
or value is missing. 
Ashwin: Done

14) LucenseSearcher again use TRACE logging for that kind of logging
Ashwin: Done

15) Dont use IOConverter directly to just do a new File(folder) to avoid that hard dependency
Ashwin: Done

16) LuceneQueryProcessor - Dont swallow exceptions and dont use e.printStrackTrace(). This
error seems so severe that the exception should be rethrown
Ashwin: Done

17) Remove code that is commented out
Ashwin: Done. Sorry about that. It slipped in :)

18) Why does the search return data as a XML String. Why not have some POJO class with the
search result. And let end user marshal to XML if you really need XML. I am not keen on this.

Ashwin: Done. Changed to data structure and removed the JAXB marshaller, schemas etc.

And use isXXXEnabled for those loggers to avoid overhead when that level is not enabled
Ashwin: Done
====================================================================


> Lucene Component
> ----------------
>
>                 Key: CAMEL-1472
>                 URL: https://issues.apache.org/activemq/browse/CAMEL-1472
>             Project: Apache Camel
>          Issue Type: New Feature
>            Reporter: Claus Ibsen
>            Assignee: Ashwin Karpe
>             Fix For: Future
>
>         Attachments: camel-lucene-20100104.patch, camel-lucene-20100104.zip
>
>
> We should add a new component for Apache Lucene integration

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message