[
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.
