Hi Dan,
Thanks for the detailed feedback. I probably don't have time to do any
changes the next few weeks but will do that in August.
Erik
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 ?
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
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
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
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
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?
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
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
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
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.
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)
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
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
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
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
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.
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?
(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?
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 ?
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
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?
~~~~~~~~~
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?
Thanks again
Dan
[1] https://github.com/incodehq/incode-platform
On Thu, 13 Jul 2017 at 18:39 Erik de Hair <[email protected]> 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
