Hi Dan,
I've finally fixed it. Sorry it took so long. See my reply below.
On 07/16/2017 06:25 PM, Dan Haywood wrote:
Hi Erik,
Thanks for taking the time to pull out this ElasticSearch functionality
into a new module.
I don't think it's necessary to try to enumerate all possible functionality
... it's far better to just let the use cases drive out the functionality.
So I'd rather that the currently working functionality is documented and
made available, than try to second guess what new features might be
required.
Meantime, I've had a look over the code, and have some observations, some
trivial and some less so.
First, some trivial points:
https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/ElasticSearchService.java#L58
- don't think there's any reason for this field to be static ?
Fixed. No, no good reason. I was playing around because at some point I
noticed multiple clients were created in our application and I wanted to
find out how this happened.
https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/ElasticSearchService.java#L42
- "cluster.name" ... tend to put a prefix of some sort, eg
"isis.services.audit.objects=all" suggest "isis.services.elasticsearch."
as the prefix
Fixed
https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/resources/org/isisaddons/module/elasticsearch/search/result/SearchResult.layout.xml
for some reason in src/main/resources, whereas other layouts in
src/main/java
Think that src/main/java is the way to go
Fixed
https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexServiceTest.java
this is a test class, should be in src/test/java
Fixed. No test-directory existed and I didn't notice the files were
created in the java-directory.
https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/AbstractIndex.java
as a naming convention, I tend to use "Abstract" as a suffix
Fixed.
https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java#L90
settings seem temporarily to be disabled/commented out?
Some setting might be application specific because different
applications might require diffent search/index behaviour. At some time
I was testing different indexing strategies to improve our search
engine. I didn't know what to do with this so I just left it there. This
could be solved by adding a hook like you mention below.
https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java#L31
I tend to mark these as @Programmatic, to keep the size of the metamodel
down (avoid
Fixed
https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java#L123
and
https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java#L133
and
https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java#L157
and
https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java#L168
recommend to just throw a new RuntimeException(e) so we can fail fast
rather than swallow exceptions
Fixed. I should take a better look at exception handling. It's easy to
make a mess of that.
https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexerFactory.java
this looks like an SPI, so typically put these in an 'spi' subpackage
Fixed
https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/search/result/SearchResult.java
and
https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/search/result/SearchResultsPage.java
recommend making these JAXB view model rather than using @ViewModel; more
flexible.
Fixed. Didn't use any JAXB view models yet.
https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/search/result/SearchResultsPage.java#L60
can use @Nullable here instead of @Parameter(optionality =
Optionality.OPTIONAL)
Fixed. Will the optionality-property disappear in future versions of
Apache Isis. Is there any difference?
https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/search/result/SearchResult.java#L38
suggest use the ?: ternary operator
Fixed.
https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java
? quite a few of the methods in IndexService aren't called, and/or seem
incomplete:
- addMappings
- initialiseSearchEngine
- addToBulkRequest
Like mentioned above 'addMappings' could be a hook to improve/change
index/search behaviour.
We call 'initialiseSearchEngine' every night to recreate the index. Our
database is used by multiple applications (and sometimes even updated by
hand :-/ ) and these applications also update the same ElasticSearch
index. We recreate the index just to be sure it's up to date at least at
the beginning of the day. I can imagine this is not really needed for
every application.
'addToBulkRequest' is used by above initialisation.
https://github.com/erikdehair/isis-module-elasticsearch/blob/master/fixture/src/main/java/org/isisaddons/module/elasticsearch/fixture/dom/ElasticSearchDemoObject.java#L67
the implementation could be simplified to:
Bookmark bookmark = bookmarkService2.bookmarkFor(this);
//return bookmark.getObjectType() +":"+ bookmark.getIdentifier()
return bookmark.toString();
and then perhaps this could be moved into the caller, with Indexabe
interface defining getIndexIsd() as optional (return null):
- make
default String getIndexId() { return null; }
ie, and if returns null, then have the caller work out the Id using the
bookmark
Fixed
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
So much for the trivial stuff; I also have some "deeper" questions on the
design.
As I understand the code, we use Indexable as an interface to determine -
via the IndexableSubscriber - which domain objects get added to the
ElasticSearch index. To do that, it obtains a corresponding Indexer for
the domain object, which then returns an AbstractIndex - basically a DTO
which is an extract of the relevant fields. This is serialized to JSON.
I see that Index defines a getTenancy() property, and initially wondered
whether that made sense, but on second thoughts I think it *does *make
sense to include it in the JSON .. applications that don't need app tenancy
can just use "/" as a global value. I'd like to pull this concept of
tenancy into a separate module within incode-platform, but that can wait
til a later date for now.
The search search now only returns the first (I believe) 25 results from
ElasticSearch. If the current user isn't allowed to see (some of) these
results then no (or less) results will be displayed while the index
contains a lot of items matching the search criteria the user ís allowed
to see. So we have to check the tenancy while searching, so
ElasticSearch only returns items the current user is allowed to see.
I also see that AbstractIndex has a getType() property. Is this
necessary? Why not just use the ObjectSpecId (as per
@DomainObject#objectType) of the object being indexed?
This might give problems when using multiple applications talking to the
same index. At least there must be a common way to mark the indexed
items with an id both applications understand.
(One possible answer is if we wanted to bundle indexes of various different
domain objects - perhaps different subclasses into a single index, like a
"roll-up". But does ElasticSearch do a query across all indexes anyway?
If so, then probably no need to have getType() property)
Still on AbstractIndex, I wonder if createJson needs to be defined upon
it? Why not just have its caller (IndexService) do this serialization?
Probably better than let the index transform itself (this is only
something that happens in movies)
Looking at Indexer :
https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/Indexer.java
first question is, why is this an abstract class rather than an interface ?
Right now the index-field of the Indexer-class is used by subclasses. It
could be changed to an interface but then the creation of the JSON must
be moved outside the Indexer-classes and that's the only thing they do now.
But a deeper question is... was wondering if we can get rid of Indexer, or
at least remove it from the SPI? All it seems to do is to copy out
selected properties from the domain object into the corresponding Index
(with that Index then just being serialized out to JSON). Perhaps a
different design would be extend @Property attribute to indicate which
fields are to be included within the index? Then we can obtain the data to
be indexed via the metamodel?
- or, can do this with a custom annotation (cf summernote and pdfjs), eg
@ElasticSearchProperty
- with additional facets
I was wondering this too but I'm not yet confident enough with the meta
model to refactor this using annotations.
The way I implemented it now, we have to add some new classes and
register these to the IndexerFactory and add methods to the domain
objects we want to be indexed also. This is not an ideal situation. The
same for the preferred types and their boosting factor.
If it is to be kept as an SPI, then perhaps allow multiple implementations
of IndexerFactory to be implemented (with these injected using @Inject
List<IndexerFactory>).
I have a feeling that maybe some SPI is required for settings too; maybe
these concepts can be unified?
I don't exactly understand your question.
~~~~~~~~~
Hope the above is of use.
As you might recall from the mini-conference, I've been pulling together
all the existing addons into a single "platform" [1]. This is till work in
progress, but once I have that assembled and restructured, I intend to put
some coding standards/guidelines as to what should be in a framework module
- as you rightly point out, the code is written for re-use so can make
fewer assumptions about how it is called, and should provide more hooks.
For example, one of the things I've tried to do for any modules that define
entities - eg security - is to ensure that every property/colleciton/action
is annotated with a domain event; this makes it easy for consuming
applications to selectively hide or disable functionality. There's also a
hierarchy for these domain event classes. Another example might be to use
title events rather than title() - I haven't done this consistently but
would like to; again it provides a consistent hook for consuming apps.
Another point is to ensure that any public API (as used by domain objects)
should be in a separate maven module. This avoid polluting the classpath
of the domain modules with unnecessary implementation classes (eg the
ElasticSearch client).
What I would like to do though, if you're happy, is to bring in this
ElasticSearch into the incode-platform. How does that sound?
Would be honored ;-)
Thanks again
Dan
[1] https://github.com/incodehq/incode-platform
On Thu, 13 Jul 2017 at 18:39 Erik de Hair <e.deh...@pocos.nl> wrote:
Right now the module can do indexing and a basic query can be executed
to find some different types of demo objects and boost a particular type
of demo object, so it will show up at the top of the result list.
Still have to simplify some things and write a readme/documentation. The
demo application is a based on the module template so it can be run as
every other module demo application. The only thing is: you need an
installed ElasticSearch cluster/node.
The question about this elastic search module is: what features should
it offer?
What i've implemented is:
* Manage connection to a ES cluster
* Indexing of entities marked as Indexable
* A basic search service that offers an action to find some entity based
on the content as defined by the 'Indexer' of that entity.
* A search results page to show the results in descending order of the
search score and to refine your previous search.
* Boosting of some preferred type to give it a higher score.
Writing a framework module is something different than implementing
one's company's domain model. It's hard to decide how things should be
done and to determine the level of how generic it must be. So far I've
only been trying to let it do what it should do. Didn't spend much time
on the architecture. Reviews/comments very welcome!
Erik
On 06/22/2017 03:13 PM, Erik de Hair wrote:
Hi all,
As promised, I started working on an ElasticSearch module. It can be
found at [1]. There's still a lot to do but at least it's running and
indexing demo objects. I'll keep you updated about the progress. I
probably have to write a README before you can give it a try.
Erik
[1] https://github.com/erikdehair/isis-module-elasticsearch
